Better expression errors

[isaacabraham]

Having done #5007 I have now applied changes to four other similar error messages, by changing the message from "this expression" to "the first item" etc., which better explains why the compiler has inferred the expected type.

The error messages are:

List element has wrong type

let x = [ 1; 2; "a" ]

Before: All elements of a list constructor expression must have the same type. This expression was expected to have type 'int', but here has type 'string'.
After: All elements of a list must be of the same type as the first element, which here is 'int'. This element has type 'string'.

Array element has wrong type

let x = [| 1; 2; "a" |]

Before: All elements of an array constructor expression must have the same type. This expression was expected to have type 'int', but here has type 'string'.
After: All elements of an array must be of the same type as the first element, which here is 'int'. This element has type 'string'.

Missing Else Branch

if 1 = 1 then 999

Before: The 'if' expression is missing an 'else' branch. The 'then' branch has type 'int'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type.
After: This 'if' expression is missing an 'else' branch. Because 'if' is an expression, and not a statement, add an 'else' branch which also returns a value of type 'int'.

Else Branch Has Wrong Type

if 1 = 1 then 999 else "blah"

Before: All branches of an 'if' expression must have the same type. This expression was expected to have type 'int', but here has type 'string'.
After: All branches of an 'if' expression must return values of the same type as the first branch, which here is 'int'. This branch returns a value of type 'string'.

Following Pattern Match Clause Has Wrong Type

match 1 with | 1 -> 123 | _ -> "blah"

Before: All branches of a pattern match expression must return values of the same type. The first branch returned a value of type 'int', but this branch returned a value of type 'string'.
After: All branches of a pattern match expression must return values of the same type as the first branch, which here is 'int'. This branch returns a value of type 'string'.

[smoothdeveloper]

@cartermp ideally, this should be handled outside this repository with separate integration pipeline.

This repository would pull those files from the localization repository during the build and run the command to update any missing entry before compiling.

The localization repository would have an automated integration task pulling the fscomp changes in master from here and check them in the localization repository.

The case a contributor to Microsoft/visualfsharp is also going to handle the localization is very slim (less than 1%?) and we have to deal with increased pipeline / feedback loop anytime someone works on better errors (just dealing with the test suite is enough), this is a recurrent contribution pattern that we want to streamline.

[smoothdeveloper]

I'll add that the noise generated in code reviews and additional checkins increase potential errors and discourage focused reviews.

[forki]

you need to update neg20.bsl

[smoothdeveloper]

@isaacabraham after running the test, can you try using https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/update.base.line.with.actuals.fsx and check if it does what you expect? it should replace the .bsl file with the updated ouput based on the .err file.

https://github.com/Microsoft/visualfsharp/blob/master/TESTGUIDE.md#test-outcome-against-baseline For more information.

[abelbraaksma]

I welcome any improvement here, i often have trouble finding out what these errors mean. But 'as the first item' may not always be correct. I.e. if a type is inferred from calling arguments, or specified with type arguments or a type specification then there isn't a 'first' element. Or are that different errors?

[smoothdeveloper]

@isaacabraham do you mind updating the description in top comment to mention sample F# code which trigger those or some of the messages?

It is hard to figure out just by reading proposed error messages and comments how this will look like in actual use.

[forki]

@abelbraaksma can you give a simple code example of what you mean? we can add it as test

[isaacabraham]

@smoothdeveloper good point - done.

[isaacabraham]

@KevinRansom I think that the build is breaking from some unit tests - please don't fix them, I want to solve this one myself :-)

[isaacabraham]

@KevinRansom @cartermp @forki anyone know how to get the neg20.bsl and neg80.vsbsl files to update? These are still showing the old error messages and causing the unit tests to fail.

[forki]

run "build.cmd ci_part2" then it should runt the tests. after that rename neg20.err to neg20.bsl

[smoothdeveloper]

@isaacabraham thanks, so I like the changes, especially for the branching expressions.

I'm just wondering about using "return" which could be confusing (as return is a keyword and people coming from non expression languages might connect the message to it), what do you think of "evaluating" or "resulting" instead of "returning" when talking about the type of expression?

For example:

All branches of an 'if' expression must return values of the same type as the first branch, which here is 'int'. This branch returns a value of type 'string'.

could be phrased as

All branches of an 'if' expression must result in values of the same type as the first branch, which here is 'int'. This branch returns a value of type 'string'.

or

All branches of an 'if' expression must evaluate into values of the same type as the first branch, which here is 'int'. This branch returns a value of type 'string'.

The changes are good in anycase.

@abelbraaksma if you have a code sample to clarify which error message you are referring to, that would also help 🙂

[isaacabraham]

@smoothdeveloper I know what you mean - however I think that going down that route leads us back to the original sort of thing i.e. "the type of this expression is xxx" which beginners don't grok. I've not seen people struggle much with the notion that "the last line in a block is implicitly the return value", so I think that this is ok.

[abelbraaksma]

@abelbraaksma can you give a simple code example of what you mean? we can add it as test
@abelbraaksma if you have a code sample to clarify which error message you are referring to, that
would also help slightly_smiling_face

@smoothdeveloper, and @forki and et al, here are a few:

The following should not be changed by this PR (didn't check if it did, but should perhaps be added as a test if one doesn't already exist):

This one isn't very clear, but I can't think of a good way of clarifying it further:

The following is a version of the one I referred to, this error is generic to all types, not just lists, but it is often difficult to find out what is meant with "Expecting a ...". In particular, what is the object of that phrase? Who's expecting what? Is it the argument for g, is the return type of the function as a whole incorrect? Is f() giving me the wrong type? (in this case, it is not the argument for g, but the return type of g).

This last one has been reported on several occasions, let me see if I can find the relevant discussion.

(apologies if this is out of scope of this PR, but I thought it was related)

[abelbraaksma]

And here is a counter-example where it is not "the first branch" that determines what the result of the if-expression should be, so we ought to be careful about using that phrase everywhere:

[abelbraaksma]

This last one has been reported on several occasions, let me see if I can find the relevant discussion.

#3973 is perhaps related, albeit a bit wider scoped.

[isaacabraham]

@abelbraaksma that's a good example where the first item isn't matched - however, this is a different case because there's an explicit type hint in the function signature. In the cases dealt with in this PR, they're type inference is used, under which circumstances the first element of a list is used to specify the expected type of the list. The fact that there's a different message suggests a different situation to me (and we should probably look to reword that one, too).

[abelbraaksma]

that's a good example where the first item isn't matched - however, this is a different case because there's an explicit type hint in the function signature. I

Aha, I see you your point (@isaacabraham). So then, this would be an example where the second branch is used to decide what the return type is?

let foobar a b =
    match b with
    | "" -> a    // inferred to be string based on second case
    | "t" -> b   // determines inferred type
    | _ -> 12    // error here about not being string "All branches of a pattern match..."

and we should probably look to reword that one, too

Agreed, "context" in that error message is rather vague, I think.

[isaacabraham]

@forki i fixed that but build is now failing for some other reason - seems to be something about nuget. Any ideas?

[forki]

What does it say? Isaac Abraham <notifications@github.com> schrieb am So., 24. Juni 2018, 17:32:

…

@forki <https://github.com/forki> i fixed that but build is now failing for some other reason - seems to be something about nuget. Any ideas? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5114 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNJKr4VfmYLRmZdp6RGwE9rt1qvfdks5t_7ElgaJpZM4Ucbg-> .

[isaacabraham]

@forki NuGetPackage Issue. When i drill into the error, the build log shows: "WARNING: Unable to find version '3.5.0' of package 'NUnitLite'." and then a load of other nuget issues.

[forki]

Mhm no idea Sorry Isaac Abraham <notifications@github.com> schrieb am Di., 26. Juni 2018, 12:46:

…

@forki <https://github.com/forki> NuGetPackage Issue. When i drill into the error, the build log shows: "WARNING: Unable to find version '3.5.0' of package 'NUnitLite'." and then a load of other nuget issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5114 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNCHf98sn_m9Bif86r084vUy4_71uks5uAhETgaJpZM4Ucbg-> .

[isaacabraham]

Can we "retry" this build with that whole "dotnet pretty please rebuild" comment thing? cc @KevinRansom @cartermp or is this some other error?

[0x53A]

You can close/reopen the PR to trigger a rebuild

[isaacabraham]

Right. That random error has gone away. Now there's a test failure, because the message has changed:

1) Error : FSharp-Tests-Core+TypecheckTests.type check neg80
07:05:42 System.Exception : neg80.vserr neg80.vsbsl differ; "diff between [D:\j\w\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs\neg80.vsbsl] and [D:\j\w\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs\neg80.vserr]
07:05:42 line 6
07:05:42  - neg80.fsx(79,6,79,6): typecheck error FS0001: All branches of a pattern match expression must return values of the same type. The first branch returned a value of type 'string', but this branch returned a value of type 'unit'.
07:05:42  + neg80.fsx(79,6,79,6): typecheck error FS0001: All branches of a pattern match expression must return values of the same type as the first branch, which here is 'string'. This branch returns a value of type 'unit'.
07:05:42 "

What is this neg80.fsx file? How is it created? Where does it get run from? :-/

[KevinRansom]

Rerunning the test: @dotnet-bot test this please

[cartermp]

@isaacabraham Looks like a baseline file needs updating:

18:52:43 Errors and Failures
18:52:43 
18:52:43 1) Error : FSharp-Tests-Core+TypecheckTests.type check neg80
18:52:43 System.Exception : neg80.vserr neg80.vsbsl differ; "diff between [D:\j\w\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs\neg80.vsbsl] and [D:\j\w\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs\neg80.vserr]
18:52:43 line 6
18:52:43  - neg80.fsx(79,6,79,6): typecheck error FS0001: All branches of a pattern match expression must return values of the same type. The first branch returned a value of type 'string', but this branch returned a value of type 'unit'.
18:52:43  + neg80.fsx(79,6,79,6): typecheck error FS0001: All branches of a pattern match expression must return values of the same type as the first branch, which here is 'string'. This branch returns a value of type 'unit'.
18:52:43 "
18:52:43    at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1645.Invoke(String message) in D:\j\w\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core\printf.fs:line 1645
18:52:43    at SingleTest.singleNegTest(TestConfig cfg, String testname) in D:\j\w\release_ci_pa---866fd2c3\tests\fsharp\single-test.fs:line 237

[cartermp]

I like it, especially the consistency between the branching constructs. Assuming the baseline gets updated and tests pass, this would be great to take.

[isaacabraham]

Hallelujah. It only took 6 weeks!

[isaacabraham]

@cartermp @KevinRansom @forki thank you for helping me get this "green". I have to say, the process (whilst a LOT better than 18 months ago) is still kind of weird: tests that are "generated" based attributes with strings etc., tests that are stored in files with extensions I've never heard of (vbsl etc. etc.). Plus it takes a long, long time to run through the tests. Also - what documentation should I have looked at to understand all this?

Is there anything that can be done to help improve these issues?

[smoothdeveloper]

@isaacabraham I've recently added a readme in https://github.com/Microsoft/visualfsharp/tree/master/tests/fsharpqa as well as a script. The testguide in the root was also updated to give more context on areas of pain.

To make it better for contributors, I think having the translation and build from source done in separate repository and handled sollely at packaging time would make it easier.

Right now every new (or sparse) contributor is bound to spend couple of hours to acquaint with the whole test system and some more time dealing with CI feedback loop that requires adjusting few files after each edit to the error messages and seeing the test failure, it helps a lot to be able to run the tests locally.

[cartermp]

Yeah, we've definitely normalized the insanity in tests all-up in this repo, but undoing it all would probably come at the cost of a new language version's worth of work. Ditto on IDE unit tests, which arguably even crazier, but would involve a monumental effort to get all of the redundancies stripped out.

[KevinRansom]

Thanks for this. It gets easier ... not easy, but easier ...

[ShalokShalom]

I think such error messages are very important for translations.
I once looked into all the massages here at Github in order to translate them and quickly quit the plan, since it was such confusing.
Great commit, thanks @isaacabraham