Feat/exclude completions of variable initializers

[chenjigeng]

Fixes #41731

[chenjigeng]

@andrewbranch Please look at this PR here.I am doing the same thing as you thought.
Walk up parent nodes, and bail if the current node is FunctionLikeDeclaration. Keep going until we find the variable declaration.

I changed the existing test cases (because they had previously considered undeclared variables) and added a new test cases

[andrewbranch]

Looks good, but I want to wait until more of the team is back after the holidays to add an additional review.

[andrewbranch]

You may want to check if you’re actually in the body of the function, since this fails to filter out e.g. parameter defaults:

const fn = (p = /* 'fn' appears here */) => {}

That said, leaving in some bad completions that we already have is not a big problem IMO, so this is a minor nitpick.

A similar comment applies to isVariableDeclaration(node): what you’re interested in is whether we walked up from the initializer of a variable declaration, but you might have a false positive on a case like this:

const { a, b = /**/ } = { ... }

We’re not interested in getting the variable declaration if we started at the default binding for b. But, it ends up having no effect on the outcome, since there will be no symbol where symbol.valueDeclaration === variableDeclaration. But if it’s easy to skip that property access and equality check, we should do it, since it runs for every completion item. It could maybe shave off a couple milliseconds if the list is long.

This also brings up another point worth noting, that this logic won’t work for destructuring:

const { a, b } = { /** }; // 'a' and 'b' are offered

but I don’t think it’s worth doing anything about this. I really don’t want to have to loop through every binding element declaration for every symbol—I’m happy that currently the check is only one equality comparison per symbol.

[chenjigeng]

1. You may want to check if you’re actually in the body of the function, since this fails to filter out e.g. parameter defaults:

   Yes. You are right, maybe we should use isFunctionBlock to check.

2. We’re not interested in getting the variable declaration if we started at the default binding for b. But, it ends up having no effect on the outcome, since there will be no symbol where symbol.valueDeclaration === variableDeclaration. But if it’s easy to skip that property access and equality check, we should do it, since it runs for every completion item. It could maybe shave off a couple milliseconds if the list is long.

   I think we can use the isBindingPattern to filter out this two situations

const { a, b = /**/ } = { ... }
const [ a, b = /**/ ] = [ ... ]

3. This also brings up another point worth noting, that this logic won’t work for destructuring

   I have the same idea with you. I think the time loss may be too much

For the 1 and 2, I have updated the code and added some test cases. Please help check it @andrewbranch

[andrewbranch]

Supersedes #42056

[andrewbranch]

This doesn’t work for arrow function expressions without braces, e.g. () => expression/**/. It seems like there should be a util for this but I can’t find an existing one: isFunctionBlock(node) || isArrowFunction(node.parent) && node.parent.expression === node

[chenjigeng]

Yes. But I found no node.parent.expression. maybe node.parent.body?
isFunctionBlock(node) || (isArrowFunction(node.parent) && node.parent.body === node)
I has updated the code and add a new test case of arrow function expressions without braces

[andrewbranch]

Yeah, body is what I meant 👍

[andrewbranch]

This was a very strange test

[chenjigeng]

This test has existed before, after this modification, there should be no 'x'. Let me optimize it

[andrewbranch]

I’m not saying there’s anything wrong here, just observing that it was weird for someone to write this test this way in the first place, a long time ago. I think the changes you made to these make sense 👍

[andrewbranch]

Looks good to me 🌟

[sandersn]

Just some minor formatting requests -- the check and its tests look correct to me.

[chenjigeng]

Just some minor formatting requests -- the check and its tests look correct to me.

thanks for you suggestions. done @sandersn

[sandersn]

One minor nit with a cast

[sandersn]

shouldn't this be as VariableDeclaration | undefined?

[chenjigeng]

Yes. You are right. Done