Skip to content

Improve record, union, enum completions #12351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KevinRansom merged 9 commits into from
Dec 19, 2021
Merged

Improve record, union, enum completions #12351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e6734db
Improve record, union, enum completions
kerams Nov 6, 2021
b4abbe7
Merge branch 'main' of https://github.com/dotnet/fsharp into autoc
kerams Nov 12, 2021
d76ff92
Tests
kerams Nov 12, 2021
8b0051b
Remove undesirable tests
kerams Nov 15, 2021
0e2b4d7
Update surface area, constrain type alias completions
kerams Dec 16, 2021
f802432
Fix surface area
kerams Dec 16, 2021
b029a0c
Address comments
kerams Dec 17, 2021
e529cc6
Update ServiceParsedInputOps.fs
kerams Dec 17, 2021
ea4e2bf
Update FSharp.CompilerService.SurfaceArea.netstandard.expected
kerams Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/fsharp/service/FSharpCheckerResults.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,9 @@ type internal TypeCheckInfo
Some(GetClassOrRecordFieldsEnvironmentLookupResolutions(mkPos line loc, [typeName]))
|> Option.map toCompletionItems

// No completion at '...: string'
| Some(CompletionContext.RecordField(RecordContext.Declaration true)) -> None

// Completion at ' SomeMethod( ... ) ' with named arguments
| Some(CompletionContext.ParameterList (endPos, fields)) ->
let results = GetNamedParametersAndSettableFields endPos
Expand Down Expand Up @@ -1022,7 +1025,13 @@ type internal TypeCheckInfo
| _ -> false), denv, m)

// Completion at '(x: ...)"
| Some CompletionContext.PatternType ->
| Some CompletionContext.PatternType
// Completion at '| Case1 of ...'
| Some CompletionContext.UnionCaseFieldsDeclaration
// Completion at 'type Long = int6...' or 'type SomeUnion = Abc...'
| Some CompletionContext.TypeAbbreviationOrSingleCaseUnion
// Completion at 'Field1: ...'
| Some(CompletionContext.RecordField(RecordContext.Declaration false)) ->
GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors, resolveOverloads, false, getAllSymbols)
|> Option.map (fun (items, denv, m) ->
items
Expand Down
51 changes: 48 additions & 3 deletions src/fsharp/service/ServiceParsedInputOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ type RecordContext =
| CopyOnUpdate of range: range * path: CompletionPath
| Constructor of typeName: string
| New of path: CompletionPath
| Declaration of isInIdentifier: bool

[<RequireQualifiedAccess>]
type CompletionContext =
Expand All @@ -69,6 +70,13 @@ type CompletionContext =
/// Completing pattern type (e.g. foo (x: |))
| PatternType

/// Completing union case fields declaration (e.g. 'A of stri|' but not 'B of tex|: string')
| UnionCaseFieldsDeclaration

/// Completing a type abbreviation (e.g. type Long = int6|)
/// or a single case union without a bar (type SomeUnion = Abc|)
| TypeAbbreviationOrSingleCaseUnion

type ShortIdent = string

type ShortIdents = ShortIdent[]
Expand Down Expand Up @@ -1022,7 +1030,8 @@ module ParsedInput =
Some CompletionContext.Invalid
| _ -> defaultTraverse synBinding

member _.VisitHashDirective (_path, _directive, range) =
member _.VisitHashDirective (_path, _directive, range) =
// No completions in a directive
if rangeContainsPos range pos then Some CompletionContext.Invalid
else None

Expand All @@ -1031,11 +1040,15 @@ module ParsedInput =
| Some lastIdent when pos.Line = lastIdent.idRange.EndLine && lastIdent.idRange.EndColumn >= 0 && pos.Column <= lineStr.Length ->
let stringBetweenModuleNameAndPos = lineStr.[lastIdent.idRange.EndColumn..pos.Column - 1]
if stringBetweenModuleNameAndPos |> Seq.forall (fun x -> x = ' ' || x = '.') then
// No completions in a top level a module or namespace identifier
Some CompletionContext.Invalid
else None
| _ -> None

member _.VisitComponentInfo(_path, SynComponentInfo(range = range)) =
member _.VisitComponentInfo(_path, SynComponentInfo(range = range)) =
// No completions in component info (unless it's within an attribute)
// /// XmlDo|
// type R = class end
if rangeContainsPos range pos then Some CompletionContext.Invalid
else None

Expand All @@ -1046,9 +1059,12 @@ module ParsedInput =

member _.VisitSimplePats (_path, pats) =
pats |> List.tryPick (fun pat ->
// No completions in an identifier or type in a pattern
match pat with
// fun x| ->
| SynSimplePat.Id(range = range)
| SynSimplePat.Typed(SynSimplePat.Id(range = range), _, _) when rangeContainsPos range pos ->
// fun (x: int|) ->
| SynSimplePat.Typed(SynSimplePat.Id(range = range), _, _) when rangeContainsPos range pos ->
Some CompletionContext.Invalid
| _ -> None)

Expand Down Expand Up @@ -1077,6 +1093,35 @@ module ParsedInput =
| SynType.LongIdent _ when rangeContainsPos ty.Range pos ->
Some CompletionContext.PatternType
| _ -> defaultTraverse ty

member _.VisitRecordDefn(_path, fields, _range) =
fields |> List.tryPick (fun (SynField (idOpt = idOpt; range = fieldRange)) ->
match idOpt with
| Some id when rangeContainsPos id.idRange pos -> Some(CompletionContext.RecordField(RecordContext.Declaration true))
| _ when rangeContainsPos fieldRange pos -> Some(CompletionContext.RecordField(RecordContext.Declaration false))
| _ -> None)

member _.VisitUnionDefn(_path, cases, _range) =
cases |> List.tryPick (fun (SynUnionCase (ident = id; caseType = caseType)) ->
if rangeContainsPos id.idRange pos then
// No completions in a union case identifier
Some CompletionContext.Invalid
else
match caseType with
| SynUnionCaseKind.Fields fieldCases ->
fieldCases |> List.tryPick (fun (SynField (idOpt = fieldIdOpt; range = fieldRange)) ->
match fieldIdOpt with
// No completions in a union case field identifier
| Some id when rangeContainsPos id.idRange pos -> Some CompletionContext.Invalid
| _ -> if rangeContainsPos fieldRange pos then Some CompletionContext.UnionCaseFieldsDeclaration else None)
| _ -> None)

member _.VisitEnumDefn(_path, _, range) =
// No completions anywhere in an enum
if rangeContainsPos range pos then Some CompletionContext.Invalid else None

member _.VisitTypeAbbrev(_path, _, range) =
if rangeContainsPos range pos then Some CompletionContext.TypeAbbreviationOrSingleCaseUnion else None
}

SyntaxTraversal.Traverse(pos, parsedInput, walker)
Expand Down
8 changes: 8 additions & 0 deletions src/fsharp/service/ServiceParsedInputOps.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type public RecordContext =
| CopyOnUpdate of range: range * path: CompletionPath
| Constructor of typeName: string
| New of path: CompletionPath
| Declaration of isInIdentifier: bool
Copy link
Contributor Author

@kerams kerams Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove the field and return Declaration only when the position is outside of the identifier, like in UnionCaseFieldsDeclaration? Or maybe I could get rid of all of this and just reuse CompletionContext.PatternType?

Copy link
Contributor Author

@kerams kerams Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more general question would be, should there be cases for different kinds of contexts even when those contexts end up with no completions? Reusing CompletionContext.Invalid would yield the same result.

Copy link
Contributor Author

@kerams kerams Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I please get an answer here? This is the only thing that's holding up the PR as far as I am concerned.

Copy link
Contributor

@dsyme dsyme Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong intuition about this. Could you get the PR green and then we can do a final end-to-end review? It looks very thorough and a great contribution. thanks

Copy link
Contributor

@dsyme dsyme Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, if you get it green you can leave it as you have it. It's enough. Add /// comments to describe any inconsistency between the cases in this union type


[<RequireQualifiedAccess>]
type public CompletionContext =
Expand All @@ -44,6 +45,13 @@ type public CompletionContext =
/// Completing pattern type (e.g. foo (x: |))
| PatternType

/// Completing union case fields declaration (e.g. 'A of stri|' but not 'B of tex|: string')
| UnionCaseFieldsDeclaration

/// Completing a type abbreviation (e.g. type Long = int6|)
/// or a single case union without a bar (type SomeUnion = Abc|)
| TypeAbbreviationOrSingleCaseUnion

type public ModuleKind =
{ IsAutoOpen: bool
HasModuleSuffix: bool }
Expand Down
17 changes: 17 additions & 0 deletions tests/FSharp.Compiler.Service.Tests/FSharp.CompilerService.SurfaceArea.netstandard.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2440,6 +2440,8 @@ FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 ParameterList
FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 PatternType
FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 RangeOperator
FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 RecordField
FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 TypeAbbreviationOrSingleCaseUnion
FSharp.Compiler.EditorServices.CompletionContext+Tags: Int32 UnionCaseFieldsDeclaration
FSharp.Compiler.EditorServices.CompletionContext: Boolean Equals(FSharp.Compiler.EditorServices.CompletionContext)
FSharp.Compiler.EditorServices.CompletionContext: Boolean Equals(System.Object)
FSharp.Compiler.EditorServices.CompletionContext: Boolean Equals(System.Object, System.Collections.IEqualityComparer)
Expand All @@ -2451,6 +2453,8 @@ FSharp.Compiler.EditorServices.CompletionContext: Boolean IsParameterList
FSharp.Compiler.EditorServices.CompletionContext: Boolean IsPatternType
FSharp.Compiler.EditorServices.CompletionContext: Boolean IsRangeOperator
FSharp.Compiler.EditorServices.CompletionContext: Boolean IsRecordField
FSharp.Compiler.EditorServices.CompletionContext: Boolean IsTypeAbbreviationOrSingleCaseUnion
FSharp.Compiler.EditorServices.CompletionContext: Boolean IsUnionCaseFieldsDeclaration
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsAttributeApplication()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsInherit()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsInvalid()
Expand All @@ -2459,6 +2463,8 @@ FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsParameterList()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsPatternType()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsRangeOperator()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsRecordField()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsTypeAbbreviationOrSingleCaseUnion()
FSharp.Compiler.EditorServices.CompletionContext: Boolean get_IsUnionCaseFieldsDeclaration()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext AttributeApplication
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext Invalid
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext NewInherit(FSharp.Compiler.EditorServices.InheritanceContext, System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]])
Expand All @@ -2467,10 +2473,14 @@ FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext NewRecordField(FSharp.Compiler.EditorServices.RecordContext)
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext PatternType
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext RangeOperator
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext TypeAbbreviationOrSingleCaseUnion
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext UnionCaseFieldsDeclaration
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_AttributeApplication()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_Invalid()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_PatternType()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_RangeOperator()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_TypeAbbreviationOrSingleCaseUnion()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext get_UnionCaseFieldsDeclaration()
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext+Inherit
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext+OpenDeclaration
FSharp.Compiler.EditorServices.CompletionContext: FSharp.Compiler.EditorServices.CompletionContext+ParameterList
Expand Down Expand Up @@ -3472,25 +3482,32 @@ FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate: FSharp.Compiler.Text.
FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate: FSharp.Compiler.Text.Range range
FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate: System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]] get_path()
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: 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 New
FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(FSharp.Compiler.EditorServices.RecordContext)
FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(System.Object)
FSharp.Compiler.EditorServices.RecordContext: Boolean Equals(System.Object, System.Collections.IEqualityComparer)
FSharp.Compiler.EditorServices.RecordContext: Boolean IsConstructor
FSharp.Compiler.EditorServices.RecordContext: Boolean IsCopyOnUpdate
FSharp.Compiler.EditorServices.RecordContext: Boolean IsDeclaration
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_IsNew()
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+Constructor
FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+CopyOnUpdate
FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+Declaration
FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+New
FSharp.Compiler.EditorServices.RecordContext: FSharp.Compiler.EditorServices.RecordContext+Tags
FSharp.Compiler.EditorServices.RecordContext: Int32 GetHashCode()
Expand Down
61 changes: 61 additions & 0 deletions vsintegration/tests/UnitTests/CompletionProviderTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,67 @@ let x = A.``B C`` + D.``E F``
"""
VerifyCompletionListSpan(fileContents, "D.``E F``", "``E F``")

[<Test>]
let ``No completion on record field identifier at declaration site``() =
let fileContents = """
type A = { le: string }
"""
VerifyNoCompletionList(fileContents, "le")

[<Test>]
let ``Completion list on record field type at declaration site contains modules and types but not keywords or functions``() =
let fileContents = """
type A = { Field: l }
"""
VerifyCompletionList(fileContents, "Field: l", ["LanguagePrimitives"; "List"], ["let"; "log"])

[<Test>]
let ``No completion on union case identifier at declaration site``() =
let fileContents = """
type A =
| C of string
"""
VerifyNoCompletionList(fileContents, "| C")

[<Test>]
let ``No completion on union case field identifier at declaration site``() =
let fileContents = """
type A =
| Case of blah: int * str: int
"""
VerifyNoCompletionList(fileContents, "str")

[<Test>]
let ``Completion list on union case type at declaration site contains modules and types but not keywords or functions``() =
let fileContents = """
type A =
| Case of blah: int * str: l
"""
VerifyCompletionList(fileContents, "str: l", ["LanguagePrimitives"; "List"], ["let"; "log"])

[<Test>]
let ``Completion list on union case type at declaration site contains modules and types but not keywords or functions2``() =
let fileContents = """
type A =
| Case of l
"""
VerifyCompletionList(fileContents, "of l", ["LanguagePrimitives"; "List"], ["let"; "log"])

[<Test>]
let ``Completion list on type alias contains modules and types but not keywords or functions``() =
let fileContents = """
type A = l
"""
VerifyCompletionList(fileContents, "= l", ["LanguagePrimitives"; "List"], ["let"; "log"])

[<Test>]
let ``No completion on enum case identifier at declaration site``() =
let fileContents = """
type A =
| C = 0
"""
VerifyNoCompletionList(fileContents, "| C")

#if EXE
ShouldDisplaySystemNamespace()
#endif
32 changes: 0 additions & 32 deletions vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4257,38 +4257,6 @@ let x = query { for bbbb in abbbbc(*D0*) do
[ "Contains" ] // should contain
[ ] // should not contain

[<Test>]
member public this.``InDeclaration.Bug3176a``() =
AssertCtrlSpaceCompleteContains
[ "type T<'a> = { aaaa : 'a; bbbb : int } " ]
"aa" // marker
[ "aaaa" ] // should contain
[ "bbbb" ] // should not contain

[<Test>]
member public this.``InDeclaration.Bug3176b``() =
AssertCtrlSpaceCompleteContains
[ "type T<'a> = { aaaa : 'a; bbbb : int } " ]
"bb" // marker
[ "bbbb" ] // should contain
[ "aaaa" ] // should not contain

[<Test>]
member public this.``InDeclaration.Bug3176c``() =
AssertCtrlSpaceCompleteContains
[ "type C =";
" val aaaa: int" ]
"aa" // move to marker
["aaaa"] [] // should contain 'aaaa'

[<Test>]
member public this.``InDeclaration.Bug3176d``() =
AssertCtrlSpaceCompleteContains
[ "type DU<'a> =";
" | DULabel of 'a" ]
"DULab" // move to marker
["DULabel"] [] // should contain 'DULabel'

[<Test>]
member public this.``IncompleteIfClause.Bug4594``() =
AssertCtrlSpaceCompleteContains
Expand Down