-
Notifications
You must be signed in to change notification settings - Fork 830
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.
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
Conversation
psfinaki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff as usual, thanks @kerams :)
I am thinking we might put all these recent changes together in a blog post, that's a substantial improvement of the UX.
| |> 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 , ).
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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

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 😸.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see.
... when no identifier has been typed yet and the user presses Ctrl-space. In this case, the original ordering of completions from FCS is preserved, so we get suggested names at the top of the list (and the list itself is filtered). If you instead trigger completions by typing the first letter, VS will partially sort the list. That's not great when you don't necessarily know what you're looking for, and the suggested names would have been helpful.
Before
Current, with the first letter, sorted by VS
After