Skip to content
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

Fixed JS completions type spread #45484

Merged
merged 5 commits into from Aug 24, 2021
Merged

Conversation

@armanio123
Copy link
Member

@armanio123 armanio123 commented Aug 17, 2021

Fixed #45436.

Also, fixes a couple of tests that I believe are incorrect.

Copy link
Member

@andrewbranch andrewbranch left a comment

There is probably some room for debate here, but I think the expected behavior described in the bug is wrong. The completions offered here are arguably correct (the existing tests certainly weren’t written in error; they are clearly asserting the behavior that’s being reverted here); the problem is that isNewIdentifierLocation is false but should be true. Flipping it to true will keep the existing completions, but will prevent . from triggering them, I think (needs to be manually tested).

Loading

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 17, 2021

Also, isMemberCompletion seems possibly wrong, but I don’t know what that controls 🤔

Loading

@armanio123
Copy link
Member Author

@armanio123 armanio123 commented Aug 17, 2021

I checked on the isNewIdentifierLocation as well and I disagree it must be true. This issue only happens for js files and when comparing against ts files, isNewIdentifierLocation still remains as false. symbols and keywordFilters are the ones that avoid returning early.

In regards to the tests, the completions provided are actually incorrect as the code would be wrong and not really access the property. i.e. in completionInUncheckedJSFile.ts, hello and goodbye are not related to the console object or in javaScriptPrototype2.ts the example is not really able to access to ctor prototype attribute so completions would be incorrect in my opinion.

Loading

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Aug 18, 2021

In regards to the tests, the completions provided are actually incorrect as the code would be wrong and not really access the property

If I recall correctly, the point of isNewIdentifierLocation being true in JS cases is that we don't really have an authoritative view of the types, and can't let editors aggressively complete there; however, it's useful to leverage other words in the file or project, and so you still want to provide completion results (again, since we don't know that they're unrelated). To try to relay that they're unrelated, we deprioritize them compared to whatever else we're confident of.

Loading

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 18, 2021

Discussed in the call, but to put in writing—the reason isNewIdentifierLocation should be true in this case is because

({ | })

at this point you very well may be writing a new property declaration or even binding pattern, which is exactly what isNewIdentifierLocation is for—any time you could be writing a name not already in scope, that property must be set to true on any completion responses. It sounds like that flag is wrong in TS files too.

Loading

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Aug 18, 2021

I think the expected behavior described in the bug is wrong

I'm not sure I agree with this part. The thing that's really being complained about is the fact that we're providing completions after a . in a context where no completion item would be valid.

But otherwise, yes, the existing tests were written intentionally.

Loading

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 18, 2021

The thing that's really being complained about is the fact that we're providing completions after a . in a context where no completion item would be valid.

Yeah, I missed that in the bug because I wasn’t reproing it successfully, but Armando clarified for me—I was claiming that there should be completions at ({ | }) (with a manual trigger), not at ({ .| }).

Loading

Copy link
Member

@andrewbranch andrewbranch left a comment

Nice work!

Loading

src/services/completions.ts Outdated Show resolved Hide resolved
Loading
((isCallExpression(node) || isFunctionLike(node)) &&
node.end === contextToken.pos &&
node.getChildCount(sourceFile) &&
last(node.getChildren(sourceFile)).kind !== SyntaxKind.CloseParenToken)) {
Copy link
Member

@andrewbranch andrewbranch Aug 24, 2021

Choose a reason for hiding this comment

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

I actually wonder if this whole second condition is even needed now... seems like the examples in the comments would be covered by the new condition you added. It doesn’t hurt to leave it in, so I probably wouldn’t change it this close to a release, but it might be worth running tests with it removed just to see if it can be cleaned up and simplified in the future.

Loading

Copy link
Member Author

@armanio123 armanio123 Aug 24, 2021

Choose a reason for hiding this comment

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

I tried removing it as well and a couple of tests failed so I decided to leave it in place. Errors were related to writing syntax like

const foo = {
    bar: function (.| ) {}
}

Loading

Copy link
Member

@sandersn sandersn left a comment

+1 on @andrewbranch's nodeIsMissing check.

Loading

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 24, 2021

@typescript-bot cherry-pick this to release-4.4

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 24, 2021

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.4 on this PR at 2253da6. You can monitor the build here.

Loading

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 24, 2021

Hey @andrewbranch, I've opened #45564 for you.

Loading

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this issue Aug 24, 2021
Component commits:
33829fa Fix and updated tests

2249d6c Added test

e1a2b9e Revert "Fix and updated tests"
This reverts commit 33829fa.

51881c4 Filter out empty access expression

2253da6 PR feedback
@andrewbranch andrewbranch merged commit ead9dfb into microsoft:main Aug 24, 2021
10 checks passed
Loading
DanielRosenwasser pushed a commit that referenced this issue Aug 30, 2021
Component commits:
33829fa Fix and updated tests

2249d6c Added test

e1a2b9e Revert "Fix and updated tests"
This reverts commit 33829fa.

51881c4 Filter out empty access expression

2253da6 PR feedback

Co-authored-by: Armando Aguirre <armanio123@outlook.com>
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.4.3 milestone Sep 10, 2021
BobobUnicorn added a commit to BobobUnicorn/TypeScript that referenced this issue Oct 24, 2021
* Fix and updated tests

* Added test

* Revert "Fix and updated tests"

This reverts commit 33829fa.

* Filter out empty access expression

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants