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

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

Merged
merged 8 commits into from Dec 3, 2016

Conversation

@forki
Copy link
Contributor

forki commented Apr 25, 2016

This implements #1109

Before:

image

After:

image

@forki forki changed the title Emit special warning when = is used and user might want to use <- WIP: Emit special warning when = is used and user might want to use <- Apr 25, 2016
@forki forki force-pushed the forki:assignment-warning branch 2 times, most recently from 0e694dd to 3d10dda Apr 25, 2016
@KevinRansom
Copy link
Member

KevinRansom commented Apr 25, 2016

@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
Copy link
Contributor Author

forki commented Apr 25, 2016

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
Copy link
Contributor

smoothdeveloper commented Apr 25, 2016

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
Copy link
Member

KevinRansom commented Apr 25, 2016

@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
Copy link
Contributor

smoothdeveloper commented Apr 25, 2016

@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
Copy link
Contributor

isaacabraham commented Apr 25, 2016

@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
Copy link
Member

KevinRansom commented Apr 25, 2016

@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
Copy link
Contributor

smoothdeveloper commented Apr 26, 2016

@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 forki force-pushed the forki:assignment-warning branch from 3d10dda to f76912b Apr 26, 2016
@forki
Copy link
Contributor Author

forki commented Apr 26, 2016

  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.

@forki forki force-pushed the forki:assignment-warning branch from f76912b to 59d0ad5 Apr 26, 2016
@7sharp9
Copy link
Contributor

7sharp9 commented Apr 26, 2016

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
Copy link

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
Copy link
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
Copy link
Contributor

7sharp9 commented Apr 26, 2016

Cant these roslyn analysers be accomplished with Typed expressions TAST?

@dsyme
Copy link
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
Copy link
Contributor

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
Copy link
Contributor

dsyme commented Apr 26, 2016

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"

@forki forki force-pushed the forki:assignment-warning branch 3 times, most recently from 2eca844 to a7ce0b9 Apr 27, 2016
@dsyme dsyme mentioned this pull request Apr 28, 2016
@dsyme dsyme changed the title WIP: Emit special warning when = is used and user might want to use <- [WIP] Emit special warning when = is used and user might want to use <- Apr 28, 2016
@forki forki force-pushed the forki:assignment-warning branch 2 times, most recently from c81ff9f to 87be924 Apr 29, 2016
@forki
Copy link
Contributor Author

forki commented Nov 23, 2016

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

This comment has been minimized.

Copy link
@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
Copy link
Contributor

dsyme commented Nov 25, 2016

@forki Needs another rebase

@forki forki force-pushed the forki:assignment-warning branch 2 times, most recently from 274fbe7 to 9d2fd51 Nov 26, 2016
@forki
Copy link
Contributor Author

forki commented Nov 26, 2016

done.

@forki
Copy link
Contributor Author

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

@@ -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 =

This comment has been minimized.

Copy link
@dsyme

dsyme Nov 26, 2016

Contributor

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

This comment has been minimized.

Copy link
@dsyme

dsyme Nov 26, 2016

Contributor

Also briefly search the codebase for any other identical code

This comment has been minimized.

Copy link
@forki

forki Nov 26, 2016

Author Contributor

done.

@dsyme
Copy link
Contributor

dsyme commented Nov 26, 2016

One comment, otherwise all looks good


if hasCorrespondingSetter then
warning (UnitTypeExpectedWithPossiblePropertySetter (denv,ty,vf.DisplayName,propertyName,m))
let checkExpr exprOpt =

This comment has been minimized.

Copy link
@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

This comment has been minimized.

Copy link
@forki

forki Nov 26, 2016

Author Contributor

sure

This comment has been minimized.

Copy link
@forki

forki Nov 26, 2016

Author Contributor

done

@forki
Copy link
Contributor Author

forki commented Nov 26, 2016

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

@forki forki force-pushed the forki:assignment-warning branch from f6ad0d3 to d275324 Nov 26, 2016
@forki forki force-pushed the forki:assignment-warning branch from d275324 to 1b2b744 Nov 27, 2016
@forki
Copy link
Contributor Author

forki commented Nov 27, 2016

fixed. It now correctly detects "in" expressions

@forki forki force-pushed the forki:assignment-warning branch from 1b2b744 to 60fe7ec Nov 27, 2016
@forki
Copy link
Contributor Author

forki commented Dec 1, 2016

old

@isaacabraham
Copy link
Contributor

isaacabraham commented Dec 1, 2016

Whoooooo

@dsyme dsyme merged commit 0f841b8 into dotnet:master Dec 3, 2016
6 checks passed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.