diff --git a/src/Compiler/Service/QuickParse.fs b/src/Compiler/Service/QuickParse.fs index 6a3e9a2b8b..db377baf6b 100644 --- a/src/Compiler/Service/QuickParse.fs +++ b/src/Compiler/Service/QuickParse.fs @@ -90,6 +90,14 @@ module QuickParse = && (lineStr[index] = '|' || IsIdentifierPartCharacter lineStr[index]) -> Some index + // Handle optional parameter syntax: if we're on '?' and the next char is an identifier, use the next position + | _ when + (index < lineStr.Length) + && lineStr[index] = '?' + && (index + 1 < lineStr.Length) + && IsIdentifierPartCharacter lineStr[index + 1] + -> + Some(index + 1) | _ -> None // not on a word or '.' let (|Char|_|) p = diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj index 1244517a4c..3e5e2c5894 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj +++ b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj @@ -54,6 +54,7 @@ + diff --git a/tests/FSharp.Compiler.Service.Tests/QuickParseTests.fs b/tests/FSharp.Compiler.Service.Tests/QuickParseTests.fs new file mode 100644 index 0000000000..905e043d73 --- /dev/null +++ b/tests/FSharp.Compiler.Service.Tests/QuickParseTests.fs @@ -0,0 +1,73 @@ +module FSharp.Compiler.Service.Tests.QuickParseTests + +open Xunit +open FSharp.Compiler.EditorServices + +// QuickParse.GetCompleteIdentifierIsland is used by language service features +// to extract identifier context from source text at cursor positions. +// When it returns None (as it did for '?' before the fix), downstream services +// like semantic classification, completion, and hover can misinterpret the context. +// This impacts Visual Studio's syntax highlighting - see issue #11008753 + +[] +let ``QuickParse handles optional parameter identifier extraction when cursor is on question mark``() = + let lineStr = "member _.memb(?optional:string) = optional" + + // Test when cursor is exactly on the '?' character + let posOnQuestionMark = 14 + Assert.Equal('?', lineStr[posOnQuestionMark]) + + let island = QuickParse.GetCompleteIdentifierIsland false lineStr posOnQuestionMark + + // We expect to get "optional" as the identifier + Assert.True(Option.isSome island, "Should extract identifier island when positioned on '?'") + + match island with + | Some(ident, startCol, isQuoted) -> + Assert.Equal("optional", ident) + Assert.False(isQuoted) + // The identifier should start after the '?' + Assert.True(startCol >= 15, sprintf "Start column %d should be >= 15" startCol) + | None -> + Assert.Fail("Expected to find identifier 'optional' when positioned on '?'") + +[] +let ``QuickParse handles optional parameter identifier extraction when cursor is on identifier``() = + let lineStr = "member _.memb(?optional:string) = optional" + + // Test when cursor is on the identifier "optional" after the '?' + let posOnOptional = 17 + Assert.Equal('t', lineStr[posOnOptional]) + + let island = QuickParse.GetCompleteIdentifierIsland false lineStr posOnOptional + + // We expect to get "optional" as the identifier + Assert.True(Option.isSome island, "Should extract identifier island when positioned on identifier") + + match island with + | Some(ident, startCol, isQuoted) -> + Assert.Equal("optional", ident) + Assert.False(isQuoted) + | None -> + Assert.Fail("Expected to find identifier 'optional'") + +[] +let ``QuickParse does not treat question mark as identifier in other contexts``() = + let lineStr = "let x = y ? z" + + // Test when cursor is on the '?' in a different context (not optional parameter) + let posOnQuestionMark = 10 + Assert.Equal('?', lineStr[posOnQuestionMark]) + + let island = QuickParse.GetCompleteIdentifierIsland false lineStr posOnQuestionMark + + // In this context, '?' is followed by space, not an identifier start + // So we should get None or the next identifier 'z' + // Let's check what we actually get + match island with + | Some(ident, _, _) -> + // If we get something, it should be 'z' (the next identifier after the space) + Assert.Equal("z", ident) + | None -> + // Or we might get None, which is also acceptable + () diff --git a/tests/FSharp.Compiler.Service.Tests/TokenizerTests.fs b/tests/FSharp.Compiler.Service.Tests/TokenizerTests.fs index 53b2127a83..d288db5dff 100644 --- a/tests/FSharp.Compiler.Service.Tests/TokenizerTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/TokenizerTests.fs @@ -223,3 +223,26 @@ let ``Unfinished idents``() = ["IDENT", "```"]] actual |> Assert.shouldBe expected + +[] +let ``Tokenizer test - optional parameters with question mark``() = + let tokenizedLines = + tokenizeLines + [| "member _.memb(?optional:string) = optional" |] + + let actual = + [ for lineNo, lineToks in tokenizedLines do + yield lineNo, [ for str, info in lineToks do yield info.TokenName, str ] ] + + let expected = + [(0, + [("MEMBER", "member"); ("WHITESPACE", " "); ("UNDERSCORE", "_"); ("DOT", "."); + ("IDENT", "memb"); ("LPAREN", "("); ("QMARK", "?"); + ("IDENT", "optional"); ("COLON", ":"); ("IDENT", "string"); + ("RPAREN", ")"); ("WHITESPACE", " "); ("EQUALS", "="); ("WHITESPACE", " "); + ("IDENT", "optional")])] + + if actual <> expected then + printfn "actual = %A" actual + printfn "expected = %A" expected + actual |> Assert.shouldBeEqualWith expected (sprintf "actual and expected did not match,actual =\n%A\nexpected=\n%A\n" actual expected) diff --git a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs index eae06773f6..2df4a48653 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs @@ -240,3 +240,69 @@ module ``It should still show up as a keyword even if the type parameter is inva """ verifyClassificationAtEndOfMarker (sourceText, marker, classificationType) + + [] + member _.``Optional parameters should be classified correctly``() = + let sourceText = + """ +type TestType() = + member _.memb(?optional:string) = optional +""" + + let ranges = getRanges sourceText + + // The issue was that QuickParse returning None for '?' caused misclassification + // This test verifies that we get semantic classification data and nothing is + // incorrectly classified as a type or namespace due to the ? prefix + + // Look for any identifier "optional" in the classifications + let text = SourceText.From(sourceText) + + let optionalRanges = + ranges + |> List.filter (fun item -> + try + // Get the actual text from the source using SourceText + let span = RoslynHelpers.TryFSharpRangeToTextSpan(text, item.Range) + + match span with + | ValueSome textSpan -> + let actualText = text.GetSubText(textSpan).ToString() + actualText = "optional" + | ValueNone -> false + with _ -> + false) + + // Provide detailed diagnostics if test fails + let allClassifications = + ranges + |> List.map (fun item -> + try + let span = RoslynHelpers.TryFSharpRangeToTextSpan(text, item.Range) + + let textStr = + match span with + | ValueSome ts -> text.GetSubText(ts).ToString() + | ValueNone -> "[no span]" + + sprintf "Range %A: '%s' (%A)" item.Range textStr item.Type + with ex -> + sprintf "Range %A: [error: %s] (%A)" item.Range ex.Message item.Type) + |> String.concat "\n" + + let errorMessage = + sprintf + "Should have classification data for 'optional' identifier.\nFound %d ranges total.\nAll classifications:\n%s" + ranges.Length + allClassifications + + Assert.True(optionalRanges.Length > 0, errorMessage) + + // Verify that none of the "optional" occurrences are classified as type/namespace + // (which would indicate the bug is present) + for optionalRange in optionalRanges do + let classificationType = + FSharpClassificationTypes.getClassificationTypeName optionalRange.Type + + Assert.NotEqual(ClassificationTypeNames.ClassName, classificationType) + Assert.NotEqual(ClassificationTypeNames.NamespaceName, classificationType)