Skip to content

Improve completions for union field patterns #15754

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
merged 2 commits into from
Aug 8, 2023
Merged

Improve completions for union field patterns #15754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4d2c1c
Improve completions for union field patterns
kerams Aug 5, 2023
fd22b45
.
kerams Aug 5, 2023
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
28 changes: 23 additions & 5 deletions src/Compiler/Service/ServiceParsedInputOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,20 +1269,29 @@ module ParsedInput =
else
let context = Some(PatternContext.NamedUnionCaseField(patId.idText, id.Range))
TryGetCompletionContextInPattern suppressIdentifierCompletions pat context pos)
| SynPat.LongIdent (argPats = SynArgPats.Pats pats; longDotId = id) ->
| SynPat.LongIdent (argPats = SynArgPats.Pats pats; longDotId = id; range = m) when rangeContainsPos m pos ->
match pats with
| [ SynPat.Named _ as pat ] ->
TryGetCompletionContextInPattern false pat (Some(PatternContext.PositionalUnionCaseField(None, id.Range))) pos

// fun (Some v| ) ->
| [ SynPat.Named _ ] -> Some(CompletionContext.Pattern(PatternContext.PositionalUnionCaseField(None, id.Range)))

// fun (Case (| )) ->
| [ SynPat.Paren (SynPat.Const (SynConst.Unit, _), m) ] when rangeContainsPos m pos ->
Some(CompletionContext.Pattern(PatternContext.PositionalUnionCaseField(Some 0, id.Range)))

// fun (Case (a| , b)) ->
| [ SynPat.Paren (SynPat.Tuple _ | SynPat.Named _ as pat, _) ] ->
TryGetCompletionContextInPattern false pat (Some(PatternContext.PositionalUnionCaseField(Some 0, id.Range))) pos
|> Option.orElseWith (fun () -> Some CompletionContext.Invalid)

| _ ->
pats
|> List.tryPick (fun pat -> TryGetCompletionContextInPattern false pat None pos)
| SynPat.Ands (pats = pats)
| SynPat.ArrayOrList (elementPats = pats) ->
pats
|> List.tryPick (fun pat -> TryGetCompletionContextInPattern suppressIdentifierCompletions pat None pos)
| SynPat.Tuple (elementPats = pats) ->
| SynPat.Tuple (elementPats = pats; commaRanges = commas; range = m) ->
pats
|> List.indexed
|> List.tryPick (fun (i, pat) ->
Expand All @@ -1295,6 +1304,15 @@ module ParsedInput =
None

TryGetCompletionContextInPattern suppressIdentifierCompletions pat context pos)
|> Option.orElseWith (fun () ->
// Last resort - check for fun (Case (a, | )) ->
// That is, pos is after the last comma and before the end of the tuple
match previousContext, List.tryLast commas with
| Some (PatternContext.PositionalUnionCaseField (_, caseIdRange)), Some mComma when
rangeBeforePos mComma pos && rangeContainsPos m pos
->
Some(CompletionContext.Pattern(PatternContext.PositionalUnionCaseField(Some(pats.Length - 1), caseIdRange)))
| _ -> None)
| SynPat.Named (range = m) when rangeContainsPos m pos ->
if suppressIdentifierCompletions then
Some CompletionContext.Invalid
Expand All @@ -1312,7 +1330,7 @@ module ParsedInput =
TryGetCompletionContextInPattern suppressIdentifierCompletions pat1 None pos
|> Option.orElseWith (fun () -> TryGetCompletionContextInPattern suppressIdentifierCompletions pat2 None pos)
| SynPat.IsInst (_, m) when rangeContainsPos m pos -> Some CompletionContext.Type
| SynPat.Wild m when rangeContainsPos m pos -> Some CompletionContext.Invalid
| SynPat.Wild m when rangeContainsPos m pos && m.StartColumn <> m.EndColumn -> Some CompletionContext.Invalid
Copy link
Contributor

@psfinaki psfinaki Aug 7, 2023

Choose a reason for hiding this comment

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

Just curious, why is this needed? When would this come into play?

Copy link
Contributor Author

@kerams kerams Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That distinguishes | G (a, ) from | G (a, _). There's no indication of these synthetic wildcards other than range. They're not even wrapped in SynPat.FromParserError.

True SynPat.Wild (_) gets CompletionContet.Invalid. Without the column check, we could get no completions on one position somewhere in , ).

Copy link
Contributor

@psfinaki psfinaki Aug 7, 2023

Choose a reason for hiding this comment

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

Hmm good to know, thanks!

Copy link
Member

@auduchinok auduchinok Aug 7, 2023

Choose a reason for hiding this comment

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

They're not even wrapped in SynPat.FromParserError.

We should probably either wrap them or add a union case that represents an error at a range without including another pattern inside of it, similar to SynExpr.ArbitraryAfterError.

Copy link
Contributor Author

@kerams kerams Aug 8, 2023

Choose a reason for hiding this comment

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

@nojaf, do you have an opinion on this?

Copy link
Contributor Author

@kerams kerams Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, so do we merge this and return to revamp the represantation of errors in patterns later? I managed to work around the deficencies, but it would be nice to make the code cleaner.

Copy link
Contributor

@psfinaki psfinaki Aug 8, 2023

Choose a reason for hiding this comment

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

Yes, I don't mind merging it this way.

Copy link
Contributor

@nojaf nojaf Aug 16, 2023

Choose a reason for hiding this comment

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

Hi @kerams, great work here!
I'm all for proper representation in AST. I think what Eugene proposes makes sense.
SynPat.Wild isn't really correct when there is no underscore I think

As for
image
Is it not a bit weird that bee is proposed here given you know you are inside a tuple and named access can no longer happen? I could be wrong here, it has all been a while for me indeed 😸.

Copy link
Contributor Author

@kerams kerams Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The top 2 completions are suggested names. In other words, the bee completion item does not refer to the bee union case field name, but it's a suggested name for a pattern in that position, which is based on the field name. It's merely the text that is the same.

Copy link
Contributor

@nojaf nojaf Aug 16, 2023

Choose a reason for hiding this comment

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

Oh I see.

| SynPat.Typed (pat = pat; targetType = synType) ->
if rangeContainsPos pat.Range pos then
TryGetCompletionContextInPattern suppressIdentifierCompletions pat previousContext pos
Expand Down
16 changes: 16 additions & 0 deletions vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,6 +1674,22 @@ let x (du: Du list) =
VerifyCompletionList(fileContents, "| [ C (first, rest); C (f", [ "option" ], [ "first" ])
VerifyCompletionList(fileContents, "| [ C (first, rest); C (f, l", [ "list" ], [ "rest" ])

[<Fact>]
let ``Completion list contains relevant items for the correct union case field pattern before its identifier has been typed`` () =
let fileContents =
"""
type Du =
| C of first: Du * second: Result<int, string>

let x du =
match du with
| C () -> ()
| C (first, ) -> ()
"""

VerifyCompletionList(fileContents, "| C (", [ "first"; "du" ], [ "second"; "result" ])
VerifyCompletionList(fileContents, "| C (first, ", [ "second"; "result" ], [ "first"; "du" ])

[<Fact>]
let ``Completion list contains suggested names for union case field pattern in a let binding, lambda and member`` () =
let fileContents =
Expand Down