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

In this PR we introduce a new getAssignmentIdentifiers method to extract identifiers allowed in the left hand side.

In Babel 8, [...] getAssignmentIdentifiers only returns identifiers that is reassigned.

To be clear, will this method return identifiers that are assigned to, or identifiers that can be assigned to (but don't create a new binding)? If it's the latter, is this affected by non-strict mode (where assigning to an undeclared variable creates a property on the global object)?

To be clear, will this method return identifiers that are assigned to, or identifiers that can be assigned to (but don't create a new binding)? If it's the latter, is this affected by non-strict mode (where assigning to an undeclared variable creates a property on the global object)?

Good catch. This method is supposed to return those that are assigned and it does not take into account non-strict mode. I overlooked the non-strict mode behaviour of assignment expressions. I believe this is why getBindingIdentifiers currently handles assignment expressions as well.

However, getBindingIdentifiers also covers update expressions and unary expressions, e.g. delete x, which mutates the value referenced by the identifiers but never introduce new bindings.

The map getBindingIdentifiers.keys also determines the virtual type BindingIdentifier, and here is known mismatch between Babel's type BindingIdentifier / ReferencedIdentifier and the spec production BindingIdentifier / IdentifierReference. Note: Babel's BindingIdentifier and ReferencedIdentifier also cover some TS/Flow types, which are omitted for clarity.

As we can see,

• the Babel BindingIdentifier covers binding identifier as well as most mutated identifier references, however, it does not cover x in for (x of []);
• the Babel ReferencedIdentifier covers mostly immutable references, however, it does cover x in x++
• the Babel BindingIdentifier set intersects with ReferencedIdentifier, e.g. x in x++ can be matched by both BindingIdentifier and ReferencedIdentifier
• The union of BindingIdentifier and ReferencedIdentifier is BindingIdentifier + LabelIdentifier + IdentifierReference

If we introduce getAssignmentIdentifiers without updating these virtual types, it might be even more confusing as a Babel "BindingIdentifier", e.g. IdentifierReference in an assignment LHS, will not be returned from getBindingIdentifiers.

Here are some use cases for Babel's "BindingIdentifier" and "ReferencedIdentifier":

1. transform-parameters deoptimize ...args when args is shadowed or mutated using BindingIdentifier
   

   babel/packages/babel-plugin-transform-parameters/src/rest.ts

   Lines 196 to 207 in 55a2050

   	/**
   	* Deopt on use of a binding identifier with the same name as our rest param.
   	*
   	* See https://github.com/babel/babel/issues/2091
   	*/
   	BindingIdentifier(path, state) {
   	if (referencesRest(path, state)) {
   	state.deopted = true;
   	}
   	},
   	};

2. transform-reserved-words renames invalid ES3 identifiers using the union BindingIdentifier | ReferencedIdentifier
   

   babel/packages/babel-plugin-transform-reserved-words/src/index.ts

   Lines 11 to 14 in 55a2050

   	"BindingIdentifier|ReferencedIdentifier"(path: NodePath<t.Identifier>) {
   	if (!t.isValidES3Identifier(path.node.name)) {
   	path.scope.rename(path.node.name);
   	}

3. transform-class-properties renames the class binding references by the new class name using ReferencedIdentifier
   

   babel/packages/babel-helper-create-class-features-plugin/src/fields.ts

   Lines 1440 to 1449 in 55a2050

   	const innerReferencesVisitor: Visitor<ReplaceInnerBindingReferenceState> = {
   	ReferencedIdentifier(path, state) {
   	if (
   	path.scope.bindingIdentifierEquals(path.node.name, state.innerBinding)
   	) {
   	state.needsClassRef = true;
   	path.node.name = state.thisRef.name;
   	}
   	},
   	};

4. traverse collects whether arguments are referenced using isReferencedIdentifier:
   

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

   Lines 778 to 779 in 55a2050

   	Identifier(child, { argumentsPaths }) {
   	if (!child.isReferencedIdentifier({ name: "arguments" })) return;

5. traverse remove bindings from the scope
   

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

   Lines 27 to 30 in 55a2050

   	export function _removeFromScope(this: NodePath) {
   	const bindings = getBindingIdentifiers(this.node, false, false, true);
   	Object.keys(bindings).forEach(name => this.scope.removeBinding(name));
   	}

If we are to align current Babel definition to the spec notation, we will have to adjust case 1 and 4 since they now cover different AST structures. The case 2 will not be affected because the union set is not changed unless we want to separate LabelIdentifier from the BindingIdentifier. The case 3 is probably fine, too as the inner class binding is immutable. The case 5 need no changes and we can further remove the newBindingsOnly option.

I think the case 1 justifies the requirement of getAssignmentIdentifiers, although we can query similar information from binding.constantViolations, the AST query will return the up-to-date information, otherwise we will have to do a scope crawl to ensure that the constantViolations is correct.

One of technical challenges that may block alignment is identifiers within a pattern. To determine whether such an identifier is a reference, we have to walk up to the parent of the toplevel object/array pattern, but the current isBindingIdentifier only have access to up to 2 levels of ancestors. We could do the walk before calling the query and pass down something like patternParent, but then the babel-types isBindingIdentifier and isReferecedIdentifier check will be more expensive and no longer context-free.