Skip to content

Conversation

@smoothdeveloper
Copy link
Contributor

the first changes for this issue #11445

@cartermp
Copy link
Contributor

Unfortunately the code appears to make this less easy, but some things to consider:

  • Threading the suggestNames parameter through to only suggest on direct compile calls, not IDE diagnostics
  • Wiring up a code fixer for the error code which does suggest names and orders them by edit distance

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.

@smoothdeveloper
Copy link
Contributor Author

would you mind pointing at the fixer code and explain how it interacts with ErrorResolutionHints.SuggestionBuffer?

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.

but the main consideration on my end is bloating the error tooltip and not having an automatic suggestion.

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!

@cartermp
Copy link
Contributor

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.

@smoothdeveloper
Copy link
Contributor Author

@cartermp, the main issue to bring the right suggestions is that FSharpCheckFileResults.GetDeclarationListInfo seems to bring everything under the sun when the caret is in the type provider itself at the misspelled static parameter name.

For type provider declaration, the list should be restricted to literals and the static parameter names.

@smoothdeveloper smoothdeveloper changed the title error message suggesting Type Provider static parameters names [wip] error message suggesting Type Provider static parameters names May 9, 2021
@smoothdeveloper
Copy link
Contributor Author

@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.

@KevinRansom
Copy link
Contributor

@smoothdeveloper ,

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants