Improve record, union, enum completions

[kerams]

This PR aims to disable autocompletion or provide filtered completion items in the context of (nominal) record, union and enum declarations.

Addressed issues:
#5519
#5974 (even though it's not talking about VS specifically)

Consider the following snippet, $1 - $8 represent places where autocompletion is triggered (i.e. ctrl + spacebar)

type A = { le$1: str$2 }

type B =
    | Z$3 of le$4: unit * un$5

type C =
    | X$6 = 0

type D =
    | $7

type E =
    | X of $8

Right now, unfiltered completion items (including keywords) are provided at all of these positions. At position $1 the top completion item is the keyword let, which is obviously wrong.

With the changes in this PR, the completions are as follows

• $1 - within a record field identifier, no completions shown
• $2 - expecting a type, only "type-like" completions shown (ModuleOrNamespaces, Types, UnqualifiedType, ExnCase)
• $3 - within a case identifier, no completions shown
• $4 - within a case field identifier, no completions shown
• $5 - expecting a type, only "type-like" completions shown
• $6 - within an enum, no completions shown anywhere
• $7 - within an incomplete definition that the parser does not recognize as a union, all completions shown, but it's worth mentioning that unless you press ctrl + spacebar after typing pipe and space, the completions are not shown.
• $8 - expecting a type, but this position is outside of the union as recognized by the parser. Similarly to $7, pressing ctrl + space will show all completions, but typing a letter instead will trigger "type-like" completions only.

@vzarytovskii, are there any tests for completions?

[kerams]

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?

[kerams]

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.

[kerams]

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

[dsyme]

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

[dsyme]

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

[vzarytovskii]

Hey @kerams, sorry for delay in reply

@vzarytovskii, are there any tests for completions?

Yeah, we do have some completion tests here:

fsharp/tests/service/EditorTests.fs

Lines 328 to 345 in eccf87b

	[<Test>]
	let ``Completion in base constructor`` () =
	let input =
	"""
	type A(foo) =
	class
	end
	type B(bar) =
	inherit A(bar)"""
	// Split the input & define file name
	let inputLines = input.Split('\n')
	let file = "/home/user/Test.fsx"
	let parseResult, typeCheckResults = parseAndCheckScript(file, input)
	let decls = typeCheckResults.GetDeclarationListInfo(Some parseResult, 7, inputLines.[6], PartialLongName.Empty(17), (fun _ -> []))
	decls.Items |> Seq.exists (fun d -> d.Name = "bar") |> shouldEqual true

As well as here:

fsharp/tests/FSharp.Compiler.Private.Scripting.UnitTests/CompletionTests.fs

Lines 12 to 21 in eccf87b

	[<Fact>]
	member _.``Instance completions in the same submission``() =
	async {
	use script = new FSharpScript()
	let lines = [ "let x = 1"
	"x." ]
	let! completions = script.GetCompletionItems(String.Join("\n", lines), 2, 2)
	let matchingCompletions = completions |> Array.filter (fun d -> d.Name = "CompareTo")
	Assert.Equal(1, matchingCompletions.Length)
	} |> Async.StartAsTask :> Task

There are also some VS-specific completion tests here:

fsharp/vsintegration/tests/UnitTests/CompletionProviderTests.fs

Lines 135 to 158 in eccf87b

	[<Test>]
	let ShouldTriggerCompletionAtCorrectMarkers() =
	let testCases =
	[("x", true)
	("y", true)
	("1", false)
	("2", false)
	("x +", false)
	("Console.Write", false)
	("System.", true)
	("Console.", true) ]
	for (marker, shouldBeTriggered) in testCases do
	let fileContents = """
	let x = 1
	let y = 2
	System.Console.WriteLine(x + y)
	"""
	let caretPosition = fileContents.IndexOf(marker) + marker.Length
	let documentId = DocumentId.CreateNewId(ProjectId.CreateNewId())
	let getInfo() = documentId, filePath, []
	let triggered = FSharpCompletionProvider.ShouldTriggerCompletionAux(SourceText.From(fileContents), caretPosition, CompletionTriggerKind.Insertion, getInfo, IntelliSenseOptions.Default)
	Assert.AreEqual(shouldBeTriggered, triggered, "FSharpCompletionProvider.ShouldTriggerCompletionAux() should compute the correct result")

and here:

fsharp/vsintegration/tests/UnitTests/FsxCompletionProviderTests.fs

Lines 81 to 93 in eccf87b

	[<Test>]
	let fsiShouldTriggerCompletionInFsxFile() =
	let fileContents = """
	fsi.
	"""
	let expected = List<string>([
	"CommandLineArgs"; "EventLoop"; "FloatingPointFormat"; "FormatProvider"; "PrintDepth";
	"PrintLength"; "PrintSize"; "PrintWidth"; "ShowDeclarationValues"; "ShowIEnumerable";
	"ShowProperties"; "AddPrinter"; "AddPrintTransformer"; "Equals"; "GetHashCode";
	"GetType"; "ToString"; ])
	// We execute in a seperate appdomain so that we can set BaseDirectory to a non-existent location
	getWorker().VerifyCompletionListExactly(fileContents, "fsi.", expected)

[dsyme]

@kerams This is fantastic work

[baronfel]

I agree, reading this made me realize that I didn't need my homemade completion context in FSAC for determining if we're in a string literal, I could just extend this same codebase. Nice inspiration, @kerams :)

[kerams]

Are these tests sufficient?

A couple of tests like this started failing:

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

I'm not sure what is being tested here. That if autocomplete is triggered in the middle of the identifier, the entire identifier appears in the completion list? Why would that be desirable?

[dsyme]

if autocomplete is triggered in the middle of the identifier, the entire identifier appears in the completion list?

From the naming it seems to be a correct test. Ctrl-space shows the full identifiers (this after 's'):

[kerams]

Right, but what use is it at declaration site? If I have type A = { field1: string; field2: string } and do ctrl + space on 'f', the caret just moves to the end of the identifier. If this behavior needs to be preserved, I'll have to rework the PR a little bit.

[dsyme]

Right, but what use is it at declaration site?

Yes, you are correct, this shouldn't activate at a declaration site

[dsyme]

@kerams Could you add these cases

type A = $1
type A = le$1

These should show completions for a type though note the user may also be about to type {, | `or a fresh union case identifier.

[kerams]

@dsyme, completions for aliases should now only include type-like items. This is now ready for a final review.

Prior to this PR, the main completion types looked like this:

[<RequireQualifiedAccess>]
type RecordContext =
    | CopyOnUpdate of range: range * path: CompletionPath
    | Constructor of typeName: string
    | New of path: CompletionPath

[<RequireQualifiedAccess>]
type CompletionContext = 
    /// Completion context cannot be determined due to errors
    | Invalid
    /// Completing something after the inherit keyword
    | Inherit of context: InheritanceContext * path: CompletionPath
    /// Completing records field
    | RecordField of context: RecordContext
    | RangeOperator
    /// Completing named parameters\setters in parameter list of constructor\method calls
    /// end of name ast node * list of properties\parameters that were already set
    | ParameterList of pos * HashSet<string>
    | AttributeApplication
    | OpenDeclaration of isOpenType: bool
    /// Completing pattern type (e.g. foo (x: |))
    | PatternType

The XmlDoc for CompletionContext.Invalid suggests it should be used for things like parser errors. In reality, it's often returned when the context is known, but we don't want any completions (e.g. the caret is inside a string literal). Right now, I'm also (ab)using it when the caret is inside a union case (field) identifier.

I've added Declaration of isInIdentifier: bool to RecordContext. When the value is true, no completions are desired. CompletionContext has been extended with TypeAbbreviation and UnionCaseFieldsDeclaration. I don't like the latter - it should realistically be converted to something akin to RecordField with its own context type.

Do we want to keep the completion context minimal and reuse Invalid wherever possible? Or should the context include as much detail as possible for future use?

I imagine a meticulous context (thinking solely about record, union, enum, type alias completions) would look like this:

// NEW
[<RequireQualifiedAccess>]
type RecordDeclarationContext =
    | FieldIdentifier
    | FieldType
    | Other // the caret is somewhere else in a record declaration

[<RequireQualifiedAccess>]
type RecordContext =
    | CopyOnUpdate of range: range * path: CompletionPath
    | Constructor of typeName: string
    | New of path: CompletionPath
    | Declaration of context: RecordDeclarationContext // NEW

// NEW
[<RequireQualifiedAccess>]
type UnionDeclarationContext =
    | CaseIdentifier
    | CaseType
    | CaseFieldIdentifier
    | Other // the caret is somewhere else in a union declaration

// NEW
[<RequireQualifiedAccess>]
type UnionContext =
    | Declaration of context: UnionDeclarationContext
    // this could be extended to account for the usage of a case constructor, but I'm not sure we're able to recognize that right now

[<RequireQualifiedAccess>]
type CompletionContext = 
    /// Completion context cannot be determined due to errors
    | Invalid
    /// Completing something after the inherit keyword
    | Inherit of context: InheritanceContext * path: CompletionPath
    /// Completing records field
    | RecordField of context: RecordContext
    | RangeOperator
    /// Completing named parameters\setters in parameter list of constructor\method calls
    /// end of name ast node * list of properties\parameters that were already set
    | ParameterList of pos * HashSet<string>
    | AttributeApplication
    | OpenDeclaration of isOpenType: bool
    /// Completing pattern type (e.g. foo (x: |))
    | PatternType
    | TypeAbbreviation  // NEW
    | Union of context: UnionContext // NEW

Also curious to hear what @cartermp and @baronfel think.

[dsyme]

I imagine a meticulous context (thinking solely about record, union, enum, type alias completions) would look like this:

I see, thank you!

Perhaps for now add a comment at each of the places where we return an Invalid, saying what the context actually is.

[dsyme]

I've left two requests for additional comments, otherwise this is great and ready to go, so will mark as approved

[dsyme]

I think add a comment that this covers both type abbreviation type X = int$ and single-case-unions-without-the-bar type X = AB$ , and perhaps rename to cover that.

[pbachmann]

@KevinRansom

I feel stupid for asking, but...

Given that these changes appear to have been merged into main in Dec last year, shouldn't I be seeing them by today (in Visual Studio v17.1.4)?

eg. with

type A = { le$1: str$2 }

$1 still shows 'let' at the top of the list.

The reason for my interest is that this seems to be a hugely important area for improvement - and I dread to think how many people who have tried to learn F# have given up after intellisense has misdirected them. I know you have continued your work with #12873 but there seems much more to do.

Take a very basic example where a record type has been defined and the coder is trying to use it:

type Employee = {EmployeeId:int; YearsOfService:int}

type PersonCategory = 
    |Manager 
    |Employee of Employee
    |Customer of {| CustomerId: int |}

type Person = {Name:string; Category:PersonCategory}

let kevin = { $1
let kevin = {Person.Name = "Kevin"; $2}
let kevin = {Person.Name = "Kevin"; Category = $3}
let kevin = {Person.Name = "Kevin"; Category = Customer {| $4 |} }

I find:
$1 Lists all sorts of stuff including Name
$2 The red underline error knows that Category is so I expect it to be prompted for this automatically. Instead I have to press Ctl + Space and see a bunch of stuff, almost none of it useable.
$3 One of the wonderful things about F# is that there is a known limit to what can appear here, but there is no automatic prompt and Ctl+Space prompts with a long list of mostly unusable stuff.
$4 Red squiggly compiler error knows that CustomerId is required, but VS does not prompt for it.

[kerams]

Nope, you need 17.2., which is available as a preview. Also please note that this PR deals with completions when declaring types, therefore it isn't going to affect what you see in your second snippet. There's a lot of room for improvements, no doubt about that.

[pbachmann]

Thanks for the response.

I installed VS 2022 17.2 preview but no luck. Language tools installed appearing in this version appears to be the same as before, maybe that explains why no change: ?

Visual F# Tools 17.1.0-beta.21610.4+07b5673e4f2fa7630e78abe37f16b372353a7242

[kerams]

I see 17.1.0-beta.22178.3+6da0245a7ce4bb8483b8d1f2993c8ecaea967ad9 in 17.2.0 Preview 3.0. The changes here have worked for me since Preview 1 I think.

I only have the preview installed, so I have no idea how the 2 versions would interact.

[pbachmann]

I thought if it is good enough for an MSFT guy to be using a preview version, it's good enough for me, so I uninstalled both versions and reinstalled the preview. Visual F# Tools version now matches yours but still get prompted to name my field 'let'.

[kerams]

Sorry, didn't realize this was the case (adding a new field) you were trying out. That piece of the puzzle will be fixed with #12873. Try adding a union case though, that should already behave nicely.

The shortcoming of this PR is the fact that the type already needs to be present for you not to get completions on the identifier. In other words, the improvement is currently only present when going back to edit an existing field's name.

[pbachmann]

All good now, thanks so much. I went through all your examples and confirmed that they work as you describe.

This work may be difficult (the compiler not always providing you with enough information), but I think it will be a HUGE benefit to people learning F#. In some ways, there is nothing better you can do for a developer than nudge them the right direction, and nothing worse you can do than lead them astray.