Skip method this type validity filter for objects with more than 20 members #28692
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
sheetalkamat
left a comment
sheetalkamat
left a comment
There was a problem hiding this comment.
Could this be confusing from user perspective? I am wondering what we are achieving anything significant through filtering. To me it seems the filtering helps when there are too many options but here we are limiting when there are fewer options so seems contradictory. Change looks good but will let @DanielRosenwasser comment on whether we should skip this type check all the time or with the condition you have.
| if (typeChecker.isValidPropertyAccessForCompletions(node.kind === SyntaxKind.ImportType ? <ImportTypeNode>node : <PropertyAccessExpression>node.parent, type, symbol)) { | ||
| const props = type.getApparentProperties(); | ||
| for (const symbol of props) { | ||
| if (typeChecker.isValidPropertyAccessForCompletions(node.kind === SyntaxKind.ImportType ? <ImportTypeNode>node : <PropertyAccessExpression>node.parent, type, symbol, props.length > 20)) { |
Member
There was a problem hiding this comment.
Make 20 a constant for better readability and maintainability
Member
Author
|
It's probably worth noting that at least @rbuckton apparently has a library that relies on this filtering for more accurate method completions. :S |
Member
Author
|
Superseded by #31377 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #23285 specifically by limiting the number of properties we filter methods on to 20 (so if an object has > 20 members, we no longer filter invalid method calls from the completion list), though the underlying issue remains. It's also questionable if the filtering is even really correct, since using the member in a non-calling fashion is still acceptable - the choice to filter methods at all feels somewhat pragmatic.
That underlying issue is that every anonymous type instantiation is unique - so even though we're querying similar types across the ~300 this-type comparisons we perform, since the input anonymous types are differing identities, we repeat a lot of very similar work. Caching on signatures doesn't help here, since each comparison is triggered for a different signature. The issue is simply that if I have 200 methods with
this: {item: T}, then every time we infer from{item: string}to{item: T}and instantiateTwithstring, we get a new overall type identity and need to perform a structural comparison, even though it's still just{item: string}.