Fix completion record for variable declarations

[addaleax]

Q	A
Fixed Issues?	–
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link	–
Any Dependency Changes?	–
License	MIT

Variable declarations are ignored for completion records,
so they should be skipped.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0e512b4:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47525/

[JLHwung]

I think we should iterate statement list from end to start, if the completion record is a variable declaration, discard it and check the previous one:

babel/packages/babel-traverse/src/path/family.ts

Lines 195 to 201 in 844baeb

	// When we are in a context where `break` must not exist, we can skip linear
	// search on statement lists and assume that the last statement determines
	// the completion
	completions = completions.concat(
	_getCompletionRecords(paths[paths.length - 1], context),
	);
	}

[addaleax]

@JLHwung Sure, done!

[JLHwung]

I think we should apply _getCompletionRecords and check if the completion is VariableDeclaration, e.g. The completion record of this snippet

1 + 2;
{
  var a = 1;
}

is 1 + 2.

[addaleax]

@JLHwung Sure, done, and added a test. I have to admit that I’m a bit unsure about the code here – is it valid to assume that if the list of completion records returned by _getCompletionRecords contains a VariableDeclaration, then that automatically implies that the list has length 1? It seems like something that should be true to me, and I’m not sure how to handle this if it isn’t.

[JLHwung]

is it valid to assume that if the list of completion records returned by _getCompletionRecords contains a VariableDeclaration, then that automatically implies that the list has length 1

That's a very good question! AFAIK only IfStatement, SwitchStatement and TryStatement may produce multiple completion records, and if a VariableDeclaration is directly within these statements, it should be recursively handled here and as long as we don't return VariableDeclaration as completion records in getStatementListCompletion, then the VariableDeclaration must not come from these statements and thus the list here should contain only one completion record.

[addaleax]

Right – that’s what I’d be thinking. :) Should I add an assertion of some kind?

[JLHwung]

an assertion of some kind

I think it's fine. The _getCompletionRecords is mostly used in proposal-do-expression, and the proposal now bans variable declaration ending in do block. So I guess few people will use it in that way.

[nicolo-ribaudo]

Thanks!