From 91f7e5596cab33d44d99eeb3b99eeee682db6fb1 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 18 Jun 2026 13:44:20 +0200 Subject: [PATCH] Fix F# editor semantic-classification issues (#19905 items 1/2/5/7) Items 1/2/5/7 are completion-safe and shipped here: - item 1: suppress the synthesized delegate Invoke value over the `delegate of ...` RHS - item 2: classify CE builders inside list/array comprehensions as ComputationExpression - item 5: don't paint slice brackets as Method; fix open-ended slice upper-bound range - item 7: `open type T` is not unused when its static members/fields or DU cases are used Items 3/4/6 (generic `<...>` argument colorization) are deferred: narrowing the shared name-resolution range to fix colorization breaks find-all-references, completion and parameter-info, because the resolution sink matches symbol uses by exact range end. The correct fix requires separating classification ranges from symbol-use ranges in the sink. Guard tests assert the IDE features still work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 1 + .../Checking/Expressions/CheckExpressions.fs | 2 +- .../Service/SemanticClassification.fs | 60 +++- src/Compiler/Service/ServiceAnalysis.fs | 37 ++- .../SemanticClassificationRegressions.fs | 302 ++++++++++++++++++ .../EditorTests.fs | 25 +- 6 files changed, 398 insertions(+), 29 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 194de518bcb..d342bd416d8 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -1,5 +1,6 @@ ### Fixed +* Fix several F# editor semantic-classification errors: F# delegate declarations no longer highlight the `delegate of …` syntax as a method, computation-expression builders inside list/array comprehensions are classified as `ComputationExpression`, the closing `]` of an open-ended slice (e.g. `xs[0..]`) is no longer classified as `Function`/`Method`, and `open type T` is no longer reported as unused when its imported members (static members, static fields, or DU union cases) are used. ([Issue #19905](https://github.com/dotnet/fsharp/issues/19905), [PR #19960](https://github.com/dotnet/fsharp/pull/19960)) * Tooltip "Full name" now shows demangled companion module names (e.g. `MyType.func` instead of `MyTypeModule.func`). ([Issue #17335](https://github.com/dotnet/fsharp/issues/17335), [PR #19867](https://github.com/dotnet/fsharp/pull/19867)) * Fix internal error (FS0193) when calling an indexed property setter with a named argument that matches an indexer parameter. ([Issue #16034](https://github.com/dotnet/fsharp/issues/16034), [PR #19851](https://github.com/dotnet/fsharp/pull/19851)) * Fix missing FS1182 ("unused binding") warning for unused `let` function bindings inside class types. ([Issue #13849](https://github.com/dotnet/fsharp/issues/13849), [PR #19805](https://github.com/dotnet/fsharp/pull/19805)) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index bfc90ecf1ce..d84c75aea50 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -6772,7 +6772,7 @@ and ExpandIndexArgs (cenv: cenv) (synLeftExprOpt: SynExpr option) indexArgs = | Some (a2, isFromEnd2) -> yield mkSynSomeExpr range2 (if isFromEnd2 then rewriteReverseExpr pos a2 range2 else a2) | None -> - yield mkSynNoneExpr range1 + yield mkSynNoneExpr range2 ] ) |> List.collect id diff --git a/src/Compiler/Service/SemanticClassification.fs b/src/Compiler/Service/SemanticClassification.fs index da0193e4405..7805445bd32 100644 --- a/src/Compiler/Service/SemanticClassification.fs +++ b/src/Compiler/Service/SemanticClassification.fs @@ -185,22 +185,36 @@ module TcResolutionsExtensions = if cnrs.Length = 1 then cnrs - elif cnrs.Length = 2 then - match cnrs[0].Item, cnrs[1].Item with - | Item.Value _, Item.CustomBuilder _ -> [| cnrs[1] |] - | Item.CustomBuilder _, Item.Value _ -> [| cnrs[0] |] - | _ -> cnrs else - cnrs + let isCE (cnr: CapturedNameResolution) = + match cnr.Item with + | Item.CustomBuilder _ + | Item.CustomOperation _ -> true + | _ -> false + + let isPlainValue (cnr: CapturedNameResolution) = + match cnr.Item with + | Item.Value _ -> true + | _ -> false + + let ceCnrs = cnrs |> Array.filter isCE + + if ceCnrs.Length > 0 && cnrs |> Array.forall (fun c -> isCE c || isPlainValue c) then + ceCnrs + else + cnrs let resolutions = - match range with - | Some range -> - sResolutions.CapturedNameResolutions.ToArray() - |> Array.filter (fun cnr -> rangeContainsPos range cnr.Range.Start || rangeContainsPos range cnr.Range.End) - |> Array.groupBy (fun cnr -> cnr.Range) - |> Array.collect (fun (_, cnrs) -> takeCustomBuilder cnrs) - | None -> sResolutions.CapturedNameResolutions.ToArray() + let baseCnrs = + match range with + | Some range -> + sResolutions.CapturedNameResolutions.ToArray() + |> Array.filter (fun cnr -> rangeContainsPos range cnr.Range.Start || rangeContainsPos range cnr.Range.End) + | None -> sResolutions.CapturedNameResolutions.ToArray() + + baseCnrs + |> Array.groupBy (fun cnr -> cnr.Range) + |> Array.collect (fun (_, cnrs) -> takeCustomBuilder cnrs) let duplicates = HashSet(comparer) @@ -210,6 +224,9 @@ module TcResolutionsExtensions = if duplicates.Add m then results.Add(SemanticClassificationItem((m, typ))) + let isDelegateInvokeMemberName name = + name = "Invoke" || name = "BeginInvoke" || name = "EndInvoke" + resolutions |> Array.iter (fun cnr -> match cnr.Item, cnr.ItemOccurrence, cnr.Range with @@ -228,7 +245,13 @@ module TcResolutionsExtensions = elif vref.IsPropertyGetterMethod || vref.IsPropertySetterMethod then add m SemanticClassificationType.Property elif vref.IsMember then - add m SemanticClassificationType.Method + let isSynthesizedDelegateMember = + isDelegateInvokeMemberName vref.DisplayName + && vref.HasDeclaringEntity + && vref.DeclaringEntity.IsFSharpDelegateTycon + + if not isSynthesizedDelegateMember then + add m SemanticClassificationType.Method elif IsOperatorDisplayName vref.DisplayName then add m SemanticClassificationType.Operator else @@ -296,14 +319,17 @@ module TcResolutionsExtensions = let isSynthesizedDelegateMemberInDecl = minfos |> List.forall (fun minfo -> - let name = minfo.LogicalName - - (name = "Invoke" || name = "BeginInvoke" || name = "EndInvoke") + isDelegateInvokeMemberName minfo.LogicalName && minfo.ApparentEnclosingTyconRef.IsFSharpDelegateTycon && rangeContainsRange minfo.ApparentEnclosingTyconRef.Range m) if isSynthesizedDelegateMemberInDecl then () + elif + minfos + |> List.forall (fun minfo -> minfo.LogicalName = "GetSlice" || minfo.LogicalName = "SetSlice") + then + () elif minfos |> List.forall (fun minfo -> minfo.IsExtensionMember || minfo.IsCSharpStyleExtensionMember) diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 6455d9f0ff3..dc0c9a19b44 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/src/Compiler/Service/ServiceAnalysis.fs @@ -24,6 +24,12 @@ module UnusedOpens = /// Compute an indexed table of the set of symbols revealed by 'open', on-demand let revealedSymbols: Lazy> = lazy + let unionCasesRevealedByOpen (e: FSharpEntity) = + if e.IsFSharpUnion && not (e.HasAttribute()) then + e.UnionCases :> seq<_> + else + Seq.empty + let symbols: FSharpSymbol[] = [| for ent in entity.NestedEntities do @@ -33,9 +39,8 @@ module UnusedOpens = for rf in ent.FSharpFields do rf - if ent.IsFSharpUnion && not (ent.HasAttribute()) then - for unionCase in ent.UnionCases do - unionCase + for unionCase in unionCasesRevealedByOpen ent do + unionCase if ent.HasAttribute() then for fv in ent.MembersFunctionsAndValues do @@ -57,6 +62,9 @@ module UnusedOpens = for field in entity.FSharpFields do if field.IsStatic && field.IsLiteral then field + + for unionCase in unionCasesRevealedByOpen entity do + unionCase |] HashSet<_>(symbols, symbolHash) @@ -278,15 +286,24 @@ module UnusedOpens = let symbolUsesRangesByDeclaringEntity = Dictionary(entityHash) - for symbolUse in symbolUses1 do - match symbolUse.Symbol with - | :? FSharpMemberOrFunctionOrValue as f -> - match f.DeclaringEntity with - | Some entity when entity.IsNamespace || entity.IsFSharpModule -> - symbolUsesRangesByDeclaringEntity.BagAdd(entity, symbolUse.Range) - | _ -> () + let recordByDeclaringEntity (symbolUse: FSharpSymbolUse) = + let declaringEntity, importedByOpenType = + match symbolUse.Symbol with + | :? FSharpMemberOrFunctionOrValue as f -> f.DeclaringEntity, not f.IsInstanceMember + | :? FSharpField as f -> f.DeclaringEntity, f.IsStatic + | _ -> None, false + + match declaringEntity with + | Some entity when entity.IsNamespace || entity.IsFSharpModule || importedByOpenType -> + symbolUsesRangesByDeclaringEntity.BagAdd(entity, symbolUse.Range) | _ -> () + for symbolUse in symbolUses1 do + recordByDeclaringEntity symbolUse + + for symbolUse in symbolUses2 do + recordByDeclaringEntity symbolUse + let! results = filterOpenStatementsIncremental symbolUses2 diff --git a/tests/FSharp.Compiler.ComponentTests/FSharpChecker/SemanticClassificationRegressions.fs b/tests/FSharp.Compiler.ComponentTests/FSharpChecker/SemanticClassificationRegressions.fs index f2a2503da1f..8d6332af7ea 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharpChecker/SemanticClassificationRegressions.fs +++ b/tests/FSharp.Compiler.ComponentTests/FSharpChecker/SemanticClassificationRegressions.fs @@ -271,3 +271,305 @@ type MyDelegate = delegate of int -> string && (let text = substringOfRange source c.Range text = "BeginInvoke" || text = "EndInvoke")) Assert.Empty(asyncInvokeMethods) + +[] +let ``Issue 19905 item 1 - tupled delegate decl has no Method classification`` () = + let source = """ +type SumDelegate = delegate of x: int * y: int -> int +""" + let classifications = getClassifications source + + let badOnDeclLine = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = 2 + && (c.Type = SemanticClassificationType.Method + || c.Type = SemanticClassificationType.ExtensionMethod)) + + Assert.True( + badOnDeclLine.Length = 0, + sprintf + "Expected no Method/ExtensionMethod classification on the delegate declaration line, but found: %A" + (badOnDeclLine + |> Array.map (fun c -> + c.Range.StartColumn, c.Range.EndColumn, c.Type, substringOfRange source c.Range)) + ) + +[] +let ``Issue 19905 item 6 - MailboxProcessor int dot Start classified correctly`` () = + let source = """ +let mbx = MailboxProcessor.Start(fun mbx -> async { return () }) +""" + let classifications = getClassifications source + + let line2 = source.Replace("\r\n", "\n").Split('\n').[1] + let openLt = line2.IndexOf('<') + let closeGt = line2.IndexOf('>') + Assert.True(openLt > 0 && closeGt > openLt, "snippet should contain `` on line 2") + + let mailboxIdx = line2.IndexOf("MailboxProcessor") + let mailboxLen = "MailboxProcessor".Length + + let typeHits = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = 2 + && c.Range.StartColumn = mailboxIdx + && c.Range.EndColumn = mailboxIdx + mailboxLen + && (c.Type = SemanticClassificationType.ReferenceType + || c.Type = SemanticClassificationType.DisposableType + || c.Type = SemanticClassificationType.Type)) + + Assert.True( + typeHits.Length >= 1, + sprintf + "MailboxProcessor identifier should be classified as a type. All classifications on line 2: %A" + (classifications + |> Array.filter (fun c -> c.Range.StartLine = 2) + |> Array.map (fun c -> c.Range.StartColumn, c.Range.EndColumn, c.Type)) + ) + + let badAcrossAngles = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = 2 + && (c.Type = SemanticClassificationType.Function + || c.Type = SemanticClassificationType.Method + || c.Type = SemanticClassificationType.ExtensionMethod) + && c.Range.StartColumn <= openLt + && c.Range.EndColumn > openLt) + + Assert.True( + badAcrossAngles.Length = 0, + sprintf + "No Function/Method classification should cover the `<` of ``, but found: %A" + (badAcrossAngles + |> Array.map (fun c -> + c.Range.StartColumn, c.Range.EndColumn, c.Type, substringOfRange source c.Range)) + ) + + let badAfterAngle = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = 2 + && (c.Type = SemanticClassificationType.Function + || c.Type = SemanticClassificationType.Method + || c.Type = SemanticClassificationType.ExtensionMethod) + && c.Range.StartColumn >= openLt + && c.Range.StartColumn <= closeGt) + + Assert.True( + badAfterAngle.Length = 0, + sprintf + "No Function/Method classification should start inside `<...>`, but found: %A" + (badAfterAngle + |> Array.map (fun c -> + c.Range.StartColumn, c.Range.EndColumn, c.Type, substringOfRange source c.Range)) + ) + +[] +let ``Issue 19905 item 2 - CE inside list comprehension classified as ComputationExpression`` () = + let source = """ +type OptionalBuilder() = + member _.Zero() = None + member _.Bind(x, f) = Option.bind f x + member _.Return(x) = Some x + member _.ReturnFrom(x) = x + +let optional = OptionalBuilder() +let myList = [1; 2; 3] +let myNewList = [ + for i in myList do + optional { + return! Some(i+1) + } +] +""" + let classifications = getClassifications source + + let lines = source.Replace("\r\n", "\n").Split('\n') + let ceLineIdx = + lines + |> Array.findIndex (fun l -> l.TrimStart() = "optional {") + let ceLine = ceLineIdx + 1 + + let line = lines.[ceLineIdx] + let optionalCol = line.IndexOf("optional") + let optionalEndCol = optionalCol + "optional".Length + + let ceHits = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = ceLine + && c.Range.StartColumn = optionalCol + && c.Range.EndColumn = optionalEndCol + && c.Type = SemanticClassificationType.ComputationExpression) + + Assert.True( + ceHits.Length >= 1, + sprintf + "Expected ComputationExpression classification on `optional` at line %d col %d-%d, got: %A" + ceLine optionalCol optionalEndCol + (classifications + |> Array.filter (fun c -> c.Range.StartLine = ceLine) + |> Array.map (fun c -> c.Range.StartColumn, c.Range.EndColumn, c.Type, substringOfRange source c.Range)) + ) + + let valueHits = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = ceLine + && c.Range.StartColumn = optionalCol + && c.Range.EndColumn = optionalEndCol + && (c.Type = SemanticClassificationType.LocalValue + || c.Type = SemanticClassificationType.Value)) + + Assert.True( + valueHits.Length = 0, + sprintf + "`optional` at line %d col %d-%d should not also be classified as LocalValue/Value, got: %A" + ceLine optionalCol optionalEndCol + (valueHits |> Array.map (fun c -> c.Type)) + ) + + +[] +let ``Issue 19905 item 5 - open-ended slice closing bracket not classified as Function`` () = + let source = "\nlet list = [1; 2; 3]\nlet x = list[0..]\n" + let classifications = getClassifications source + let lines = source.Replace("\r\n", "\n").Split('\n') + + let line3 = lines.[2] + let openBr = line3.IndexOf('[') + let closeBr = line3.IndexOf(']', openBr) + Assert.True(openBr > 0 && closeBr > openBr, "snippet should have `[...]` on line 3") + + let badAtClosingBracket = + classifications + |> Array.filter (fun c -> + c.Range.StartLine = 3 + && (c.Type = SemanticClassificationType.Function + || c.Type = SemanticClassificationType.Method + || c.Type = SemanticClassificationType.ExtensionMethod) + && c.Range.StartColumn <= closeBr + && c.Range.EndColumn > closeBr) + + Assert.True( + badAtClosingBracket.Length = 0, + sprintf + "Closing `]` of `list[0..]` should not be inside a Function/Method classification, got: %A" + (badAtClosingBracket + |> Array.map (fun c -> c.Range.StartColumn, c.Range.EndColumn, c.Type, substringOfRange source c.Range)) + ) + +[] +let ``Issue 19905 item 7 - open type not reported as unused`` () = + let source = """module Test + +[] +type View = + static member Window (x: int) = x + static member TextBlock (s: string) = s + +open type View + +let w = Window 1 +let t = TextBlock "hi" + +type Choice = A | B + +open type Choice + +let c = A +""" + + let fileName, snapshot, checker = singleFileChecker source + let parseResults, checkAnswer = checker.ParseAndCheckFileInProject(fileName, snapshot) |> Async.RunSynchronously + let checkResults = getTypeCheckResult (parseResults, checkAnswer) + + let getSourceLineStr lineNo = + let lines = source.Replace("\r\n", "\n").Split('\n') + if lineNo >= 1 && lineNo <= lines.Length then lines.[lineNo - 1] else "" + + let unused = + UnusedOpens.getUnusedOpens(checkResults, getSourceLineStr) + |> Async.RunSynchronously + + let unusedLines = unused |> List.map (fun r -> r.StartLine) |> Set.ofList + + Assert.False( + unusedLines.Contains 8, + sprintf + "`open type View` on line 8 should not be reported as unused. Full unused list: %A" + (unused |> List.map (fun r -> r.StartLine, r.StartColumn, r.EndLine, r.EndColumn)) + ) + + Assert.False( + unusedLines.Contains 15, + sprintf + "`open type Choice` on line 15 should not be reported as unused. Full unused list: %A" + (unused |> List.map (fun r -> r.StartLine, r.StartColumn, r.EndLine, r.EndColumn)) + ) + +[] +let ``Issue 19905 item 7 - open type used via static field not reported as unused`` () = + let source = """module Test +open type System.Math +let _ = PI +""" + let fileName, snapshot, checker = singleFileChecker source + let parseResults, checkAnswer = checker.ParseAndCheckFileInProject(fileName, snapshot) |> Async.RunSynchronously + let checkResults = getTypeCheckResult (parseResults, checkAnswer) + + let getSourceLineStr lineNo = + let lines = source.Replace("\r\n", "\n").Split('\n') + if lineNo >= 1 && lineNo <= lines.Length then lines.[lineNo - 1] else "" + + let unused = + UnusedOpens.getUnusedOpens(checkResults, getSourceLineStr) + |> Async.RunSynchronously + + Assert.True( + (unused |> List.filter (fun r -> r.StartLine = 2)).IsEmpty, + sprintf "`open type System.Math` (line 2) should not be unused; got: %A" + (unused |> List.map (fun r -> r.StartLine, r.StartColumn, r.EndColumn))) + +[] +let ``Issue 19905 item 5 - open-ended array slice closing bracket not classified`` () = + let source = "\nlet arr = [|1;2;3|]\nlet x = arr[0..]\n" + let items = getClassifications source + let line3 = source.Replace("\r\n", "\n").Split('\n').[2] + let closeBracket = line3.LastIndexOf(']') + + let bad = + items + |> Array.filter (fun c -> + c.Range.StartLine = 3 + && (c.Type = SemanticClassificationType.Method || c.Type = SemanticClassificationType.Function) + && c.Range.EndColumn >= closeBracket + 1) + + Assert.True(bad.Length = 0, sprintf "array slice ] painted: %A" (bad |> Array.map (fun c -> c.Range.StartColumn, c.Range.EndColumn, c.Type))) + +[] +let ``Issue 19905 item 7 - open type unused when only an instance record field is used`` () = + let source = """module Test +type R = { X: int } +open type R +let r = { X = 1 } +let _ = r.X +""" + let fileName, snapshot, checker = singleFileChecker source + let parseResults, checkAnswer = checker.ParseAndCheckFileInProject(fileName, snapshot) |> Async.RunSynchronously + let checkResults = getTypeCheckResult (parseResults, checkAnswer) + + let getSourceLineStr lineNo = + let lines = source.Replace("\r\n", "\n").Split('\n') + if lineNo >= 1 && lineNo <= lines.Length then lines.[lineNo - 1] else "" + + let unused = + UnusedOpens.getUnusedOpens(checkResults, getSourceLineStr) + |> Async.RunSynchronously + + Assert.False( + (unused |> List.filter (fun r -> r.StartLine = 3)).IsEmpty, + "`open type R` (line 3) should be unused when only an instance record field of R is used") diff --git a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs b/tests/FSharp.Compiler.Service.Tests/EditorTests.fs index f9c1da195fe..8884b126358 100644 --- a/tests/FSharp.Compiler.Service.Tests/EditorTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/EditorTests.fs @@ -2194,4 +2194,27 @@ type X = let x = new X() let _ = { field1 =; f{caret} } """ - assertItemsWithNames false ["field1"; "field2"] info \ No newline at end of file + assertItemsWithNames false ["field1"; "field2"] info + +[] +let ``19905 - object-initializer property completion still works`` () = + let parseResults, checkResults = getParseAndCheckResults "\ntype A() =\n member val SettableProperty = 1 with get,set\n member val NonSettableProperty = 1\nA()\n" + let decls = checkResults.GetDeclarationListInfo(Some parseResults, 5, "A()", PartialLongName.Empty(2), (fun _ -> [])) + let names = decls.Items |> Array.map (fun i -> i.NameInCode) |> Set.ofArray + Assert.True(names.Contains "SettableProperty", sprintf "object-initializer completion regressed: %d items" names.Count) + +[] +let ``19905 - generic constructor parameter info still works`` () = + let parseResults, checkResults = getParseAndCheckResults "\nopen System.Collections.Generic\nlet _ = new Dictionary<_, _>()\n" + match parseResults.FindParameterLocations(FSharp.Compiler.Text.Position.mkPos 3 29) with + | None -> Assert.True(false, "FindParameterLocations returned None for generic ctor") + | Some nwpl -> + let lidEnd = nwpl.LongIdEndLocation + let methods = checkResults.GetMethods(lidEnd.Line, lidEnd.Column, "", Some nwpl.LongId) + Assert.True(methods.Methods.Length > 0, "generic constructor parameter info regressed (no methods)") + +[] +let ``19905 - custom GetSlice usage via slice syntax is found`` () = + let _, checkResults = getParseAndCheckResults "\ntype T() =\n member _.GetSlice(a: int option, b: int option) = [a; b]\nlet xs = T()\nlet ys = xs.[0..2]\n" + let callSites = checkResults.GetAllUsesOfAllSymbolsInFile() |> Seq.filter (fun u -> u.Symbol.DisplayName = "GetSlice" && not u.IsFromDefinition) |> Seq.length + Assert.True(callSites > 0, "custom GetSlice slice call site not found by find-all-references")