-
Notifications
You must be signed in to change notification settings - Fork 803
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
Name resolution: resolve interfaces in expressions #15660
Name resolution: resolve interfaces in expressions #15660
Conversation
Is this still an experiment, or ready for review? I have a feeling that the typecheck error reported now is worse from understandability points of view => I would need convincing that the tradeoff is worth it. |
@T-Gro This is currently still an experiment. I'm currently on vacation and going to get back to it afterwards. The general idea here is to provide symbols (with the type arguments where possible) and have better errors for unfinished expressions like: // module/class/record/union
List
List.
// interface
IList
IList<_>
IList<int> Currently no symbols are provided and the errors are misleading. This experiment already improves things for some of these cases, but needs further work. The change in the tests is mostly about quite a corner case related to incorrect type definitions, and the previous error about constructors is also misleading. I'm going to look into improving it; maybe I'll just try to reuse this error. |
@T-Gro I think they're actually better. The most of the updated baselines capture cases where there's a type that can't be instantiated properly due to an issue in its declaration, and previous errors would say that the types are undefined (which is not true and is misleading) while the newer errors say that the types are used in a wrong way, which is probably good enough considering that a fix is needed on the declaration side, and there's an appropriate error reported for the declaration. |
33e4b4d
to
51f31c8
Compare
This is ready. It changes the way how wrong usages of interfaces are encoded in name resolution items, but it looks good and makes interface items more inline with rest of the types by removing the special case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm I guess the resulting message could be clearer but IMO the new way it is not worse than what used to be. Okay with me.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
3d596c8
to
b96fa07
Compare
This is green now. |
* upstream/main: (166 commits) typo in foldBack summary (dotnet#16453) Fix for dotnet#83 (improve constraint error message) (dotnet#16304) Name resolution: resolve interfaces in expressions (dotnet#15660) AddExplicitReturnType refactoring (dotnet#16077) Disabling 2 tests: running for too long, causing CI timeouts Improve value restriction error message dotnet#1103 (dotnet#15877) Parens: Keep parens for non-identical infix operator pairs with same precedence (dotnet#16372) More release note entries (dotnet#16438) Using Ordinal is both faster and more correct as our intent is to do … (dotnet#16439) merge (dotnet#16427) Optimize empty string compares (dotnet#16435) Checker: recover on unresolved type in 'inherit' member (dotnet#16429) Release notes proposal (dotnet#16377) [main] Update dependencies from dotnet/source-build-reference-packages (dotnet#16411) Allow usage of [<TailCall>] with older FSharp.Core package versions (dotnet#16373) Parser: recover on unfinished 'as' patterns (dotnet#16404) Parens: Keep parens in method calls in dot-lambdas (dotnet#16395) Checker: check unfinished obj expression inside computations (dotnet#16413) Added default dotnet-tools + additional tasks to launch them (dotnet#16409) make `remarks` and `returns` visible in quick info (dotnet#16417) ...
Here's an experiment to see what breaks in CI.