Filter match clause completions #12990
Conversation
| let eventInfoCache = MakeInfoCache GetIntrinsicEventInfosUncached hashFlags1 | ||
| let namedItemsCache = MakeInfoCache GetIntrinsicNamedItemsUncached hashFlags2 | ||
| let mostSpecificOverrideMethodInfoCache = MakeInfoCache GetIntrinsicMostSpecificOverrideMethodSetsUncached hashFlags0 | ||
| let unionCaseInfoCache = MakeInfoCache GetIntrinsicUnionCaseInfosUncached hashFlags3 |
There was a problem hiding this comment.
This cache seems to be invalidated every time I make a change in the document (the DU stays the same though), so perhaps it's completely unnecessary? Or perhaps it's supposed to work for unions external to the current document?
|
@kerams Could you please add tests for |
|
I'm really pleased with the outcome. DU type is added automatically when qualified access is required.
When the cases are not in scope, the appropriate namespace will be opened on commit:
When we're matching against an unknown type, only items that are legal in a match clause pattern are shown, namely literals, enums, union (cases), exception cases, active patterns, modules and namespaces:
Union cases, among other completion items, unfortunately fall under Marking it as ready for review. Tests will be added once the other completion PRs are merged in order to avoid conflicts. |
|
#5594 should be resolved too. |
|
Will it be possible to add some simple tests (including what @auduchinok suggested) please? Otherwise looks good to me. |
|
Of course, after the other completion PRs get in. |
Awesome, thanks. You mean #12933? Just merged it. Let me know if you'll need some help with conflicts. |
|
@vzarytovskii, @dsyme, @KevinRansom, while I'm adding more tests, can you give this a go? It's a major change (towards the better, with any luck), so I'd like to know everything behaves as expected and feels right to others too. There are further low-hanging fruits that I'm leaving open for followup PRs (right now these positions should give you the full completions list)
|
|
@kerams I'll take a close look at this and give it a go Some initial questions I'll be thinking about:
|
In general yes. However, when it's known (via type annotations or previous match clauses) that the type of the identifier the user is completing is a DU, only its cases and active patterns are shown.
RQA should be covered. AutoOpen works like this
In the second case
This opens |
|
Got it, |
|
One more thing module N =
module M =
type ChoiceZ =
| Choice1
| Choice2
open N
let call (choice: N.M.ChoiceZ) =
match choice with
| cinserts the open at the wrong place module N =
module M =
type ChoiceZ =
| Choice1
| Choice2
open M
open N
let call (choice: N.M.ChoiceZ) =
match choice with
| Choice1I'm inclined to say that correct open insertion is not my problem :). |
As long as it inserts a correct open (i.e. syntactically and semantically), it's fine, if needed, auto-inserts of |
|
Hmmm stack overflow failure in test on MacOS |
|
I'm doing some adhoc testing for completions in pattern matching and will jot notes on various cases I've tried using this PR. 🟢 Manually verified fixed as part of this PR 🟠 Completions are not restricted here. This is likely a more general problem in parser error recovery. We should get this fixed because it's so basic, but I'm not expecting it to be part of this PR 🟠 Likewise completions are not restricted here (ideally they should be). This is likely the same problem in parser error recovery. 🟢 They are filtered here 😃 😃 😃 😃 🔴 Over-filtering - module 🟢 Correct filtering here: type C = AAAA | BBB | CCC
let f (n: C) =
match n with
| (C$)
| BBB -> 2
🟠 No filtering here, because no captured resolutions, we could improve this in a separate PR type C = AAAA | BBB | CCC
let f (n: C) =
match n with
| C$ _ -> 1
| BBB -> 2🔴 No type C = AAAA | BBB | CCC
let (|HHH|) (c: C) = None
let f (n: C) =
match n with
| H$
| BBB -> 2Likewise here: type C = AAAA | BBB | CCC
let (|HHH|_|) (c: C) = None
let f (n: C) =
match n with
| H$
| BBB -> 2Likewise here: type C = AAAA | BBB | CCC
let (|HHH|JJJ|) (c: C) = HHH
let f (n: C) =
match n with
| H
| BBB -> 2
I was also expecting this to filter? type C() = class end
let (|HHH|JJJ|) (c: C) = HHH
let f (n: C) =
match n with
| H
| _ -> 2
|
As a general rule, all valid completions must always be shown - if a valid completion is not in the list the user will generally consider this a bug in auto-complete. There are a couple of places we violate this rule (e.g. we don't show I think we need to filter the modules according to whether they have contain relevant active patterns or not. I'll take a look to see how feasible that is. Searching inside modules will generally be OK from a performance point of view, especially if cached in some way, though we need to take a little care.
I do agree with this - though I think we should first try hard to stick to the principle of showing all valid completions |
It would force reading nested modules and namespaces that may sometimes be unwanted. @dsyme Could you make this behaviour optional if you're going to work on it, please? |
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
I looked over again and left some comments
| EntityRefContainsSomethingAccessible ncenv m ad modref && | ||
| not (IsTyconUnseen ad g ncenv.amap m modref)) | ||
| |> List.map ItemForModuleOrNamespaceRef | ||
| GetVisibleNamespacesAndModulesAtPoint ncenv nenv fullyQualified m ad |> List.map ItemForModuleOrNamespaceRef |
There was a problem hiding this comment.
Thanks for factoring this out. There's quite a lot of duplication in NameResolution.fs ResolvePartial* routines and it would be really good to remove all inner functions and factor out all common/reusable predicate code
| | TType_var (typar, _) -> | ||
| match typar.Solution with | ||
| | Some paramType -> | ||
| doesActivePatternTakeTypeAsInput g ty paramType |
There was a problem hiding this comment.
You can use this I believe:
if TypeRelations.TypeFeasiblySubsumesType 0 g amap m thisTy TypeRelations.CanCoerce ty then| | TType_var (typar, _) -> | ||
| match typar.Solution with | ||
| | Some paramType -> | ||
| doesActivePatternTakeTypeAsInput g ty paramType |
There was a problem hiding this comment.
I think we ideally want to filter active patterns as well as we filter union cases. It's not good long-term if tooling works better with union cases than active patterns as it encourages people to rely on the union cases (e.g. not hide them) and avoid using active patterns.
|
@dsyme, so I've stepped through this
type C = AAAA | BBB | CCC
let f (n: C) =
match n with
| C$ _ -> 1
| BBB -> 2The problem is the fact that captured name resolutions do not contain anything for that line. Not sure why that is. |
Filtering namespaces is not really possible because it would eagerly explore too much metadata. This means we either have to show all available namespaces, or break the "show all completions" design principle in this case. I think it's a reasonable place to break this rule. Note we could turn off filtering for For modules I do think we should do the the work to look in the contents of the module for relevant active patterns as part of this PR (we could also include all module names for now and do it in a separate PR). Reasons:
Reading through nested modules is feasible, as F# metadata is always read eagerly. We should however cache the computation result in InfoReader. I don't think we'd make it optional - we don't currently allow user-specified options for auto-complete and the spec of auto-complete is pretty plain: show all available, legitimate completions, and preferably no more (with some deliberate exceptions). |
OK, I see. We can deal with these in follow up PRs if you prefer? I think the module name completions are the only blocking thing I've seen so far - also I'd like you to consider this comment https://github.com/dotnet/fsharp/pull/12990/files/5e285f23cb0d39011ebf278984400fb7e9af6a52#diff-da6f3313031f72e650eeae86b874d89513fe4b05a1fd6ebc5983a4415521d755 I've changed this one to 🟠 in the list above. |
Yes, this is what I was afraid of happening. 🙂 |
|
Aside: In the far distant past (F# 0.9!) we did filter union case completions for pattern matching. However we did it imperfectly, and ended up ripping out all the code and ditching it. I don't want that cycle to repeat - if possible I'd like us to iterate to "get this right, and get it fully tested and pinned down" so the feature sticks for the long term. I can see that will take multiple PRs. |
The rest of 🔴 should now be green.
Can you please explain what you mean here? I'm not following. |
To me the code should follow this spec: If we're at a match identifier position for ID$ (or plain empty $)
So whether the input type is a union type is not particularly important except that part (1) is equivalent to checking if the input type is a union type and returning precisely the set of union cases supported by that union type. But even if the type is a union type (2), (4), (5) still apply . So structurally we may as well just always do (1) - (6), and that will have the benefit of correctly filtering such things as string literals (only if the type is feasibly compatible with 'string'), enum type names and so on. |
|
I see, thanks.
That is true until the input type is a DU/enum that is out of scope. The relevant items then aren't in the original list in the first place. This is thus something that needs to be explicitly checked for every time, so that we can append additional completion items that open a module/namespace on commit. (Or not, it would not be a regression, but my current implementation is able to do this) Do you want to have this approach to filtering cleaned up and sorted in this PR? |
This may be quite unexpected in cases where some code is being or may be rewritten. Imagine that you're matching a function return like this: match f () with
| {caret}And, while writing a pattern, you decided to use a different type, not the one that the function returns. In R# we only move relevant symbols up and don't remove them by default (there's another filtering completion mode that uses another Ctrl+Space stroke or a different shortcut, but it's not implemented for F# yet). Moving things up and not removing makes it possible 1) to use a symbol with unexpected type and 2) to use a quick fix that would update expected type after completing the item (i.e. change the function return type in the example above). Breaking this scenario will make it much harder to change things and I'd consider this a serious regression. I've tried moving relevant union cases up in some simple scenarios in F# plugin, and it already significantly improved the completion experience: Please note how it doesn't block you from using a different type, since the completion is about helping you type any allowed things in. Though, |
Fair enough. However, that's another dimension to the problem with VS sorting I described #12990 (comment). Not only will sorting need to be implemented as completion context dependent, but moving certain DU cases up instead of removing the rest of them will require sorting to also be type dependent. Not saying I have a problem with that and what you do in Rider, but it's a bit more work and I don't immediately have a good idea of what the implementation would involve and look like. (Nor is it something I frankly feel like diving into myself right now.) |
I think I would prefer that - it seems to me it will make the code more uniform across all the cases we need to consider? I know you've taken a few iterations on this - if you're running out of steam I could take a pass to help you get it across the line? |
This is an important consideration. It is also actually another one of the reasons we removed the filtering I mentioned before (false filtering when input type has not yet been changed). |
|
@dsyme, I've done some work here. Things that are illegal in a match clause (and namespaces, non-union/enum types) are removed from the list completely. Everything else is sorted according to your spec - compatible items are on the top, the rest is just moved to the bottom. I've introduced One casualty of the refactoring are union cases out of scope, which now do not make it on the list. |
I still think removing namespaces might be a bad thing. @dsyme Could we have a switch for this logic, at least for the time being? Maybe a param to the completion items methods? |
|
/run fantomas |
|
Failed to run fantomas: https://github.com/dotnet/fsharp/actions/runs/5080709095 |
5 participants
Implements #6996.
When it's determined we're pattern matching against a DU of a specific type, the completions will only show the latter's cases as well as compatible active patterns in scope.
We could go further:
Here we can tell that we're pattern matching on the second argument of
Choice1, which is of typeChoice, so completions should be filtered in the same fashion again. But that's for another time ;).