Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't suggest using mutable keyword for parameters #15803

Open
psfinaki opened this issue Aug 15, 2023 · 7 comments
Open

Don't suggest using mutable keyword for parameters #15803

psfinaki opened this issue Aug 15, 2023 · 7 comments
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language.
Milestone

Comments

@psfinaki
Copy link
Member

This is misleading:
image

The error is okay I think, we just shouldn't suggest making parameters mutable, this is illegal.

@github-actions github-actions bot added this to the Backlog milestone Aug 15, 2023
@psfinaki psfinaki changed the title Don't suggest use mutable keyword for parameters Don't suggest using mutable keyword for parameters Aug 15, 2023
@Happypig375
Copy link
Member

Instead it could suggest to redefine the x as mutable with a let mutable x = x.

@psfinaki
Copy link
Member Author

@Happypig375 mhm this would be compilable code indeed, I just wonder if there can really be a valid use case for that - as in, any real world code where you would recommend a developer such an approach instead of e.g. refactoring code to remove mutables :)

@smoothdeveloper
Copy link
Contributor

@psfinaki, it would make sense to have another error message for this case:

Suggestion, but there is probably better way to phrase it.

The x binding is an immutable parameter, consider defining a local mutable binding or adjusting the parameter to byref.

related: #1103

@psfinaki
Copy link
Member Author

Yeah I think I would be cautious with the word "consider" and be more like "if you for some weird reason intended to ... then ..." :) But yeah, that's details, more important it should be just a different diag/message.

@vzarytovskii
Copy link
Member

@Happypig375 mhm this would be compilable code indeed, I just wonder if there can really be a valid use case for that - as in, any real world code where you would recommend a developer such an approach instead of e.g. refactoring code to remove mutables :)

Yes, when you want to make defensive copy.

@T-Gro T-Gro added Area-Diagnostics mistakes and possible improvements to diagnostics Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language. and removed Needs-Triage labels Aug 21, 2023
@T-Gro
Copy link
Member

T-Gro commented Aug 21, 2023

Using a ref<> here in case of a parameter would be probable a good suggestion (or at least mentioned in it).
Or shadowing with let mutable x = x.

@T-Gro T-Gro added the Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. label Aug 21, 2023
@voronoipotato
Copy link
Contributor

voronoipotato commented Sep 28, 2023

Suggesting making the parameter mutable is more hostile to beginners than it is helpful to experienced F# devs. We can acknowledge that a new binding would solve the issue as most people who run across this issue are learning F# for the first time, do not need to mutate it, and there may be a good reason their teammates made it immutable in the first place 😃 .

The x binding is an immutable parameter, consider defining a new variable or adjusting the parameter to byref.

This way, it doesn't actively encourage a mutable copy when most beginners learning the language probably would have an easier time with a new immutable variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Bug Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language.
Projects
None yet
Development

No branches or pull requests

6 participants