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

Elm-like compiler errors for name resolution #1102

Merged
merged 1 commit into from
May 26, 2016
Merged

Conversation

forki
Copy link
Contributor

@forki forki commented Apr 24, 2016

Introduction

This PR is a starting point for making the F# compiler emit error messages that are more user friendly. Newcomers to Elm report that the excellent compiler errors [read http://elm-lang.org/blog/compiler-errors-for-humans] help so much to get started quickly. We should do our best to improve our error messages as well.

I think this can be done as community effort. This PR will start and bring some infrastructure in place. After that we can create a list with possible optimizatons and divide the work. This approach worked very well when we "regularized" the collections.

Sample: Propose compatible record labels when compiler can't find label and doesn't know record type

Let's consider a small sample to see what we can improve:

Microsoft (R) F# Interactive version 14.0.23413.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> type A = { Hello:string; World:string }
type B = { Size:int; Height:int }
type C = { Wheels:int }
type D = { Size:int; Height:int; Walls:int }
type E = { Unknown:string }
type F = { Wallis:int; Size:int; Height:int; }

let r = { Size=3; Height=4; Wall=1 };;

  let r = { Size=3; Height=4; Wall=1 };;
  ----------------------------^^^^

stdin(8,29): error FS0039: The record label 'Wall' is not defined

So the compiler can't find the record label Wall and complains about that. After applying the PR we will get the following:

image

As you can see the compiler proposes record labels that might work here. In this case it looks for all records that are compatible with what you already have (Size, Height labels). It then computes the edit distance (Damerau Levenshtein) to the labels of these records and proposes the top 5.

Sample: Propose compatible record labels when compiler can't find label in known record type

image

Here we only predict labels that are in same record type, ordered by edit distance.

Sample: Propose types inside of module / namespace

image

Changes in this Pull Request

  • Add Edit Distance function
  • Propose compatible record labels when compiler can't find label and doesn't know record type
  • Propose compatible record labels when compiler can't find label and knows record type
  • Propose types inside of module / namespace

@forki forki force-pushed the error1 branch 7 times, most recently from 10d6ba6 to fdcd823 Compare April 24, 2016 13:57
nenv.eFieldLabels
|> Seq.filter (fun kv ->
kv.Value
|> List.map (fun r -> r.TyconRef.ToString()) // TODO: Ask Don how to compare type names here
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @dsyme I think we want to compare full qualified names of the types. How can I do that?!

@@ -0,0 +1,13 @@
// #ErrorMessages #NameResolution
//<Expects status="error" span="(11,31-11,34)" id="FS1129">The record type 'F' does not contain a label 'Wall'\.</Expects>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KevinRansom @enricosada @dsyme I want to test for the multi line error message here. How can I do that?

@forki forki force-pushed the error1 branch 3 times, most recently from 8f66301 to a83883b Compare April 24, 2016 16:05
@@ -0,0 +1,13 @@
// #ErrorMessages #NameResolution
//<Expects status="error" span="(11,29)" id="FS0039">The record label 'Wall' is not defined\.</Expects>
Copy link
Contributor

@enricosada enricosada Apr 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@forki about multiline, just use more Expect, like there

@TIHan
Copy link
Contributor

TIHan commented Apr 25, 2016

This is awesome. :D

@forki forki force-pushed the error1 branch 4 times, most recently from 5792c92 to 155be99 Compare April 25, 2016 11:15
@forki
Copy link
Contributor Author

forki commented Apr 25, 2016

I think something seriously bad happened to nuget. This doesn't even start to build anymore because restore failed. See http://dotnet-ci.cloudapp.net/job/Microsoft_visualfsharp/job/master/job/debug_windows_nt_prtest/261/console

/cc @KevinRansom @maartenba

@maartenba
Copy link

maartenba commented Apr 25, 2016

@forki I may be missing it in the build log but where does it go wrong? If it's that first restore, it seems it's somehow not hitting the correct feed(s)

@forki
Copy link
Contributor Author

forki commented Apr 25, 2016

04:16:05 The path is not of a legal form.
04:16:05 Error: dotnet publish failed

This all worked yesterday. I only pushed a change that is related to unittests that come much later.

@maartenba
Copy link

Possible change on the build machine? (the error is kinda vague :))

@forki
Copy link
Contributor Author

forki commented Apr 25, 2016

The same happens on appveyor. Both (unrelated) build servers report this issue

@maartenba
Copy link

Definitely no server-side changes on NuGet since yesterday. Client side no idea. @KevinRansom

@forki
Copy link
Contributor Author

forki commented Apr 25, 2016

In 98a329a I commited older version of dotnet cli. This seems to work.

dead

/cc @KevinRansom @enricosada is there a way to make it download a pinned version of dotnet cli? and who do we have to ask to fix it!?

@forki
Copy link
Contributor Author

forki commented Apr 26, 2016

Implemented first round of feedback.

@forki forki force-pushed the error1 branch 8 times, most recently from 1b02724 to 9051054 Compare April 27, 2016 12:37
@forki forki changed the title WIP: Elm-like compiler errors for name resolution Elm-like compiler errors for name resolution Apr 29, 2016
@forki forki force-pushed the error1 branch 3 times, most recently from dd0bb0c to 32c44ed Compare May 13, 2016 05:54
@forki
Copy link
Contributor Author

forki commented May 13, 2016

I think this is ready and merging this would help me to work on similar issues.
(appvevor seems to be broken, but unrelated)

@forki
Copy link
Contributor Author

forki commented May 17, 2016

WTF!? appveyor reporting something unrelated:

Errors and Failures

1) Failed : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration
  Expected: True
  But was:  False
at <StartupCode$FSharp-Core-Unittests>.$AsyncModule.test@280-22(Unit unitVar0) in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line 293
at FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.OnCancel.RaceBetweenCancellationHandlerAndDisposingHandlerRegistration() in C:\projects\visualfsharp-3dtit\src\fsharp\FSharp.Core.Unittests\FSharp.Core\Microsoft.FSharp.Control\AsyncModule.fs:line 294

@forki
Copy link
Contributor Author

forki commented May 18, 2016

is there anything I can do to get this in? I would need that infrastructure for other similar cases

@forki
Copy link
Contributor Author

forki commented May 19, 2016

that test was flaky. it's green now

@dsyme
Copy link
Contributor

dsyme commented May 19, 2016

I'm OK with this change (up to the caveats I mentioned up above about also eventually making the information available through the compiler service)

@KevinRansom KevinRansom merged commit f47a6bb into dotnet:master May 26, 2016
@forki forki deleted the error1 branch May 30, 2016 15:58
@isaacabraham
Copy link
Contributor

Something I've just noticed about this - if you create a record via let x = { RecordType.FieldName = "blah" } syntax, the new and improved suggestions don't appear (compared to let x : RecordType = { FieldName = "blah" }

@isaacabraham
Copy link
Contributor

Also the "suggest types" one (e.g. Lst instead of List) doesn't work on the RHS of the equals e.g. let x = System.Collections.Generic.Lst()

@forki
Copy link
Contributor Author

forki commented Aug 15, 2016

please create separate issues with repro and set me cc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants