From 1ac6d1361264df1cc314ba76c2eb57f817114025 Mon Sep 17 00:00:00 2001 From: Eugene Auduchinok Date: Fri, 29 Apr 2022 19:14:37 +0300 Subject: [PATCH] Completion: empty record fixes (#12872) Co-authored-by: Vlad Zarytovskii --- src/fsharp/NameResolution.fs | 26 +++++---- src/fsharp/NameResolution.fsi | 4 +- src/fsharp/service/FSharpCheckerResults.fs | 48 ++++++++++++---- src/fsharp/service/ServiceParsedInputOps.fs | 23 +++++++- src/fsharp/service/ServiceParsedInputOps.fsi | 3 +- ...erService.SurfaceArea.netstandard.expected | 9 ++- tests/service/CompletionTests.fs | 55 ++++++++++++++++--- .../Tests.LanguageService.Completion.fs | 6 +- 8 files changed, 136 insertions(+), 38 deletions(-) diff --git a/src/fsharp/NameResolution.fs b/src/fsharp/NameResolution.fs index f95618677..c936bd14d 100644 --- a/src/fsharp/NameResolution.fs +++ b/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 diff --git a/src/fsharp/NameResolution.fsi b/src/fsharp/NameResolution.fsi index 0b96de1fa..3386fe231 100755 --- a/src/fsharp/NameResolution.fsi +++ b/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 +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 diff --git a/src/fsharp/service/FSharpCheckerResults.fs b/src/fsharp/service/FSharpCheckerResults.fs index faa9f8fb7..523fe359f 100644 --- a/src/fsharp/service/FSharpCheckerResults.fs +++ b/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' diff --git a/src/fsharp/service/ServiceParsedInputOps.fs b/src/fsharp/service/ServiceParsedInputOps.fs index 2dfc2ff08..8e6982d9f 100644 --- a/src/fsharp/service/ServiceParsedInputOps.fs +++ b/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 [] @@ -956,7 +957,8 @@ module ParsedInput = Some (CompletionContext.ParameterList args) | _ -> defaultTraverse expr - + | SynExpr.Record(None, None, [], _) -> + Some(CompletionContext.RecordField RecordContext.Empty) // Unchecked.defaultof | 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 diff --git a/src/fsharp/service/ServiceParsedInputOps.fsi b/src/fsharp/service/ServiceParsedInputOps.fsi index 0aa0bbcc5..a748d0957 100644 --- a/src/fsharp/service/ServiceParsedInputOps.fsi +++ b/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 [] diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.expected b/tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.expected index 142e6a81c..e51aefdb7 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.expected +++ b/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 diff --git a/tests/service/CompletionTests.fs b/tests/service/CompletionTests.fs index 432633dc1..4409f7373 100644 --- a/tests/service/CompletionTests.fs +++ b/tests/service/CompletionTests.fs @@ -1,18 +1,59 @@ 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) + + [] 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 +[] +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 + +[] +let ``Expr - record - empty 01`` () = + let info = getCompletionInfo "{ }" (4, 2) """ +type Record = { Field: int } + +{ } +""" + assertHasItemWithNames ["Field"] info + +[] +let ``Expr - record - empty 02`` () = + let info = getCompletionInfo "{ }" (6, 2) """ +type Record = { Field: int } + +let record = { Field = 1 } + +{ } +""" + assertHasItemWithNames ["Field"; "record"] info diff --git a/vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs b/vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs index 83f3a8748..b79ef889d 100644 --- a/vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs +++ b/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 [] [] -- GitLab