Skip to content

Conversation

@kurtschelfthout
Copy link
Contributor

@kurtschelfthout kurtschelfthout commented Nov 17, 2016

Take 2. Rebased on master after all the recent changes.

RFC.

@kurtschelfthout
Copy link
Contributor Author

Adding a comment to perhaps kick off a build. This was all green before.

@kurtschelfthout kurtschelfthout changed the title Implement RFC fs-1025: Improve record inference. Implement RFC fs-1025: Improve record type inference Nov 21, 2016
@KevinRansom
Copy link
Contributor

@dotnet-bot test this please

@forki
Copy link
Contributor

forki commented Nov 22, 2016

cool! side question: do we need to adjust error messages!?

@kurtschelfthout
Copy link
Contributor Author

@forki: I don't think so, but I haven't reviewed the error messages.

The nature of the change is that an ensuing check will now produce an error in fewer cases, because the type it picks during record type inference initially is a better choice. But the ensuing checks are still valid so don't think the error messages there need to be adjusted as a result of this change.

There is a warning produced just above the changed code that is also still valid, for cases like { a with F=3 } where we can't use the "pick the record type with same number of defined fields" trick because not all fields may be specified in the code for record updates. So that warning also remains valid.

I've noticed you added some code to suggest other record types when subsequent errors occur; perhaps you could argue that these may be updated to take the defined field count into account, but that might also be more confusing (i.e. excluding types that the programmer actually wanted to see).

It's entirely possible I've missed something though!

@forki
Copy link
Contributor

forki commented Nov 22, 2016

ok let's just assume things will work. And if we see samples where the error message could be better then we will fix them

@KevinRansom KevinRansom merged commit 2f5851a into dotnet:master Nov 25, 2016
@KevinRansom
Copy link
Contributor

@kurtschelfthout
Thank you for this contribution.

Kevin

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.

5 participants