Correctly colorize parameterized CE builders

[brianrourkeboll]

Change

• Enable correct colorization of parameterized custom computation expression builders, e.g.:
  • builder<…> { … } (type application).
  • builder () { … } (function application).¹

Edit: It turns out that there was a very old issue for this, which might as well be linked: #110.

Example

In the following example, builder is now correctly colorized as a keyword (like async and any other custom builder that doesn't have generic type parameters).

type Builder<'T> () =
    member _.Return x = x
    member _.Run x = x

let builder<'T> = Builder<'T> ()

let x = builder { return 3 }
let y = builder<int> { return 3 }
let z = builder<_> { return 3 }

Before

After

Tests

SyntacticClassificationServiceTests seems to be too low-level to catch this change. I have added tests for FSharpSymbolUse.IsFromComputationExpression under Tests.Service.Symbols.ComputationExpressions.

Footnotes

1. This does not apply to direct invocations of constructors, e.g., Builder () { … }. I think this behavior makes sense, since a Pascal-case constructor doesn't "feel" like it should get keyword coloring, even in this context.

   I can think of even funkier valid cases where keyword coloring still does not apply and almost certainly (?) shouldn't…

   type System.Int32 with
       member _.Return x = x
       member _.Run x = x
   
   let uh = 99 { return 3 }
   

   type List<'T> with
       member _.Return x = x
       member _.Run x = x
   
   let yeah = [1..10] { return 3 }
   

   ↩

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

Ohh thanks - that looks useful!

As for testing - I haven't done much in the colorization space but to me SyntacticClassificationServiceTests look about right. Why do you think they won't be enough?

[auduchinok]

I'm realizing after the fact that this will also apply to functions that return builders, e.g.,
I believe this should be fine, unless there's a compelling reason to keep distinct coloring for such builder functions. ↩

Hm, this doesn't sound right to me. We only colorize the builder values as keywords when they are used in an actual sequence expression application. In some of the tools this is done via checking symbolUse.IsFromComputationExpression. There're other features that depend on this flag: code completion may look at it for sorting lookup items, for instance. Changing this behaviour for unrelated symbols (i.e. a function that happen to return a builder type) is something we should try to avoid, as it'll change/break the behaviour of unrelated features.

[vzarytovskii]

I'm realizing after the fact that this will also apply to functions that return builders, e.g.,
I believe this should be fine, unless there's a compelling reason to keep distinct coloring for such builder functions. ↩

Hm, this doesn't sound right to me. We only colorize the builder values as keywords when they are used in an actual sequence expression application. In some of the tools this is done via checking symbolUse.IsFromComputationExpression. There're other features that depend on this flag: code completion may look at it for sorting lookup items, for instance. Changing this behaviour for unrelated symbols (i.e. a function that happen to return a builder type) is something we should try to avoid, as it'll change/break the behaviour of unrelated features.

Tbh, I'm not horribly against it, however this should be done in tooling instead (in semantic highlighting, I presume).

[auduchinok]

Tbh, I'm not horribly against it, however this should be done in tooling instead (in semantic highlighting, I presume).

Yes, and we should have tests that show that symbolUse.IsFromComputationExpression is not changed if we decide to change the highlighting.

[brianrourkeboll]

@auduchinok

I'm realizing after the fact that this will also apply to functions that return builders, e.g.,
I believe this should be fine, unless there's a compelling reason to keep distinct coloring for such builder functions. ↩

Hm, this doesn't sound right to me. We only colorize the builder values as keywords when they are used in an actual sequence expression application. In some of the tools this is done via checking symbolUse.IsFromComputationExpression. There're other features that depend on this flag: code completion may look at it for sorting lookup items, for instance. Changing this behaviour for unrelated symbols (i.e. a function that happen to return a builder type) is something we should try to avoid, as it'll change/break the behaviour of unrelated features.

This change gives such functions keyword coloring (and IsFromComputationExpression returns true) only in a context like builder () { … }, not in any other scenario. See:

However, if we still think keyword colorization shouldn't apply in such cases, I can make this change:

- | Expr.App (funcExpr = Expr.Val (vref, _, m))
+ | Expr.App (funcExpr = Expr.Val (vref, _, m); args = [])

args = [] means that it will match for type applications (like builder<…>) but not for function applications (like builder ()).

[auduchinok]

This change only gives such functions keyword coloring (and IsFromComputationExpression returns true) only in a context like builder () { … }, not in any other scenario.

This looks nice! Would be good to have the tests anyway 🙂

[brianrourkeboll]

@psfinaki

Ohh thanks - that looks useful!

As for testing - I haven't done much in the colorization space but to me SyntacticClassificationServiceTests look about right. Why do you think they won't be enough?

If I add a test like this:

[<Theory>]
[<InlineData("builder")>]
[<InlineData("builder<int>")>]
[<InlineData("builder<_>")>]
member this.CustomBuilder_GenericTypeParameters builder =
    this.VerifyColorizerAtEndOfMarker(
        fileContents = $"
type Builder<'T> () =
    member _.Return x = x
    member _.Run x = x

let builder<'T> = Builder<'T> ()

let x = (*marker*)%s{builder} {{ return 3 }}
",
        marker = "(*marker*)b",
        defines = [],
        classificationType = ClassificationTypeNames.Keyword)

…it fails because builder is classified as an identifier, not a keyword. I think that's because those tests are relying only on the tokenizer, which doesn't classify computation expression builder values differently, as far as I can tell.

[brianrourkeboll]

@auduchinok

This change only gives such functions keyword coloring (and IsFromComputationExpression returns true) only in a context like builder () { … }, not in any other scenario.

This looks nice! Would be good to have the tests anyway 🙂

Are these similar to what you were thinking? 9b7579e