dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.82k stars 773 forks source link

Fix reporting IsFromComputationExpression for inappropriate symbols #17375

Open DedSec256 opened 2 days ago

DedSec256 commented 2 days ago

Description

Upon investigating the syntax highlighting issue for CE code in Rider, I found that FCS reports a separate SymbolUse for expressions that are not a constructor invocation of the CE builder type or an associated with it let-binding:

image

This behavior seems incorrect because the Object.Prop expression does not appear to require highlighting as a CE keyword. Unexpected case, and therefore it may not be properly handled, which can lead to incorrect overlapped code highlighting, for example, like this

image

or even worse

image

and can also lead to navigation errors in the 'go to definition' due to overlapping SymbolUse ranges.

The same issue is reproducible in VS/VS Code.

https://github.com/dotnet/fsharp/blob/5c6d8e705a4152ca2711ba57e58850fc561eafea/src/Compiler/Service/FSharpCheckerResults.fs#L236-L242

Also, I did not find any tests confirming that IsFromComputationExpression should be true for such expressions.

Fix

This PR suggests reporting IsFromComputationExpression only for CE Builder type constructors and let-bindings.

Checklist

github-actions[bot] commented 2 days ago

:heavy_exclamation_mark: Release notes required


:white_check_mark: Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md
brianrourkeboll commented 1 day ago

Hmm, VS does not colorize things like A.Builder { return 3 } or System.Object.Builder { return 3 } as keywords. Does Rider currently do so? Or are you just saying that the extra SymbolUse shouldn't be there?

image

DedSec256 commented 1 day ago

@brianrourkeboll,

You can notice incorrect coloring of parentheses as CE keyword color there.

image

It also seems incorrect for a more complex case (tested on VS v17.10.3):

image

There is also the issue in VS Code:

image

I did not research the logic of how Visual Studio/VS Code maps symbols to highlightings; however, the extra SymbolUse with IsFromComputationExpression indeed shouldn't be listed for cases from this PR.

As a result, it will also fix highlightings in Rider (and, hopefully, in VS/VS Code)

PS. Thanks, I updated the PR description a little bit

DedSec256 commented 1 day ago

Perhaps this case is OK 🤔

image

brianrourkeboll commented 1 day ago

Perhaps this case is OK 🤔

image

Hmm, yeah, I mean how far do we want to go? :)

image

baronfel commented 1 day ago

For context, here's how FSAC does semantic tokenization.

Generally we take the classifications directly from FCS via the SemanticClassificationType that is determined for a range, but we have to map those SemanticClassificationTypes to a set of SemanticTokenType and SemanticTokenModifier that are defined partially in the LSP spec and partially in FSAC's LSP bindings. This combination of token type + token modifier is mapped to predefined and user-defined editor styling.

So in general we assume FSC is true/correct, and try to do the most minimal mapping/handling possible for this feature area.