More improve completion after method/property override (#18341)

Author: ijklam

docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@@ -26,6 +26,8 @@
 * Nullness warnings are issued for signature<>implementation conformance ([PR #18186](https://github.com/dotnet/fsharp/pull/18186))
 * Symbols: Add FSharpAssembly.IsFSharp ([PR #18290](https://github.com/dotnet/fsharp/pull/18290))
 * Type parameter constraint `null` in generic code will now automatically imply `not struct` ([Issue #18320](https://github.com/dotnet/fsharp/issues/18320), [PR #18323](https://github.com/dotnet/fsharp/pull/18323))
+* Add a switch to determine whether to generate a default implementation body for overridden method when completing. [PR #18341](https://github.com/dotnet/fsharp/pull/18341)
+
 
 ### Changed
 
@@ -35,6 +37,7 @@
 * Added nullability annotations to `.Using` builder method for `async`, `task` and compiler-internal builders ([PR #18292](https://github.com/dotnet/fsharp/pull/18292))
 * Warn when `unit` is passed to an `obj`-typed argument  ([PR #18330](https://github.com/dotnet/fsharp/pull/18330))
 * Warning for "useless null handling" works with piped syntax constructs now ([PR #18331](https://github.com/dotnet/fsharp/pull/18331))
+* Make indent in generated overridden member code depend on the context, not fix to 4. ([PR #18341](https://github.com/dotnet/fsharp/pull/18341))
 * Adjust caller info attribute error message range ([PR #18388](https://github.com/dotnet/fsharp/pull/18388))
 
 ### Breaking Changes

docs/release-notes/.VisualStudio/17.14.md

@@ -0,0 +1,9 @@
+### Fixed
+
+### Added
+* Add a switch to determine whether to generate a default implementation body for overridden method when completing. [PR #18341](https://github.com/dotnet/fsharp/pull/18341)
+
+### Changed
+* Make indent in generated overridden member code depend on the context, not fix to 4. ([PR #18341](https://github.com/dotnet/fsharp/pull/18341))
+
+### Breaking Changes

src/Compiler/Service/FSharpCheckerResults.fs

@@ -1063,7 +1063,7 @@ type internal TypeCheckInfo
         |> Option.defaultValue completions
 
     /// Gets all methods that a type can override, but has not yet done so.
-    let GetOverridableMethods pos ctx (typeNameRange: range) spacesBeforeOverrideKeyword hasThis isStatic =
+    let GetOverridableMethods pos ctx (typeNameRange: range) newlineIndentCount hasThis isStatic genBodyForOverriddenMeth =
         let checkImplementedSlotDeclareType ty slots =
             slots
             |> Option.map (List.exists (fun (TSlotSig(declaringType = ty2)) -> typeEquiv g ty ty2))
@@ -1111,17 +1111,20 @@ type internal TypeCheckInfo
         let (nenv, ad), m = GetBestEnvForPos pos
         let denv = nenv.DisplayEnv
 
+        /// Check if the method is abstract, return "raise (NotImplementedException())" if it is abstract, otherwise return the given body
         let checkMethAbstractAndGetImplementBody (meth: MethInfo) implementBody =
-            if meth.IsAbstract then
+            if not genBodyForOverriddenMeth then
+                String.Empty
+            elif meth.IsAbstract then
                 if nenv.DisplayEnv.openTopPathsSorted.Force() |> List.contains [ "System" ] then
                     "raise (NotImplementedException())"
                 else
                     "raise (System.NotImplementedException())"
             else
                 implementBody
 
-        let newlineIndent =
-            Environment.NewLine + String.make (spacesBeforeOverrideKeyword + 4) ' '
+        let newlineIndentCount = max 1 newlineIndentCount
+        let newlineIndent = Environment.NewLine + String.make newlineIndentCount ' '
 
         let getOverridableMethods superTy (overriddenMethods: MethInfo list) overriddenProperties =
             // Do not check a method with same name twice
@@ -1197,12 +1200,6 @@ type internal TypeCheckInfo
 
                     let this = if hasThis || prop.IsStatic then String.Empty else "this."
 
-                    let getterWithBody =
-                        if String.IsNullOrWhiteSpace getterWithBody then
-                            String.Empty
-                        else
-                            getterWithBody + newlineIndent
-
                     let name = $"{prop.DisplayName} with {getter}{keywordAnd}{setter}"
 
                     let textInCode =
@@ -1211,6 +1208,10 @@ type internal TypeCheckInfo
                         + newlineIndent
                         + "with "
                         + getterWithBody
+                        + (if String.IsNullOrEmpty keywordAnd then
+                               String.Empty
+                           else
+                               newlineIndent)
                         + keywordAnd
                         + setterWithBody
 
@@ -1721,7 +1722,8 @@ type internal TypeCheckInfo
             filterCtors,
             resolveOverloads,
             completionContextAtPos: (pos * CompletionContext option) option,
-            getAllSymbols: unit -> AssemblySymbol list
+            getAllSymbols: unit -> AssemblySymbol list,
+            genBodyForOverriddenMeth
         ) : (CompletionItem list * DisplayEnv * CompletionContext option * range) option =
 
         let loc =
@@ -1957,8 +1959,22 @@ type internal TypeCheckInfo
                     getDeclaredItemsNotInRangeOpWithAllSymbols ()
                     |> Option.bind (FilterRelevantItemsBy getItem2 None IsPatternCandidate)
 
-            | Some(CompletionContext.MethodOverride(ctx, enclosingTypeNameRange, spacesBeforeOverrideKeyword, hasThis, isStatic)) ->
-                GetOverridableMethods pos ctx enclosingTypeNameRange spacesBeforeOverrideKeyword hasThis isStatic
+            | Some(CompletionContext.MethodOverride(ctx,
+                                                    enclosingTypeNameRange,
+                                                    spacesBeforeOverrideKeyword,
+                                                    hasThis,
+                                                    isStatic,
+                                                    spacesBeforeEnclosingDefinition)) ->
+                let indent = max 1 (spacesBeforeOverrideKeyword - spacesBeforeEnclosingDefinition)
+
+                GetOverridableMethods
+                    pos
+                    ctx
+                    enclosingTypeNameRange
+                    (spacesBeforeOverrideKeyword + indent)
+                    hasThis
+                    isStatic
+                    genBodyForOverriddenMeth
 
             // Other completions
             | cc ->
@@ -2025,7 +2041,16 @@ type internal TypeCheckInfo
         scope.IsRelativeNameResolvable(cursorPos, plid, symbol.Item)
 
     /// Get the auto-complete items at a location
-    member _.GetDeclarations(parseResultsOpt, line, lineStr, partialName, completionContextAtPos, getAllEntities) =
+    member _.GetDeclarations
+        (
+            parseResultsOpt,
+            line,
+            lineStr,
+            partialName,
+            completionContextAtPos,
+            getAllEntities,
+            genBodyForOverriddenMeth
+        ) =
         let isSigFile = SourceFileImpl.IsSignatureFile mainInputFileName
 
         DiagnosticsScope.Protect
@@ -2044,7 +2069,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.Yes,
                         completionContextAtPos,
-                        getAllEntities
+                        getAllEntities,
+                        genBodyForOverriddenMeth
                     )
 
                 match declItemsOpt with
@@ -2085,7 +2111,7 @@ type internal TypeCheckInfo
                 DeclarationListInfo.Error msg)
 
     /// Get the symbols for auto-complete items at a location
-    member _.GetDeclarationListSymbols(parseResultsOpt, line, lineStr, partialName, getAllEntities) =
+    member _.GetDeclarationListSymbols(parseResultsOpt, line, lineStr, partialName, getAllEntities, genBodyForOverriddenMeth) =
         let isSigFile = SourceFileImpl.IsSignatureFile mainInputFileName
 
         DiagnosticsScope.Protect
@@ -2104,7 +2130,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.Yes,
                         None,
-                        getAllEntities
+                        getAllEntities,
+                        genBodyForOverriddenMeth
                     )
 
                 match declItemsOpt with
@@ -2285,7 +2312,8 @@ type internal TypeCheckInfo
                             ResolveTypeNamesToCtors,
                             ResolveOverloads.Yes,
                             None,
-                            (fun () -> [])
+                            (fun () -> []),
+                            false
                         )
 
                     match declItemsOpt with
@@ -2347,7 +2375,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.No,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -2393,7 +2422,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.No,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -2434,7 +2464,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.No,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -2470,7 +2501,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.Yes,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -2618,7 +2650,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.Yes,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -2647,7 +2680,8 @@ type internal TypeCheckInfo
                         ResolveTypeNamesToCtors,
                         ResolveOverloads.Yes,
                         None,
-                        (fun () -> [])
+                        (fun () -> []),
+                        false
                     )
 
                 match declItemsOpt with
@@ -3348,20 +3382,40 @@ type FSharpCheckFileResults
         | Some(scope, _builderOpt) -> Some scope.TcImports
 
     /// Intellisense autocompletions
-    member _.GetDeclarationListInfo(parsedFileResults, line, lineText, partialName, ?getAllEntities, ?completionContextAtPos) =
+    member _.GetDeclarationListInfo
+        (
+            parsedFileResults,
+            line,
+            lineText,
+            partialName,
+            ?getAllEntities,
+            ?completionContextAtPos,
+            ?genBodyForOverriddenMeth
+        ) =
         let getAllEntities = defaultArg getAllEntities (fun () -> [])
+        let genBodyForOverriddenMeth = defaultArg genBodyForOverriddenMeth true
 
         match details with
         | None -> DeclarationListInfo.Empty
         | Some(scope, _builderOpt) ->
-            scope.GetDeclarations(parsedFileResults, line, lineText, partialName, completionContextAtPos, getAllEntities)
+            scope.GetDeclarations(
+                parsedFileResults,
+                line,
+                lineText,
+                partialName,
+                completionContextAtPos,
+                getAllEntities,
+                genBodyForOverriddenMeth
+            )
 
-    member _.GetDeclarationListSymbols(parsedFileResults, line, lineText, partialName, ?getAllEntities) =
+    member _.GetDeclarationListSymbols(parsedFileResults, line, lineText, partialName, ?getAllEntities, ?genBodyForOverriddenMeth) =
         let getAllEntities = defaultArg getAllEntities (fun () -> [])
+        let genBodyForOverriddenMeth = defaultArg genBodyForOverriddenMeth true
 
         match details with
         | None -> []
-        | Some(scope, _builderOpt) -> scope.GetDeclarationListSymbols(parsedFileResults, line, lineText, partialName, getAllEntities)
+        | Some(scope, _builderOpt) ->
+            scope.GetDeclarationListSymbols(parsedFileResults, line, lineText, partialName, getAllEntities, genBodyForOverriddenMeth)
 
     member _.GetKeywordTooltip(names: string list) =
         ToolTipText.ToolTipText

src/Compiler/Service/FSharpCheckerResults.fsi

@@ -290,13 +290,17 @@ type public FSharpCheckFileResults =
     /// <param name="completionContextAtPos">
     ///    Completion context for a particular position computed in advance.
     /// </param>
+    /// <param name="genBodyForOverriddenMeth">
+    ///    A switch to determine whether to generate a default implementation body for overridden method when completing.
+    /// </param>
     member GetDeclarationListInfo:
         parsedFileResults: FSharpParseFileResults option *
         line: int *
         lineText: string *
         partialName: PartialLongName *
         ?getAllEntities: (unit -> AssemblySymbol list) *
-        ?completionContextAtPos: (pos * CompletionContext option) ->
+        ?completionContextAtPos: (pos * CompletionContext option) *
+        ?genBodyForOverriddenMeth: bool ->
             DeclarationListInfo
 
     /// <summary>Get the items for a declaration list in FSharpSymbol format</summary>
@@ -317,12 +321,16 @@ type public FSharpCheckFileResults =
     /// <param name="getAllEntities">
     ///    Function that returns all entities from current and referenced assemblies.
     /// </param>
+    /// <param name="genBodyForOverriddenMeth">
+    ///    A switch to determine whether to generate a default implementation body for overridden method when completing.
+    /// </param>
     member GetDeclarationListSymbols:
         parsedFileResults: FSharpParseFileResults option *
         line: int *
         lineText: string *
         partialName: PartialLongName *
-        ?getAllEntities: (unit -> AssemblySymbol list) ->
+        ?getAllEntities: (unit -> AssemblySymbol list) *
+        ?genBodyForOverriddenMeth: bool ->
             FSharpSymbolUse list list
 
     /// <summary>Compute a formatted tooltip for the given keywords</summary>

src/Compiler/Service/ServiceParsedInputOps.fs

@@ -118,7 +118,8 @@ type CompletionContext =
         enclosingTypeNameRange: range *
         spacesBeforeOverrideKeyword: int *
         hasThis: bool *
-        isStatic: bool
+        isStatic: bool *
+        spacesBeforeEnclosingDefinition: int
 
 type ShortIdent = string
 
@@ -1543,7 +1544,8 @@ module ParsedInput =
 
                     let overrideContext path (mOverride: range) hasThis isStatic isMember =
                         match path with
-                        | _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(typeInfo = SynComponentInfo(longId = [ enclosingType ]))) :: _ when
+                        | _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(
+                            typeInfo = SynComponentInfo(longId = [ enclosingType ]); trivia = { LeadingKeyword = keyword })) :: _ when
                             not isMember
                             ->
                             Some(
@@ -1552,12 +1554,13 @@ module ParsedInput =
                                     enclosingType.idRange,
                                     mOverride.StartColumn,
                                     hasThis,
-                                    isStatic
+                                    isStatic,
+                                    keyword.Range.StartColumn
                                 )
                             )
-                        | SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty)) :: SyntaxNode.SynTypeDefn(SynTypeDefn(
+                        | SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty) as enclosingDefn) :: SyntaxNode.SynTypeDefn(SynTypeDefn(
                             typeInfo = SynComponentInfo(longId = [ enclosingType ]))) :: _
-                        | _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty)) :: SyntaxNode.SynTypeDefn(SynTypeDefn(
+                        | _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty) as enclosingDefn) :: SyntaxNode.SynTypeDefn(SynTypeDefn(
                             typeInfo = SynComponentInfo(longId = [ enclosingType ]))) :: _ ->
                             let ty =
                                 match ty with
@@ -1570,11 +1573,12 @@ module ParsedInput =
                                     enclosingType.idRange,
                                     mOverride.StartColumn,
                                     hasThis,
-                                    isStatic
+                                    isStatic,
+                                    enclosingDefn.Range.StartColumn
                                 )
                             )
-                        | SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty)) :: (SyntaxNode.SynExpr(SynExpr.ObjExpr _) as expr) :: _
-                        | _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty)) :: (SyntaxNode.SynExpr(SynExpr.ObjExpr _) as expr) :: _ ->
+                        | SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty) as enclosingDefn) :: (SyntaxNode.SynExpr(SynExpr.ObjExpr _) as expr) :: _
+                        | _ :: SyntaxNode.SynMemberDefn(SynMemberDefn.Interface(interfaceType = ty) as enclosingDefn) :: (SyntaxNode.SynExpr(SynExpr.ObjExpr _) as expr) :: _ ->
                             let ty =
                                 match ty with
                                 | SynType.App(typeName = ty) -> ty
@@ -1586,10 +1590,11 @@ module ParsedInput =
                                     ty.Range,
                                     mOverride.StartColumn,
                                     hasThis,
-                                    isStatic
+                                    isStatic,
+                                    enclosingDefn.Range.StartColumn
                                 )
                             )
-                        | SyntaxNode.SynExpr(SynExpr.ObjExpr(objType = ty)) as expr :: _ ->
+                        | SyntaxNode.SynExpr(SynExpr.ObjExpr(objType = ty; newExprRange = newExprRange)) as expr :: _ ->
                             let ty =
                                 match ty with
                                 | SynType.App(typeName = ty) -> ty
@@ -1601,7 +1606,8 @@ module ParsedInput =
                                     ty.Range,
                                     mOverride.StartColumn,
                                     hasThis,
-                                    isStatic
+                                    isStatic,
+                                    newExprRange.StartColumn
                                 )
                             )
                         | _ -> Some CompletionContext.Invalid