Fix reporting IsFromComputationExpression for inappropriate symbols

[DedSec256]

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:

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

or even worse

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.

fsharp/src/Compiler/Service/FSharpCheckerResults.fs

Lines 236 to 242 in 5c6d8e7

	member _.IsFromComputationExpression =
	match symbol.Item, itemOcc with
	// 'seq' in 'seq { ... }' gets colored as keywords
	| Item.Value vref, ItemOccurence.Use when valRefEq denv.g denv.g.seq_vref vref -> true
	// custom builders, custom operations get colored as keywords
	| (Item.CustomBuilder _ | Item.CustomOperation _), ItemOccurence.Use -> true
	| _ -> false

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

• Test cases added
• Release notes entry updated

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.100.md

[brianrourkeboll]

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?

[DedSec256]

@brianrourkeboll,

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

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

There is also the issue in VS Code:

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]

Perhaps this case is OK 🤔

[brianrourkeboll]

Perhaps this case is OK 🤔

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

[baronfel]

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.

[T-Gro]

@brianrourkeboll,

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

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

There is also the issue in VS Code:

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

I like this change @DedSec256 .
Since you already created samples+screenshots showing the issue, could you do the exact same snippets with the included fix?

At least the VS once, by starting it in VisualFsharp.sln

[DedSec256]

@T-Gro, have checked before and after the fix. The issue is resolved.

Before

After

As you can see, the unit parameter of the constructor is still being highlighted in blue, but I think this is a separate issue; I don't observe the same problem in Rider/VS Code.

[psfinaki]

Nice job! Thanks for adding a test there as well.

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).