Completion: empty record fixes (#12872)

Co-authored-by: NVlad Zarytovskii <vzaritovsky@hotmail.com>

src/fsharp/NameResolution.fs

@@ -4484,22 +4484,31 @@ let rec ResolvePartialLongIdentInModuleOrNamespaceForRecordFields (ncenv: NameRe
             | _ -> []
         )
 
+let getRecordFieldsInScope nenv =
+    nenv.eFieldLabels
+   |> Seq.collect (fun (KeyValue(_, v)) -> v)
+   |> Seq.map (fun fref ->
+        let typeInsts = fref.TyconRef.TyparsNoRange |> List.map mkTyparTy
+        Item.RecdField(RecdFieldInfo(typeInsts, fref)))
+   |> List.ofSeq
+
 /// allowObsolete - specifies whether we should return obsolete types & modules
 ///   as (no other obsolete items are returned)
-let rec ResolvePartialLongIdentToClassOrRecdFields (ncenv: NameResolver) (nenv: NameResolutionEnv) m ad plid (allowObsolete: bool) =
-    ResolvePartialLongIdentToClassOrRecdFieldsImpl ncenv nenv OpenQualified m ad plid allowObsolete
+let rec ResolvePartialLongIdentToClassOrRecdFields (ncenv: NameResolver) (nenv: NameResolutionEnv) m ad plid (allowObsolete: bool) (fieldsOnly: bool) =
+    ResolvePartialLongIdentToClassOrRecdFieldsImpl ncenv nenv OpenQualified m ad plid allowObsolete fieldsOnly
 
-and ResolvePartialLongIdentToClassOrRecdFieldsImpl (ncenv: NameResolver) (nenv: NameResolutionEnv) fullyQualified m ad plid allowObsolete =
+and ResolvePartialLongIdentToClassOrRecdFieldsImpl (ncenv: NameResolver) (nenv: NameResolutionEnv) fullyQualified m ad plid allowObsolete fieldsOnly =
     let g = ncenv.g
 
     match  plid with
     |  id :: plid when id = "global" -> // this is deliberately not the mangled name
        // dive deeper
-       ResolvePartialLongIdentToClassOrRecdFieldsImpl ncenv nenv FullyQualified m ad plid allowObsolete
+       ResolvePartialLongIdentToClassOrRecdFieldsImpl ncenv nenv FullyQualified m ad plid allowObsolete fieldsOnly
     |  [] ->
 
         // empty plid - return namespaces\modules\record types\accessible fields
 
+       if fieldsOnly then getRecordFieldsInScope nenv else
 
        let mods =
            let moduleOrNamespaceRefs =
@@ -4528,12 +4537,7 @@ and ResolvePartialLongIdentToClassOrRecdFieldsImpl (ncenv: NameResolver) (nenv:
            |> Seq.toList
 
        let recdFields =
-           nenv.eFieldLabels
-           |> Seq.collect (fun (KeyValue(_, v)) -> v)
-           |> Seq.map (fun fref ->
-                let typeInsts = fref.TyconRef.TyparsNoRange |> List.map mkTyparTy
-                Item.RecdField(RecdFieldInfo(typeInsts, fref)))
-           |> List.ofSeq
+           getRecordFieldsInScope nenv
 
        mods @ recdTyCons @ recdFields
 
@@ -4549,7 +4553,7 @@ and ResolvePartialLongIdentToClassOrRecdFieldsImpl (ncenv: NameResolver) (nenv:
 
         let qualifiedFields =
             match rest with
-            | [] ->
+            | [] when not fieldsOnly ->
                 // get record types accessible in given nenv
                 let tycons = LookupTypeNameInEnvNoArity OpenQualified id nenv
                 tycons

src/fsharp/NameResolution.fsi

@@ -557,8 +557,10 @@ val internal ResolveField                      : TcResultsSink -> NameResolver -
 /// Resolve a long identifier occurring in an expression position
 val internal ResolveExprLongIdent              : TcResultsSink -> NameResolver -> range -> AccessorDomain -> NameResolutionEnv -> TypeNameResolutionInfo -> Ident list -> ResultOrException<EnclosingTypeInst * Item * Ident list>
 
+val internal getRecordFieldsInScope: NameResolutionEnv -> Item list
+
 /// Resolve a (possibly incomplete) long identifier to a loist of possible class or record fields
-val internal ResolvePartialLongIdentToClassOrRecdFields: NameResolver -> NameResolutionEnv -> range -> AccessorDomain -> string list -> bool -> Item list
+val internal ResolvePartialLongIdentToClassOrRecdFields: NameResolver -> NameResolutionEnv -> range -> AccessorDomain -> string list -> bool -> bool -> Item list
 
 /// Return the fields for the given class or record
 val internal ResolveRecordOrClassFieldsOfType  : NameResolver -> range -> AccessorDomain -> TType -> bool -> Item list

src/fsharp/service/FSharpCheckerResults.fs

@@ -638,9 +638,9 @@ type internal TypeCheckInfo
         GetEnvironmentLookupResolutions(nenv, ad, m, plid, filterCtors, showObsolete)
 
     /// Find record fields in the best naming environment.
-    let GetClassOrRecordFieldsEnvironmentLookupResolutions(cursorPos, plid) =
+    let GetClassOrRecordFieldsEnvironmentLookupResolutions(cursorPos, plid, fieldsOnly) =
         let (nenv, ad),m = GetBestEnvForPos cursorPos
-        let items = ResolvePartialLongIdentToClassOrRecdFields ncenv nenv m ad plid false
+        let items = ResolvePartialLongIdentToClassOrRecdFields ncenv nenv m ad plid false fieldsOnly
         let items = items |> List.map ItemWithNoInst
         let items = items |> RemoveDuplicateItems g
         let items = items |> RemoveExplicitlySuppressed g
@@ -913,6 +913,21 @@ type internal TypeCheckInfo
     let toCompletionItems (items: ItemWithInst list, denv: DisplayEnv, m: range ) =
         items |> List.map DefaultCompletionItem, denv, m
 
+    /// Find record fields in the best naming environment.
+    let GetEnvironmentLookupResolutionsIncludingRecordFieldsAtPosition cursorPos plid envItems =
+        // An empty record expression may be completed into something like these:
+        // { XXX = ... }
+        // { xxx with XXX ... }
+        // Provide both expression items in scope and available record fields.
+        let (nenv, _), m = GetBestEnvForPos cursorPos
+
+        let fieldItems, _, _ = GetClassOrRecordFieldsEnvironmentLookupResolutions(cursorPos, plid, true)
+        let fieldCompletionItems, _, _ as fieldsResult = (fieldItems, nenv.DisplayEnv, m) |> toCompletionItems
+
+        match envItems with
+        | Some(items, denv, m) -> Some(fieldCompletionItems @ items, denv, m)
+        | _ -> Some(fieldsResult)
+
     /// Get the auto-complete items at a particular location.
     let GetDeclItemsForNamesAtPosition(parseResultsOpt: FSharpParseFileResults option, origLongIdentOpt: string list option,
                                        residueOpt:string option, lastDotPos: int option, line:int, lineStr:string, colAtEndOfNamesAndResidue, filterCtors, resolveOverloads,
@@ -963,19 +978,30 @@ type internal TypeCheckInfo
                 |> Option.map toCompletionItems
 
             // Completion at ' { XXX = ... } "
-            | Some(CompletionContext.RecordField(RecordContext.New(plid, _))) ->
-                // { x. } can be either record construction or computation expression. Try to get all visible record fields first
-                match GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, plid) |> toCompletionItems with
-                | [],_,_ ->
-                    // no record fields found, return completion list as if we were outside any computation expression
-                    GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors,resolveOverloads, false, fun() -> [])
-                | result -> Some(result)
+            | Some(CompletionContext.RecordField(RecordContext.New((plid, _), isFirstField))) ->
+                if isFirstField then
+                    let cursorPos = mkPos line loc
+                    let envItems = GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors,resolveOverloads, false, fun () -> [])
+                    GetEnvironmentLookupResolutionsIncludingRecordFieldsAtPosition cursorPos plid envItems
+                else
+                    // { x. } can be either record construction or computation expression. Try to get all visible record fields first
+                    match GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, plid, false) |> toCompletionItems with
+                    | [],_,_ ->
+                        // no record fields found, return completion list as if we were outside any computation expression
+                        GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors,resolveOverloads, false, fun() -> [])
+                    | result -> Some(result)
+
+            // Completion at '{ ... }'
+            | Some(CompletionContext.RecordField RecordContext.Empty) ->
+                let cursorPos = mkPos line loc
+                let envItems = GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors,resolveOverloads, false, fun () -> [])
+                GetEnvironmentLookupResolutionsIncludingRecordFieldsAtPosition cursorPos [] envItems 
 
             // Completion at ' { XXX = ... with ... } "
             | Some(CompletionContext.RecordField(RecordContext.CopyOnUpdate(r, (plid, _)))) ->
                 match GetRecdFieldsForExpr(r) with
                 | None ->
-                    Some (GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, plid))
+                    Some (GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, plid, false))
                     |> Option.map toCompletionItems
                 | Some (items, denv, m) ->
                     Some (List.map ItemWithNoInst items, denv, m)
@@ -983,7 +1009,7 @@ type internal TypeCheckInfo
 
             // Completion at ' { XXX = ... with ... } "
             | Some(CompletionContext.RecordField(RecordContext.Constructor(typeName))) ->
-                Some(GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, [typeName]))
+                Some(GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, [typeName], false))
                 |> Option.map toCompletionItems
 
             // No completion at '...: string'

src/fsharp/service/ServiceParsedInputOps.fs

@@ -43,7 +43,8 @@ type InheritanceContext =
 type RecordContext =
     | CopyOnUpdate of range: range * path: CompletionPath
     | Constructor of typeName: string
-    | New of path: CompletionPath
+    | Empty
+    | New of path: CompletionPath * isFirstField: bool
     | Declaration of isInIdentifier: bool
 
 [<RequireQualifiedAccess>]
@@ -956,7 +957,8 @@ module ParsedInput =
                                     Some (CompletionContext.ParameterList args)
                                 | _ -> 
                                     defaultTraverse expr
-
+                            | SynExpr.Record(None, None, [], _) ->
+                                Some(CompletionContext.RecordField RecordContext.Empty)
                             // Unchecked.defaultof<str$>
                             | SynExpr.TypeApp (typeArgsRange = range) when rangeContainsPos range pos ->
                                 Some CompletionContext.PatternType
@@ -968,7 +970,22 @@ module ParsedInput =
                             match path with
                             | SyntaxNode.SynExpr _ :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(typeInfo=SynComponentInfo(longId=[id]))) :: _ ->  
                                 RecordContext.Constructor(id.idText)
-                            | _ -> RecordContext.New completionPath
+
+                            | SyntaxNode.SynExpr(SynExpr.Record(None, _, fields, _)) :: _ ->
+                                let isFirstField = 
+                                    match field, fields with
+                                    | Some contextLid, SynExprRecordField(fieldName = lid, _) :: _ -> contextLid.Range = lid.Range
+                                    | _ -> false
+
+                                RecordContext.New(completionPath, isFirstField)
+
+                            // Unfinished `{ xxx }` expression considered a record field by the tree walker. 
+                            | SyntaxNode.SynExpr(SynExpr.ComputationExpr _) :: _ ->
+                                RecordContext.New(completionPath, true)
+
+                            | _ ->
+                                RecordContext.New(completionPath, false)
+
                         match field with
                         | Some field -> 
                             match parseLid field with

src/fsharp/service/ServiceParsedInputOps.fsi

@@ -18,7 +18,8 @@ type public InheritanceContext =
 type public RecordContext =
     | CopyOnUpdate of range: range * path: CompletionPath
     | Constructor of typeName: string
-    | New of path: CompletionPath
+    | Empty
+    | New of path: CompletionPath * isFirstField: bool
     | Declaration of isInIdentifier: bool
 
 [<RequireQualifiedAccess>]

tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.expected

@@ -3511,11 +3511,14 @@ FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate: System.Tuple`2[Micros
 FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate: System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]] path
 FSharp.Compiler.EditorServices.RecordContext+Declaration: Boolean get_isInIdentifier()
 FSharp.Compiler.EditorServices.RecordContext+Declaration: Boolean isInIdentifier
+FSharp.Compiler.EditorServices.RecordContext+New: Boolean get_isFirstField()
+FSharp.Compiler.EditorServices.RecordContext+New: Boolean isFirstField
 FSharp.Compiler.EditorServices.RecordContext+New: System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]] get_path()
 FSharp.Compiler.EditorServices.RecordContext+New: System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]] path
 FSharp.Compiler.EditorServices.RecordContext+Tags: Int32 Constructor
 FSharp.Compiler.EditorServices.RecordContext+Tags: Int32 CopyOnUpdate
 FSharp.Compiler.EditorServices.RecordContext+Tags: Int32 Declaration
+FSharp.Compiler.EditorServices.RecordContext+Tags: Int32 Empty
 FSharp.Compiler.EditorServices.RecordContext+Tags: Int32 New
 FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(FSharp.Compiler.EditorServices.RecordContext)
 FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(System.Object)
@@ -3523,15 +3526,19 @@ FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(System.Object, Syst
 FSharp.Compiler.EditorServices.RecordContext: Boolean IsConstructor
 FSharp.Compiler.EditorServices.RecordContext: Boolean IsCopyOnUpdate
 FSharp.Compiler.EditorServices.RecordContext: Boolean IsDeclaration
+FSharp.Compiler.EditorServices.RecordContext: Boolean IsEmpty
 FSharp.Compiler.EditorServices.RecordContext: Boolean IsNew
 FSharp.Compiler.EditorServices.RecordContext: Boolean get_IsConstructor()
 FSharp.Compiler.EditorServices.RecordContext: Boolean get_IsCopyOnUpdate()
 FSharp.Compiler.EditorServices.RecordContext: Boolean get_IsDeclaration()
+FSharp.Compiler.EditorServices.RecordContext: Boolean get_IsEmpty()
 FSharp.Compiler.EditorServices.RecordContext: Boolean get_IsNew()
+FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext Empty
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext NewConstructor(System.String)
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext NewCopyOnUpdate(FSharp.Compiler.Text.Range, System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]])
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext NewDeclaration(Boolean)
-FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext NewNew(System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]])
+FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext NewNew(System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]], Boolean)
+FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext get_Empty()
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+Constructor
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate
 FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+Declaration

tests/service/CompletionTests.fs

 module FSharp.Compiler.Service.Tests.CompletionTests
 
 open FSharp.Compiler.EditorServices
-open FsUnit
 open NUnit.Framework
 
+let getCompletionInfo lineText (line, column) source =
+    let parseResults, checkResults = getParseAndCheckResults source
+    let plid = QuickParse.GetPartialLongNameEx(lineText, column)
+    checkResults.GetDeclarationListInfo(Some parseResults, line, lineText, plid)
+
+let getCompletionItemNames (completionInfo: DeclarationListInfo) =
+    completionInfo.Items |> Array.map (fun item -> item.Name)
+
+let assertHasItemWithNames names (completionInfo: DeclarationListInfo) =
+    let itemNames = getCompletionItemNames completionInfo |> set
+
+    for name in names do
+        Assert.That(Set.contains name itemNames, name)
+
+
 [<Test>]
 let ``Expr - record - field 01 - anon module`` () =
-    let parseResults, checkResults = getParseAndCheckResults """
-type Record = { Field: int}
+    let info = getCompletionInfo "{ Fi }" (4, 3)  """
+type Record = { Field: int }
 
 { Fi }
 """
-    let lineText = "{ Fi }"
-    let plid = QuickParse.GetPartialLongNameEx(lineText, 3)
-    let info = checkResults.GetDeclarationListInfo(Some parseResults, 4, lineText, plid)
+    assertHasItemWithNames ["Field"] info
 
-    info.Items |> Array.exists (fun item -> item.Name = "Field") |> shouldEqual true
+[<Test>]
+let ``Expr - record - field 02 - anon module`` () =
+    let info = getCompletionInfo "{ Fi }" (6, 3)  """
+type Record = { Field: int }
+
+let record = { Field = 1 }
+
+{ Fi }
+"""
+    assertHasItemWithNames ["Field"] info
+
+[<Test>]
+let ``Expr - record - empty 01`` () =
+    let info = getCompletionInfo "{  }" (4, 2) """
+type Record = { Field: int }
+
+{  }
+"""
+    assertHasItemWithNames ["Field"] info
+
+[<Test>]
+let ``Expr - record - empty 02`` () =
+    let info = getCompletionInfo "{  }" (6, 2) """
+type Record = { Field: int }
+
+let record = { Field = 1 }
+
+{  }
+"""
+    assertHasItemWithNames ["Field"; "record"] info

vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs

@@ -786,15 +786,15 @@ for i in 0..a."]
             ]
         let useCases = 
             [
-                "let _ = (* MARKER*){X", "(* MARKER*){X", [], ["XX"]
+                "let _ = (* MARKER*){X  }", "(* MARKER*){X", [], ["XX"]
                 "let _ = {(* MARKER*)Mod. = 1; O", "(* MARKER*)Mod.", ["XX"; "YY"], ["System"]
                 "let _ = {(* MARKER*)Mod.Rec. ", "(* MARKER*)Mod.Rec.", ["XX"; "YY"], ["System"]
+                "let _ = (* MARKER*){Mod.XX = 1;  }", "(* MARKER*){Mod.XX = 1; ", ["Mod"], ["XX"; "abs"]
             ]
 
         for (code, marker, should, shouldnot) in useCases do
             let code = prologue @ [code]
-            let shouldnot = shouldnot @ ["abs"]
-            AssertCtrlSpaceCompleteContains code marker should ["abs"]
+            AssertCtrlSpaceCompleteContains code marker should shouldnot
     
     [<Test>]
     [<Category("Records")>]