Emit special warning when = is used and user might want to use <- #1115

Merged
merged 8 commits into from Dec 3, 2016

Projects

None yet

9 participants

@forki
Contributor
forki commented Apr 25, 2016

This implements #1109

Before:

image

After:

image

@forki forki changed the title from Emit special warning when = is used and user might want to use <- to WIP: Emit special warning when = is used and user might want to use <- Apr 25, 2016
@KevinRansom
Contributor
KevinRansom commented Apr 25, 2016 edited

@forki I'm not sure I like the new message any better than the old one:

  1. It is a wall of text, with much more to read.
  2. The first choice using the <- operator will just result in a different compile error unless the developer also makes the field mutable.
  3. It doesn't offer the mutable warning when the "assumed assignment happens at the end of the function.

Whilst the current message is irritating in how commonly it occurs, the usual fix at least for me ... is to add an ignore.

Perhaps the warning message could be simplified to something like:

"This result of this expression will be discarded by a later expression in the function. Use ignore to indicate that this is expected or use let to bind the value to a name."

@forki
Contributor
forki commented Apr 25, 2016 edited

The concrete wording is not really finished, but in this case it's
important to say more than only the thing about "ignore". Using = instead
of <- is a common issue for newcomers. We should do our best to help
newcomers to overcome this issue. Maybe we should not advice to use <-
here, but we could still explain more about that issue.

@smoothdeveloper
Contributor

I got used to (and very much like) the warning of ignored return value.

When I see a warning (blue squiggles) on such expression, I now recognize the fact that I have a discarded value and fix it with |> ignore, but when I was beginning, I kept doing the same mistake, and got burnt few times with it.

I agree that the = case is a special case (not for other expressions) and it will help beginners to refer about <- assignment operator.

I can already see how extending VFPT to propose different quick fixes for both cases would make it feel more like Resharper ๐Ÿ˜„

@KevinRansom
Contributor
KevinRansom commented Apr 25, 2016 edited

@smoothdeveloper @forki I accept that c#, c++ and c developers associate = with assignment. However :

let change () =
     x = 1
     y = "Test"

Who of us is to say they didn't intend to mutate both x and y. But they wouldn't get an error when they mistyped y <- "Test" because y = "Test" is valid code at that point in the program.

Encouraging mutation is probably not a useful thing to do anyway.

@smoothdeveloper
Contributor
smoothdeveloper commented Apr 25, 2016 edited

@KevinRansom this is sensible about not advertising mutation.

In your code I guess we would like:

The operator '=' is used for comparison but its result is not being used. Use 'ignore' to discard the result of the expression, or 'let' to bind the result to a name.

and in case the symbol is mutable:

The operator '=' is used for comparison but its result is not being used. If you intend to mutate the variable, use '<-' operator instead, to discard the result of the expression use 'ignore' to discard it, or 'let' to bind it to a name.

Do you think it makes it any better? Do you prefer staying with current behaviour?

@isaacabraham
Contributor
isaacabraham commented Apr 25, 2016 edited

@KevinRansom some good points... but: -

  1. The problem is, newcomers to F# (particularly from curly brace languages) don't have a clue what ignore is. They might not even grok the difference between statements and expressions, let alone why that happens. I can promise you that the example I raised in the original issue is genuine - I've seen it happen on several occasions and every time the developer blamed the language rather than themselves because it was completely unclear what was happening. Suggesting to use ignore is dangerous because developers use that, see the warning disappear, and just carry on.
  2. Regarding the "it won't catch the last occurrence in a function - true. But that's the same now. If we can catch 99% of them, I'd consider that a win. And - if there's more than one in a function, the developer will understand the problem and almost certainly fix all of them, and not leave the last one alone.
  3. Re: Mutable error following on from this. Yep - that's fine. Again, that's probably something that I would expect and would only happen the first time. If we get nice tooling in Roslyn to add a "make value mutable" then it's not a problem ;-) And if not - then that fits with your next point regarding "we don't want to encourage mutation".
  4. I really don't like the suggestion of "let's not give a clear error message because we don't want to encourage mutation" - it's surely not the job of compiler warnings / error messages to tell developers how to write software. F# already discourages mutation by forcing you to use the mutable keyword.
  5. Long message - true. But it's important to explain this clearly and simply. If that means more text to explain something without ambiguity then it's worth it IMHO.
@KevinRansom
Contributor

@smoothdeveloper I think two different error messages depending on whether x is mutable or not is a fine way to go, and a great suggestion

However, this is perfectly valid and indicates the same cognitive error and yet will never yield an error:

let mutable x = 1
let mutable y = "Hello"

let changeY () =
    x = 1 |> ignore
    y = "test"

Even though their intention was to type the perfectly valid and infact intended function:

let mutable x = 1
let mutable y = "Hello"

let changeY () =
    x = 1 |> ignore
    y <- "test"

I would be worried about solving the cognitive error in such an opportunistic and unprincipled way.

I think this type of analysis is better handled inside the IDE, Roslyn Analyzers are perfect for this, we can even add the necessary code to inject the fixed. As well as present alternative fixes.

The work that @otawfik-ms is currently doing by integrating the F# language service with the Roslyn IDE services will place us in a better position to improve these experiences. Already Roslyn makes great use of Analyzers to fix these kinds of confusions.

@smoothdeveloper
Contributor

@KevinRansom thanks for feedback, I see where you are at with the case of return value.

Thinking about it, if the code is never called, it is fine to not have a warning.

If the call is called, while the author expected it to return unit it will now return bool so another warning will pop up and force user to reason about that.

I would be worried about solving the cognitive error in such an opportunistic and unprincipled way.

I think experienced F# users can figure out the warning about ignored expression even without looking at the compiler messages (just seeing the squiggles), if we can catch other cases beside the return value and provide helpful messages for new users I think it is worth doing.

Thanks for reminding us about ongoing work on roslyn support for analyzers and language service, it will be a great place to contribute for the community, and it is reasonable to ponder on the urgency to tweak many messages.

I think the overall experience with F# error / warnings is fine for usual code, although making it smoother for newcomers is I think a big win.

BTW, VB.NET users too are ingrained with using = for assignment.

@forki
Contributor
forki commented Apr 26, 2016 edited
  1. Regarding Roslyn: yes Roslyn analyzer will bring us new ways to do stuff. Once the thing is in place we can discuss where to emit specific warnings. That is a bright future. But real Roslyn support in F# is totally unclear for me right now. You know how long it took for C#/VB from first prototypes to the final product. But we can improve things right now. And we should. So the current discussion should be: How can we make the error message as human friendly as possible? How can we help newcomers (we need them!) to overcome the common issues? If a specific warning is not good for compiler, then does it make sense to create a FsLint issue?
  2. We shoud not advice to use mutation. I will try to refomulate that in the proposed warning.
  3. Regarding the case where the = is used in the last line. That's clearly a case where we don't want to emit a warning. And we don't do that. But it's a case that is less likely to come up for newcomers. So that is not an issue for me.
  4. We already have the following
<data name="UnitTypeExpected2" xml:space="preserve">
  <value>This expression should have type 'unit', but has type '{0}'. If assigning to a property use the syntax 'obj.Prop &lt;- expr'.</value>
</data>

This is basically the same argument. You want to give that additional info, but you don't want to issue that on the last line of a function. This is perfectly OK.
Interestingly I didn't find a code sample which would emit that warning yet. I think there is a bug in the [__] part of https://github.com/Microsoft/visualfsharp/blob/f51497919ae826ebac0cf2cdac7864c19244def1/src/fsharp/TypeChecker.fs#L694 I will fix that as well.

@7sharp9
7sharp9 commented Apr 26, 2016 edited

I would prefer the type checker to return sane error messages to the user, so that they can be consumed easily by any editor, not just a roslyneque one.

@granicz
granicz commented Apr 26, 2016

I think we are at a point where we expect the compiler to be smarter than drawing conclusions from a single expression. It should, as much as it can, guide developers based on their inferred intentions, and also offer fixes that are "most likely" in line with what the user intended.

So all in all, next to local heuristics like "the return value is not ignored", we should move towards heuristics like "x is not mutable, so the user meant equality and not assignment" or even better inferring that a function called changeX implies mutation, and thus drive error/warning generation accordingly.

I tend to agree that this sort of guiding is best handled in code analyzers and not in the compiler. The compiler needs to be super fast, and warnings/errors can be paraphrased through another assist layer (such as the IDE, etc.) that can sacrifice speed for more helpful guidance. Have a look at compiler errors for humans in Elm for a similar topic.

@dsyme
Contributor
dsyme commented Apr 26, 2016

A couple of comments about wording of error messages:

  • We generally give extra suggestive information through the use of the word "Consider", e.g. Here is an error. Consider doing XYZ
  • The use of the word "comparison" in the suggested doesn't ring true for me. In F# terminology "comparison" means <, >, max, min, compare etc. and in attributes like NoComparison, StructuralComparison etc.
@7sharp9
7sharp9 commented Apr 26, 2016

Cant these roslyn analysers be accomplished with Typed expressions TAST?

@dsyme
Contributor
dsyme commented Apr 26, 2016

@7sharp9 Yes, but only at very considerable extra memory pressure on the VFPT tooling (save TASTs for projects). But more importantly, beginners are not going to install any extra lint tooling, and perhaps not even VFPT.

We've long known that some core error messages needed to be improved. At the end of F# 2.0 Luke Hoban said something along the lines "The most important thing we could do for F# is to take the 20 most common error messages and work on them relentlessly until they are 10x better". We just never did it. So I think it's totally reasonable to take a pass to try to improve them, and in a sense this initiative is kicking off exactly what we should have done 6 years ago.

@7sharp9
7sharp9 commented Apr 26, 2016

@dsyme Yeah I was thinking at the project level / stand alone tools, Go has a lot of small standalone tools that are really useful, thats what Im currently poking about with :-)

@dsyme
Contributor
dsyme commented Apr 26, 2016 edited

Here are my suggestions for the cases where the expression is "expr1 = expr2":

If the LHS can be interpreted as a mutable l-value (.e.g determine this via an adjusted version of the logic in mkExprAddrOfExpr):

The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'. If you intend to mutate a value, then use the '&lt;-' operator e.g. 'x &lt;- expression'.

If the LHS is not a mutable l-value and is a simple value x:

The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'. If you intend to mutate a value, then mark the value  'mutable' and use the '&lt;-' operator e.g. 'x &lt;- expression'.

If the LHS is not a mutable l-value and is some more complex expression:

The result of this equality expression is implicitly discarded. Consider using 'let' to bind the result to a name, e.g. 'let result = expression'.

I think in all cases where the source expression is expression = expression then you can drop all mention of ignore in the error message. There is just no real chance that anyone writing x = y actually meant to write (x = y) |> ignore. If they really did intend to write that then they are an advanced F# user who already knows about "ignore"

@dsyme dsyme referenced this pull request Apr 28, 2016
Merged

split TypeRelations.fs #1131

@dsyme dsyme changed the title from WIP: Emit special warning when = is used and user might want to use <- to [WIP] Emit special warning when = is used and user might want to use <- Apr 28, 2016
@forki
Contributor
forki commented May 25, 2016

@dsyme we "just" need to make that TAST matching pretty. any hints?

@fsoikin fsoikin commented on an outdated diff Aug 28, 2016
...Source/Warnings/WarnIfPossibleDotNetPropertySetter.fs
@@ -0,0 +1,13 @@
+// #Warnings
+//<Expects status="Warning" span="(10,5)" id="FS0020">If you intended to set a value to a property, then use the '<-' operator e.g. 'x <- expression'</Expects>
@fsoikin
fsoikin Aug 28, 2016

Shouldn't this say e.g. Obj.Prop <- expression instead of e.g. x <- expression?

@KevinRansom
Contributor

I'm going to close this. because nothing has changed since the beginning of august, also I am reluctant to urge developers towards mutable in this corner scenario. Where the real solution is ignore.

@KevinRansom KevinRansom closed this Nov 8, 2016
@isaacabraham
Contributor

How can that be the real solution if they wanted to do a mutation?

@isaacabraham
Contributor

Please re open this. There's clearly discussion on here and I've seen first hand where people have fallen foul of this - It's a real issue.

@dsyme
Contributor
dsyme commented Nov 8, 2016

@KevinRansom I do agree that the user always intends to use mutation or property setting in these situations. It's a good thing to address. I'll reopen based on the positive community feedback.

@dsyme dsyme reopened this Nov 8, 2016
@forki forki changed the title from [WIP] Emit special warning when = is used and user might want to use <- to Emit special warning when = is used and user might want to use <- Nov 23, 2016
src/fsharp/TypeChecker.fs
+ else
+ warning (UnitTypeExpectedWithEquality (denv,ty,m))
+ | Expr.Op(_,_,Expr.Val(vf,_,_) :: _,_) :: _ ->
+ let isProperty = true // TODO: ask @dsyme about better way
@dsyme
dsyme Nov 25, 2016 Contributor

Looking in the MemberInfo of the F# value should do the trick, c.f. this in Symbols.fs. You might want to add a new IsPropertyGetterMethod to Val and ValRef in TastOps.fs/fsi and make sure all similar chunks of code are shared.

        | V v -> 
            match v.MemberInfo with 
            | None -> false 
            | Some memInfo -> memInfo.MemberFlags.MemberKind = MemberKind.PropertyGet
@dsyme
Contributor
dsyme commented Nov 25, 2016

@forki Needs another rebase

@forki
Contributor
forki commented Nov 26, 2016

done.

@forki
Contributor
forki commented Nov 26, 2016

this is now much more robust and only suggest to use <- for properties when they actually have a setter.
Also I fixed the message to show bindingName.PropertyName like:

image

src/fsharp/tast.fs
@@ -3100,6 +3100,18 @@ and
/// - If this is an implementation of an abstract slot then this is the name of the property implemented by the abstract slot
member x.PropertyName = x.Deref.PropertyName
+ /// Indicates whether this value represents a property getter.
+ member x.IsPropertyGetter =
@dsyme
dsyme Nov 26, 2016 Contributor

Please call this IsPropertyGetterMethod and IsPropertySetterMethod and adjust the identical code in Symbols.fs to use these.

@dsyme
dsyme Nov 26, 2016 Contributor

Also briefly search the codebase for any other identical code

@forki
forki Nov 26, 2016 Contributor

done.

@dsyme
Contributor
dsyme commented Nov 26, 2016

One comment, otherwise all looks good

src/fsharp/TypeChecker.fs
-
- if hasCorrespondingSetter then
- warning (UnitTypeExpectedWithPossiblePropertySetter (denv,ty,vf.DisplayName,propertyName,m))
+ let checkExpr exprOpt =
@dsyme
dsyme Nov 26, 2016 Contributor

Could you factor this out into a separate function that makes it obvious it is about error reporting please? Thanks

@forki
forki Nov 26, 2016 Contributor

sure

@forki
forki Nov 26, 2016 Contributor

done

@forki
Contributor
forki commented Nov 26, 2016

of course now I have to fix all these wrong test assumptions as well. sigh

@forki
Contributor
forki commented Nov 27, 2016

fixed. It now correctly detects "in" expressions

@forki forki Detect "in" expressions
60fe7ec
@forki
Contributor
forki commented Dec 1, 2016

old

@isaacabraham
Contributor

Whoooooo

@dsyme dsyme merged commit 0f841b8 into Microsoft:master Dec 3, 2016

6 checks passed

Ubuntu14.04 Release Build Build finished.
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
Windows_NT Release_net40_no_vs Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki forki deleted the forki:assignment-warning branch Dec 4, 2016
@brettfo brettfo added a commit to brettfo/visualfsharp that referenced this pull request Jan 18, 2017
@brettfo brettfo Merged PR 49572: Merge insert-master-into-microbuild to microbuild
 - Improve the tutorial (#1904)
 - Fix 1898 (#1900)
 - Refresh the check of files when notified by F# analysis engine (#1906)
 - Emit special warning when = is used and user might want to use <- (#1115)
 - always allow breakpoints to be set on same line if possible (#1918)
 - use shared MEF components instead of static fields (#1912)
 - Don't compute ranges and stuff when we don't need that (#1927)
 - fix autocomplete after integer literals (#1930)
 - Enable zooming in F# Interactive window (#1914)
 - Fix "ci_part1" tests on master (#1931)
...
ccc8037
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment