From 751026fd328dd3f0a6927bfeae1ddbbbc062c3a3 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Tue, 26 Sep 2023 19:03:52 -0400 Subject: [PATCH 1/6] Correctly colorize custom CE builders with typars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * This change enables correct colorization of custom computation expression builders with generic type parameters, e.g., `builder<…>`. --- src/Compiler/Checking/CheckComputationExpressions.fs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Compiler/Checking/CheckComputationExpressions.fs b/src/Compiler/Checking/CheckComputationExpressions.fs index e27dcffc7bf..6fa5bbbbebb 100644 --- a/src/Compiler/Checking/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/CheckComputationExpressions.fs @@ -229,7 +229,10 @@ let TcComputationExpression (cenv: cenv) env (overallTy: OverallTy) tpenv (mWhol // Give bespoke error messages for the FSharp.Core "query" builder let isQuery = match stripDebugPoints interpExpr with - | Expr.Val (vref, _, m) -> + // A regular custom builder, e.g., async. + | Expr.Val (vref, _, m) + // A custom builder with generic type parameters, e.g., builder<…>. + | Expr.App (funcExpr=Expr.Val (vref, _, m)) -> let item = Item.CustomBuilder (vref.DisplayName, vref) CallNameResolutionSink cenv.tcSink (m, env.NameEnv, item, emptyTyparInst, ItemOccurence.Use, env.eAccessRights) valRefEq cenv.g vref cenv.g.query_value_vref From fa876fa494acfc7573d8936a496fcfb3726b3520 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 27 Sep 2023 09:35:15 -0400 Subject: [PATCH 2/6] Add spaces + clarify comments --- src/Compiler/Checking/CheckComputationExpressions.fs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compiler/Checking/CheckComputationExpressions.fs b/src/Compiler/Checking/CheckComputationExpressions.fs index 6fa5bbbbebb..92327ed3a99 100644 --- a/src/Compiler/Checking/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/CheckComputationExpressions.fs @@ -229,10 +229,10 @@ let TcComputationExpression (cenv: cenv) env (overallTy: OverallTy) tpenv (mWhol // Give bespoke error messages for the FSharp.Core "query" builder let isQuery = match stripDebugPoints interpExpr with - // A regular custom builder, e.g., async. + // A non-generic custom builder, e.g., `query`, `async`. | Expr.Val (vref, _, m) - // A custom builder with generic type parameters, e.g., builder<…>. - | Expr.App (funcExpr=Expr.Val (vref, _, m)) -> + // A custom builder with parameters, e.g., `builder<…>`, `builder ()`. + | Expr.App (funcExpr = Expr.Val (vref, _, m)) -> let item = Item.CustomBuilder (vref.DisplayName, vref) CallNameResolutionSink cenv.tcSink (m, env.NameEnv, item, emptyTyparInst, ItemOccurence.Use, env.eAccessRights) valRefEq cenv.g vref cenv.g.query_value_vref From 9b7579e2d481ea5926d239d81f7915591a77ca16 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 27 Sep 2023 09:40:25 -0400 Subject: [PATCH 3/6] Add symbol use tests for CE builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Test that FSharpSymbolUse.IsFromComputationExpression only returns true in `builder { … }`, `builder () { … }`, `builder<…> { … }`. --- tests/service/Symbols.fs | 98 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/tests/service/Symbols.fs b/tests/service/Symbols.fs index 59ac5b9b84f..7ceac62ad59 100644 --- a/tests/service/Symbols.fs +++ b/tests/service/Symbols.fs @@ -951,4 +951,100 @@ let nestedFunc (a: RecordA) = { a with Zoo.Foo = 1; Zoo.Zoo.Bar = 2; Zoo.Ba Assert.AreEqual ("RecordA`1", field.DeclaringEntity.Value.CompiledName) assertRange (4, 87) (4, 90) fieldSymbolUse.Range - | _ -> Assert.Fail "Symbol was not FSharpField" \ No newline at end of file + | _ -> Assert.Fail "Symbol was not FSharpField" + +module ComputationExpressions = + [] + let ``IsFromComputationExpression only returns true for 'builder' in 'builder { … }'`` () = + let _, checkResults = getParseAndCheckResults """ +type Builder () = + member _.Return x = x + member _.Run x = x + +let builder = Builder () + +let x = builder { return 3 } +let y = builder +""" + + match checkResults.GetSymbolUseAtLocation(6, 11, "let builder = Builder ()", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder { return 3 }", ["builder"]) with + | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(9, 15, "let y = builder", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + [] + let ``IsFromComputationExpression only returns true for 'builder' in 'builder<…> { … }'`` () = + let _, checkResults = getParseAndCheckResults """ +type Builder<'T> () = + member _.Return x = x + member _.Run x = x + +let builder<'T> = Builder<'T> () + +let x = builder { return 3 } +let y = builder { return 3 } +let z = builder<_> { return 3 } +let p = builder +let q<'T> = builder<'T> +""" + + match checkResults.GetSymbolUseAtLocation(6, 11, "let builder<'T> = Builder<'T> ()", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder { return 3 }", ["builder"]) with + | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(9, 15, "let x = builder { return 3 }", ["builder"]) with + | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(10, 15, "let x = builder<_> { return 3 }", ["builder"]) with + | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(11, 15, "let p = builder", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(12, 15, "let q<'T> = builder<'T>", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + [] + let ``IsFromComputationExpression only returns true for 'builder' in 'builder () { … }'`` () = + let _, checkResults = getParseAndCheckResults """ +type Builder () = + member _.Return x = x + member _.Run x = x + +let builder () = Builder () + +let x = builder () { return 3 } +let y = builder () +let z = builder +""" + + match checkResults.GetSymbolUseAtLocation(6, 11, "let builder () = Builder ()", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder () { return 3 }", ["builder"]) with + | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder () { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(9, 15, "let y = builder ()", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." + + match checkResults.GetSymbolUseAtLocation(10, 15, "let z = builder", ["builder"]) with + | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") + | None -> Assert.Fail "Symbol was not 'builder'." From 618960e3faf0ae7264ce0055cbb9de15a5bf1c40 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 27 Sep 2023 10:34:23 -0400 Subject: [PATCH 4/6] Update tests to use `shouldEqual` * For the double symbol uses for the builders, see: https://github.com/dotnet/fsharp/blob/fb39b9440dd27f5ed093e0617cdce49d9dbffa3c/src/Compiler/Service/SemanticClassification.fs#L179-L192 --- tests/service/Symbols.fs | 104 ++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 44 deletions(-) diff --git a/tests/service/Symbols.fs b/tests/service/Symbols.fs index 7ceac62ad59..cc965b6aa4b 100644 --- a/tests/service/Symbols.fs +++ b/tests/service/Symbols.fs @@ -967,17 +967,23 @@ let x = builder { return 3 } let y = builder """ - match checkResults.GetSymbolUseAtLocation(6, 11, "let builder = Builder ()", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." - - match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder { return 3 }", ["builder"]) with - | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." - - match checkResults.GetSymbolUseAtLocation(9, 15, "let y = builder", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + shouldEqual + [ + // let builder = Builder () + (6, 4), false + + // let x = builder { return 3 } + (8, 8), false // Item.Value _ + (8, 8), true // Item.CustomBuilder _ + + // let y = builder + (9, 8), false + ] + [ + for symbolUse in checkResults.GetAllUsesOfAllSymbolsInFile() do + if symbolUse.Symbol.DisplayName = "builder" then + (symbolUse.Range.StartLine, symbolUse.Range.StartColumn), symbolUse.IsFromComputationExpression + ] [] let ``IsFromComputationExpression only returns true for 'builder' in 'builder<…> { … }'`` () = @@ -995,29 +1001,34 @@ let p = builder let q<'T> = builder<'T> """ - match checkResults.GetSymbolUseAtLocation(6, 11, "let builder<'T> = Builder<'T> ()", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + shouldEqual + [ + // let builder<'T> = Builder<'T> () + (6, 4), false - match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder { return 3 }", ["builder"]) with - | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + // let x = builder { return 3 } + (8, 8), false // Item.Value _ + (8, 8), true // Item.CustomBuilder _ - match checkResults.GetSymbolUseAtLocation(9, 15, "let x = builder { return 3 }", ["builder"]) with - | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + // let y = builder { return 3 } + (9, 8), false // Item.Value _ + (9, 8), true // Item.CustomBuilder _ - match checkResults.GetSymbolUseAtLocation(10, 15, "let x = builder<_> { return 3 }", ["builder"]) with - | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + // let z = builder<_> { return 3 } + (10, 8), false // Item.Value _ + (10, 8), true // Item.CustomBuilder _ - match checkResults.GetSymbolUseAtLocation(11, 15, "let p = builder", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + // let p = builder + (11, 8), false - match checkResults.GetSymbolUseAtLocation(12, 15, "let q<'T> = builder<'T>", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder<…> { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + // let q<'T> = builder<'T> + (12, 12), false + ] + [ + for symbolUse in checkResults.GetAllUsesOfAllSymbolsInFile() do + if symbolUse.Symbol.DisplayName = "builder" then + (symbolUse.Range.StartLine, symbolUse.Range.StartColumn), symbolUse.IsFromComputationExpression + ] [] let ``IsFromComputationExpression only returns true for 'builder' in 'builder () { … }'`` () = @@ -1033,18 +1044,23 @@ let y = builder () let z = builder """ - match checkResults.GetSymbolUseAtLocation(6, 11, "let builder () = Builder ()", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." - - match checkResults.GetSymbolUseAtLocation(8, 15, "let x = builder () { return 3 }", ["builder"]) with - | Some symbolUse -> Assert.True (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` in `builder () { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." - - match checkResults.GetSymbolUseAtLocation(9, 15, "let y = builder ()", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." - - match checkResults.GetSymbolUseAtLocation(10, 15, "let z = builder", ["builder"]) with - | Some symbolUse -> Assert.False (symbolUse.IsFromComputationExpression, "IsFromComputationExpression should return true for `builder` only in `builder () { … }`.") - | None -> Assert.Fail "Symbol was not 'builder'." + shouldEqual + [ + // let builder () = Builder () + (6, 4), false + + // let x = builder () { return 3 } + (8, 8), false // Item.Value _ + (8, 8), true // Item.CustomBuilder _ + + // let y = builder () + (9, 8), false + + // let z = builder + (10, 8), false + ] + [ + for symbolUse in checkResults.GetAllUsesOfAllSymbolsInFile() do + if symbolUse.Symbol.DisplayName = "builder" then + (symbolUse.Range.StartLine, symbolUse.Range.StartColumn), symbolUse.IsFromComputationExpression + ] From c46ca120ab3fb5dc4a787355315d176b3767c357 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 27 Sep 2023 10:39:13 -0400 Subject: [PATCH 5/6] Make comments more consistent --- src/Compiler/Checking/CheckComputationExpressions.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Checking/CheckComputationExpressions.fs b/src/Compiler/Checking/CheckComputationExpressions.fs index 92327ed3a99..8ac9066557b 100644 --- a/src/Compiler/Checking/CheckComputationExpressions.fs +++ b/src/Compiler/Checking/CheckComputationExpressions.fs @@ -229,9 +229,9 @@ let TcComputationExpression (cenv: cenv) env (overallTy: OverallTy) tpenv (mWhol // Give bespoke error messages for the FSharp.Core "query" builder let isQuery = match stripDebugPoints interpExpr with - // A non-generic custom builder, e.g., `query`, `async`. + // An unparameterized custom builder, e.g., `query`, `async`. | Expr.Val (vref, _, m) - // A custom builder with parameters, e.g., `builder<…>`, `builder ()`. + // A parameterized custom builder, e.g., `builder<…>`, `builder ()`. | Expr.App (funcExpr = Expr.Val (vref, _, m)) -> let item = Item.CustomBuilder (vref.DisplayName, vref) CallNameResolutionSink cenv.tcSink (m, env.NameEnv, item, emptyTyparInst, ItemOccurence.Use, env.eAccessRights) From 1fee175b408d21a583f82e39819b6e9e3e1a1644 Mon Sep 17 00:00:00 2001 From: Brian Rourke Boll Date: Wed, 27 Sep 2023 12:27:35 -0400 Subject: [PATCH 6/6] Explicitly test handling of builder ctors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * For a computation expression builder type `Builder`, we don't expect keyword highlighting (or `FSharpSymbolUse.IsFromComputationExpression` to return true) in `type Builder () = …` or `Builder () { … }`. --- tests/service/Symbols.fs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/service/Symbols.fs b/tests/service/Symbols.fs index cc965b6aa4b..bd8f138ef48 100644 --- a/tests/service/Symbols.fs +++ b/tests/service/Symbols.fs @@ -965,11 +965,18 @@ let builder = Builder () let x = builder { return 3 } let y = builder +let z = Builder () { return 3 } """ shouldEqual [ - // let builder = Builder () + // type Builder () = + (2, 5), false + + // … = Builder () + (6, 14), false + + // let builder = … (6, 4), false // let x = builder { return 3 } @@ -978,11 +985,15 @@ let y = builder // let y = builder (9, 8), false + + // let z = Builder () { return 3 } + (10, 8), false ] [ for symbolUse in checkResults.GetAllUsesOfAllSymbolsInFile() do - if symbolUse.Symbol.DisplayName = "builder" then - (symbolUse.Range.StartLine, symbolUse.Range.StartColumn), symbolUse.IsFromComputationExpression + match symbolUse.Symbol.DisplayName with + | "Builder" | "builder" -> (symbolUse.Range.StartLine, symbolUse.Range.StartColumn), symbolUse.IsFromComputationExpression + | _ -> () ] []