-
Notifications
You must be signed in to change notification settings - Fork 834
[wip] error message suggesting Type Provider static parameters names #11451
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
[wip] error message suggesting Type Provider static parameters names #11451
Conversation
|
Unfortunately the code appears to make this less easy, but some things to consider:
I don't think this would have an impact on editor perf at all, but the main consideration on my end is bloating the error tooltip and not having an automatic suggestion. |
|
would you mind pointing at the fixer code and explain how it interacts with I tried leveraging the current infrastructure (I introduce a method that will be shared already) but it would need further consideration. The order of files with CompilerDiagnostic.fs (I think I missed to push a change in this one) will make it necessary to define the infrastructure earlier (ErrorLogger would be the place AFAICS). I overall understand the concern of encapsulating the data I'm assembling in a format that the editor tooling can leverage later on.
For the bloating, this is up to the number of total parameters, should not be a big concern AFAIK, and I can also just use the existing suggestion / name distance component, but it is a bit rigid in it's design right now. The suggestions can also be on a single line, it is not like we are showing overloads signatures, but just a list of identifiers. IMO The current state of the code already improves the situation: the completion of parameters tends to be janky in type providers, and in FSI session, you really don't have much hand to try to correct the code without the suggestions. I'm happy to put some efforts as I experiment to make it easier for code fixers to pick up the suggestions, especially if you show me the way! |
|
This is the entry-point that code fixers can call: https://github.com/dotnet/fsharp/blob/main/src/fsharp/service/ServiceCompilerDiagnostics.fs#L20 Some code has changed between when this was introduced and now, but following the navigation chain should give a sense of what happens. It's called here: https://github.com/dotnet/fsharp/blob/main/vsintegration/src/FSharp.Editor/CodeFix/ReplaceWithSuggestion.fs In this case, there may need to either be a new quick fix or error code added to the existing one. |
|
@cartermp, the main issue to bring the right suggestions is that For type provider declaration, the list should be restricted to literals and the static parameter names. |
|
@cartermp, would it be agreeable to review/merge this PR, if I add testing of the error message, and adjust the error message to list all arguments on a single line, comma separated? I don't think we should wait for improvement surfacing in code fixers, the main issue is when tooling is broken (like it is when you type the static argument literals), the compiler helps the user. If later on, we can improve the tooling (which may requires work in parser recovery logic giving the context to tooling) and code fixes (which right now, in this repository are proprietary to VS), this is very good, but this broadens the scope too much for a simple "quality of life" improvement; IMO. More of those improvements being merged, means more opportunities later on for refining the tooling, the parser error recovery logic and how the signatures are prompted in the FSharp.Editor is a bit outside of my reach for small contributions. |
|
Thanks for providing this, do you still want to work on it, it looks like it's been more than two years. Please reopen if you can spare the time to progress this. Thanks Kevin |
the first changes for this issue #11445