Fixed JS completions type spread

[armanio123]

Fixed #45436.

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

[andrewbranch]

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).

[andrewbranch]

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

[armanio123]

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.

[DanielRosenwasser]

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.

[andrewbranch]

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.

[DanielRosenwasser]

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.

[andrewbranch]

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 ({ .| }).

[andrewbranch]

Nice work!

[andrewbranch]

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.

[armanio123]

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 (.| ) {}
}

[sandersn]

+1 on @andrewbranch's nodeIsMissing check.

[andrewbranch]

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

[typescript-bot]

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.

[typescript-bot]

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