Cherry pick in 16.9 - Fix signature help in CEs and use better heuristics (#10835)

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

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")
 
+    [<Test>]
+    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")
+
+    [<Test>]
+    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")
+
+    [<Test>]
+    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")
+
     [<Test>]
     let ``TryRangeOfFunctionOrMethodBeingApplied - no application``() =
         let source = """
@@ -787,6 +818,23 @@ add2 1 2
             |> tups
             |> shouldEqual ((3, 17), (3, 18))
 
+    [<Test>]
+    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 =
     [<Test>]
     let ``TryIdentOfPipelineContainingPosAndNumArgsApplied - No pipeline, no infix app``() =

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