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

Improve Error Reporting: Forgetting | in anonymous record gives a confusing error message. #8127

Open
Tracked by #1103
isaacabraham opened this issue Jan 8, 2020 · 3 comments
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Request
Milestone

Comments

@isaacabraham
Copy link
Contributor

What

Whilst it's an expected pattern to move from anonymous records to nominal records, we've noticed that we sometimes find it useful to go the other direction - from a full nominal record to an anonymous record with a type alias.

Unfortunately, doing this leads to an error message that is especially misleading - and leads to lots of confusion (I'm talking sometimes 5 minutes of chasing our tails until we realise what the problem is).

This code compiles fine:

type Person = { Name : string }
let x : Person = { Name = "Test" }

Now assume we change Person to an type-aliased anonymous record:

type Person = {| Name : string |} // just add two |s in here..
let x : Person = { Name = "Test" } // Error: The record label `Name` is not defined.

Why

Whilst it's totally reasonable to give a compiler error at this point, the error is misleading - it suggests that the Name field doesn't exist: The first thing a developer does at this point is look at the type alias and say "Yes, the Name does exist! There's a problem with the compiler!".

The developer might completely overlook the fact that they are missing the | from the RHS of the second line. Also consider that this second line might exist in another file, or even project, miles away from the alias definition.

How

Detect if you're comparing or assigning a nominal record to an aliased record (and, ideally, compare all the field names). Then give an error message such as:

You are attempting to assign a record to an anonymous record. Did you forget to add vertical pipes to the record definition e.g. {| Field = value... |}?
@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jan 8, 2020

(and, ideally, compare all the field names)

I assume you mean comparing when the expected and given type is non-anonymous? Cause comparing field names for anonymous records is currently underway here: #8094. The related issue is #8091 (edit: I'm sure you're aware, it's your PR ;).

Totally agree to the suggested improvements. I'd like to add that the inverse is also not as clear as it could be. Perhaps we could improve the general case by mentioning that the expected type is a record and the given type an anonymous record, and vv:

image

One suggestion for improvement here could be:

The expression was expected to have record type X but here has anonymous record type Y. Record type X with the given fields exists, change '{|...|}' into '{...}' to turn the expression in record type X.

@isaacabraham
Copy link
Contributor Author

As this is more than just error text, I'm wondering how to start with this one. @cartermp @forki do you think this is possible without a huge amount of effort?

@smoothdeveloper
Copy link
Contributor

@isaacabraham, I haven't dealt with this area, but it looks like dealing with NameResolutionEnv to search for potential matches in anonymous records that are reachable, I assume the code is there or very close nearby:

let ResolveFieldPrim sink (ncenv: NameResolver) nenv ad ty (mp, id: Ident) allFields =
let typeNameResInfo = TypeNameResolutionInfo.Default
let g = ncenv.g
let m = id.idRange
match mp with
| [] ->
let lookup() =
let frefs =
try Map.find id.idText nenv.eFieldLabels
with :? KeyNotFoundException ->
// record label is unknown -> suggest related labels and give a hint to the user
error(SuggestLabelsOfRelatedRecords g nenv id allFields)

the issue is that I don't know if there is tracking of anonymous record types fields the way it is stored in eFieldLabels for normal records, but if ty of that function is the type of the anonymous record, you could check there and branch to a better message.

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 Feature Request
Projects
Status: New
Development

No branches or pull requests

5 participants