Filter out self-fulfilling object literal member completions #35709
Conversation
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at f398f4c. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
| /// <reference path="fourslash.ts" /> | ||
|
|
||
| ////function f1<T>(x: T) {} | ||
| ////f1({ abc/*1*/ }); |
There was a problem hiding this comment.
This change looks great but do you think we should allow abc in completions if abc is some variable in scope?
There was a problem hiding this comment.
Right, I guess don't filter our anything that could be a shorthand property declaration.
There was a problem hiding this comment.
Those were never actually provided as object literal member completions, so this PR doesn’t filter them per se. I’m not sure if anything is really needed here as those completions already come up as plain text matches (I think this is provided by Monaco / VS):
I’m not sure if it’s right to make all in-scope identifiers first-class completions. Real missing members from the contextual type are a very strong signal that the user will want to add those members, but the presence of random identifiers is a very weak signal. We can sort them appropriately, but otherwise there’s no visual distinction that one completion is a sure thing and the other is a wild guess. I think this would get really confusing.
Besides, as I learned in #34855, with generics it can be extremely difficult to know ahead of time whether some hypothetical arbitrary member is a valid completion or not. It often appears not to be until the user adds it, when inference runs and incorporates it into the contextual type just because it’s there. But with conditional types, we effectively can’t know whether that member is going to be valid without speculatively type checking the change, which is prohibitively expensive.
|
Should I still be getting completions from the constraint?
Maybe this is fine just so users know that they're typing it correctly/that it has the right type? |
|
@DanielRosenwasser correct, we’ve always given completions for the member your caret is on, I guess so you can read the type. That’s not changed in this PR. |
There was a problem hiding this comment.
Seems good - it's not clear whether always showing potential shorthand properties would be too noisy, but we can add it later if users request it. If we end up doing that, we should discuss the aforementioned UI problems with @mjbvz and @minestarks, but there's no urgency there.
Ever been typing in a generic object literal and with every other keystroke, it seems you have magically just finished typing a member name that the checker suddenly knows about and wants to “complete” for you?
(Before)
This removes completions for object literal members that are self-declaring.
Fixes #35268
Note: this doesn’t currently affect JSX attributes because we didn’t include them in the changes in #33937/#34855. It’s probably worth considering doing that separately, but I want to test with some real-world component libraries that are heavy on HOCs and generics.