From b40f73acb14f4d4586b112d09082eef7e2b1560f Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 5 Jan 2021 09:46:58 -0800 Subject: [PATCH] Fix signature help in CEs and use better heuristics (#10835) --- src/fsharp/service/ServiceUntypedParse.fs | 31 +++-- tests/service/ServiceUntypedParseTests.fs | 48 ++++++++ .../FSharp.Editor/Completion/SignatureHelp.fs | 64 +++++----- .../UnitTests/SignatureHelpProviderTests.fs | 115 +++++++++++++++++- 4 files changed, 217 insertions(+), 41 deletions(-) diff --git a/src/fsharp/service/ServiceUntypedParse.fs b/src/fsharp/service/ServiceUntypedParse.fs index 65eaec984fc..9a9d83f059a 100755 --- a/src/fsharp/service/ServiceUntypedParse.fs +++ b/src/fsharp/service/ServiceUntypedParse.fs @@ -187,8 +187,10 @@ type FSharpParseFileResults(errors: FSharpDiagnostic[], input: ParsedInput optio | Some input -> let result = AstTraversal.Traverse(pos, input, { new AstTraversal.AstVisitorBase<_>() with - member _.VisitExpr(_, _, defaultTraverse, expr) = + member _.VisitExpr(_, traverseSynExpr, defaultTraverse, expr) = match expr with + | SynExpr.App(_, _, _, SynExpr.CompExpr (_, _, expr, _), range) when rangeContainsPos range pos -> + traverseSynExpr expr | SynExpr.App (_, _, _, _, range) when rangeContainsPos range pos -> Some range | _ -> defaultTraverse expr @@ -197,40 +199,45 @@ type FSharpParseFileResults(errors: FSharpDiagnostic[], input: ParsedInput optio | None -> false member scope.TryRangeOfFunctionOrMethodBeingApplied pos = - let rec getIdentRangeForFuncExprInApp expr pos = + let rec getIdentRangeForFuncExprInApp traverseSynExpr expr pos = match expr with - | SynExpr.Ident ident -> ident.idRange + | SynExpr.Ident ident -> Some ident.idRange - | SynExpr.LongIdent(_, _, _, range) -> range + | SynExpr.LongIdent(_, _, _, range) -> Some range | SynExpr.Paren(expr, _, _, range) when rangeContainsPos range pos -> - getIdentRangeForFuncExprInApp expr pos + getIdentRangeForFuncExprInApp traverseSynExpr expr pos + + // This matches computation expressions like 'async { ... }' + | SynExpr.App(_, _, _, SynExpr.CompExpr (_, _, expr, range), _) when rangeContainsPos range pos -> + getIdentRangeForFuncExprInApp traverseSynExpr expr pos | SynExpr.App(_, _, funcExpr, argExpr, _) -> match argExpr with | SynExpr.App (_, _, _, _, range) when rangeContainsPos range pos -> - getIdentRangeForFuncExprInApp argExpr pos + getIdentRangeForFuncExprInApp traverseSynExpr argExpr pos | _ -> match funcExpr with | SynExpr.App (_, true, _, _, _) when rangeContainsPos argExpr.Range pos -> // x |> List.map // Don't dive into the funcExpr (the operator expr) // because we dont want to offer sig help for that! - getIdentRangeForFuncExprInApp argExpr pos + getIdentRangeForFuncExprInApp traverseSynExpr argExpr pos | _ -> // Generally, we want to dive into the func expr to get the range // of the identifier of the function we're after - getIdentRangeForFuncExprInApp funcExpr pos - | expr -> expr.Range // Exhaustiveness, this shouldn't actually be necessary...right? + getIdentRangeForFuncExprInApp traverseSynExpr funcExpr pos + | expr -> + traverseSynExpr expr + |> Option.map (fun expr -> expr) match scope.ParseTree with | Some input -> AstTraversal.Traverse(pos, input, { new AstTraversal.AstVisitorBase<_>() with - member _.VisitExpr(_, _, defaultTraverse, expr) = + member _.VisitExpr(_, traverseSynExpr, defaultTraverse, expr) = match expr with | SynExpr.App (_, _, _funcExpr, _, range) as app when rangeContainsPos range pos -> - getIdentRangeForFuncExprInApp app pos - |> Some + getIdentRangeForFuncExprInApp traverseSynExpr app pos | _ -> defaultTraverse expr }) | None -> None diff --git a/tests/service/ServiceUntypedParseTests.fs b/tests/service/ServiceUntypedParseTests.fs index e53d275d59b..3c87e8f7973 100644 --- a/tests/service/ServiceUntypedParseTests.fs +++ b/tests/service/ServiceUntypedParseTests.fs @@ -671,6 +671,37 @@ add2 x y let parseFileResults, _ = getParseAndCheckResults source Assert.True(parseFileResults.IsPosContainedInApplication (mkPos 3 6), "Pos should be in application") + [] + let ``IsPosContainedInApplication - inside computation expression - no``() = + let source = """ +async { + sqrt +} +""" + let parseFileResults, _ = getParseAndCheckResults source + Assert.False(parseFileResults.IsPosContainedInApplication (mkPos 2 5), "Pos should not be in application") + + [] + let ``TryRangeOfFunctionOrMethodBeingApplied - inside CE return - no``() = + let source = """ +async { + return sqrt +} +""" + let parseFileResults, _ = getParseAndCheckResults source + Assert.False(parseFileResults.IsPosContainedInApplication (mkPos 2 5), "Pos should not be in application") + + [] + let ``TryRangeOfFunctionOrMethodBeingApplied - inside CE - yes``() = + let source = """ +let myAdd x y = x + y +async { + return myAdd 1 +} + """ + let parseFileResults, _ = getParseAndCheckResults source + Assert.False(parseFileResults.IsPosContainedInApplication (mkPos 3 18), "Pos should not be in application") + [] let ``TryRangeOfFunctionOrMethodBeingApplied - no application``() = let source = """ @@ -787,6 +818,23 @@ add2 1 2 |> tups |> shouldEqual ((3, 17), (3, 18)) + [] + let ``TryRangeOfFunctionOrMethodBeingApplied - inside CE``() = + let source = """ +let myAdd x y = x + y +async { + return myAdd 1 +} +""" + let parseFileResults, _ = getParseAndCheckResults source + let res = parseFileResults.TryRangeOfFunctionOrMethodBeingApplied (mkPos 4 18) + match res with + | None -> Assert.Fail("Expected 'myAdd' but got nothing") + | Some range -> + range + |> tups + |> shouldEqual ((4, 11), (4, 16)) + module PipelinesAndArgs = [] let ``TryIdentOfPipelineContainingPosAndNumArgsApplied - No pipeline, no infix app``() = diff --git a/vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs b/vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs index 21b572fae50..371e0d7863a 100644 --- a/vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs +++ b/vsintegration/src/FSharp.Editor/Completion/SignatureHelp.fs @@ -226,24 +226,10 @@ type internal FSharpSignatureHelpProvider documentationBuilder: IDocumentationBuilder, sourceText: SourceText, caretPosition: int, + adjustedColumnInSource: int, filePath: string ) = asyncMaybe { - // Backtrack to find a non-whitespace character to get curried arg infos (if present) and a symbol to inspect. - let adjustedColumnInSource = - let rec loop s c = - if String.IsNullOrWhiteSpace(s.ToString()) then - loop (sourceText.GetSubText(c - 1)) (c - 1) - else - c - let startText = - if caretPosition = sourceText.Length then - sourceText.GetSubText(caretPosition) - else - sourceText.GetSubText(TextSpan(caretPosition, 1)) - - loop startText caretPosition - let textLine = sourceText.Lines.GetLineFromPosition(adjustedColumnInSource) let textLinePos = sourceText.Lines.GetLinePosition(adjustedColumnInSource) let pos = mkPos (Line.fromZ textLinePos.Line) textLinePos.Character @@ -465,19 +451,28 @@ type internal FSharpSignatureHelpProvider let perfOptions = document.FSharpOptions.LanguageServicePerformance let! parseResults, _, checkFileResults = checker.ParseAndCheckDocument(filePath, textVersionHash, sourceText, options, perfOptions, userOpName = userOpName) - match parseResults.FindNoteworthyParamInfoLocations(Pos.fromZ caretLinePos.Line caretLineColumn) with - | Some paramInfoLocations -> - return! - FSharpSignatureHelpProvider.ProvideMethodsAsyncAux( - caretLinePos, - caretLineColumn, - paramInfoLocations, - checkFileResults, - documentationBuilder, - sourceText, - caretPosition, - triggerTypedChar) - | None -> + + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition + + let adjustedColumnString = sourceText.GetSubText(TextSpan(adjustedColumnInSource, 1)).ToString() + + match triggerTypedChar with + // Generally ' ' indicates a function application, but it's also used commonly after a comma in a method call. + // This means that the adjusted position relative to the caret could be a ',' or a ')' or '>', + // which would mean we're already inside of a method call - not a function argument. So we bail if that's the case. + | Some ' ' when adjustedColumnString <> "," && adjustedColumnString <> ")" && adjustedColumnString <> ">" -> return! FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -487,7 +482,20 @@ type internal FSharpSignatureHelpProvider documentationBuilder, sourceText, caretPosition, + adjustedColumnInSource, filePath) + | _ -> + let! paramInfoLocations = parseResults.FindNoteworthyParamInfoLocations(Pos.fromZ caretLinePos.Line caretLineColumn) + return! + FSharpSignatureHelpProvider.ProvideMethodsAsyncAux( + caretLinePos, + caretLineColumn, + paramInfoLocations, + checkFileResults, + documentationBuilder, + sourceText, + caretPosition, + triggerTypedChar) } interface IFSharpSignatureHelpProvider with diff --git a/vsintegration/tests/UnitTests/SignatureHelpProviderTests.fs b/vsintegration/tests/UnitTests/SignatureHelpProviderTests.fs index 5737acdcf9c..ed365c6142d 100644 --- a/vsintegration/tests/UnitTests/SignatureHelpProviderTests.fs +++ b/vsintegration/tests/UnitTests/SignatureHelpProviderTests.fs @@ -24,15 +24,22 @@ module Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.SignatureHelpProvider open System open System.IO open System.Text + open NUnit.Framework + open Microsoft.CodeAnalysis.Text + open VisualFSharp.UnitTests.Roslyn + open Microsoft.VisualStudio.FSharp.Editor + open UnitTests.TestLib.LanguageService -open FSharp.Compiler + open FSharp.Compiler.Text open FSharp.Compiler.SourceCodeServices + open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Text let filePath = "C:\\test.fs" @@ -273,6 +280,21 @@ sqrt if x.IsNone then Assert.Fail("Could not parse and check document.") x.Value + + + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( @@ -283,6 +305,7 @@ sqrt DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -315,6 +338,20 @@ add2 1 if x.IsNone then Assert.Fail("Could not parse and check document.") x.Value + + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( @@ -325,6 +362,7 @@ add2 1 DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -358,7 +396,21 @@ M.f if x.IsNone then Assert.Fail("Could not parse and check document.") x.Value + + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + loop startText caretPosition + let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -368,6 +420,7 @@ M.f DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -400,6 +453,20 @@ let ``function application in single pipeline with no additional args``() = Assert.Fail("Could not parse and check document.") x.Value + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition + let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -409,6 +476,7 @@ let ``function application in single pipeline with no additional args``() = DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -437,6 +505,20 @@ let ``function application in single pipeline with an additional argument``() = Assert.Fail("Could not parse and check document.") x.Value + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition + let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -446,6 +528,7 @@ let ``function application in single pipeline with an additional argument``() = DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -480,6 +563,20 @@ let ``function application in middle of pipeline with an additional argument``() Assert.Fail("Could not parse and check document.") x.Value + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition + let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -489,6 +586,7 @@ let ``function application in middle of pipeline with an additional argument``() DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously @@ -522,6 +620,20 @@ derp Assert.Fail("Could not parse and check document.") x.Value + let adjustedColumnInSource = + let rec loop s c = + if String.IsNullOrWhiteSpace(s.ToString()) then + loop (sourceText.GetSubText(c - 1)) (c - 1) + else + c + let startText = + if caretPosition = sourceText.Length then + sourceText.GetSubText(caretPosition) + else + sourceText.GetSubText(TextSpan(caretPosition, 1)) + + loop startText caretPosition + let sigHelp = FSharpSignatureHelpProvider.ProvideParametersAsyncAux( parseResults, @@ -531,6 +643,7 @@ derp DefaultDocumentationProvider, sourceText, caretPosition, + adjustedColumnInSource, filePath) |> Async.RunSynchronously