[Bug]: async({ foo33 = 1 }) is parsed without errors

[nicolo-ribaudo]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

async({ foo33 = 1 });

Configuration file name

No response

Configuration

Default parser config

Current and expected behavior

foo33 = 1 is parsed as an ObjectProperty whose value is an AssignmentPattern. It should throw, since objects cannot have property initializers with =.

Environment

Babel 7.14.4

Possible solution

No response

Additional context

Reported at https://twitter.com/buzyescayadev/status/1398526884955058181

[tony-go]

Hey @nicolo-ribaudo

As the issue #13399 was just taken.

Maybe I could give a try on this one ?

[JLHwung]

Another related issue:

// should throw duplicate proto keys
async({ __proto__: x, __proto__: y })

Spoiler, expand when you need help!

Babel tracks the pattern foo33 = 1 in refExpressionsErrors.shorthandAssign:

babel/packages/babel-parser/src/parser/expression.js

Lines 1953 to 1955 in 0b29b5c

	if (refExpressionErrors.shorthandAssign === -1) {
	refExpressionErrors.shorthandAssign = this.state.start;
	}

However, this error is not thrown as it should have, because when we don't see an arrow after async (),

babel/packages/babel-parser/src/parser/expression.js

Lines 804 to 806 in 0b29b5c

	if (state.maybeAsyncArrow) {
	this.expressionScope.exit();
	}

we lost the access to refExpressionErrors, which is created when parsing every call expression arguments:

babel/packages/babel-parser/src/parser/expression.js

Line 934 in 0b29b5c

possibleAsyncArrow ? new ExpressionErrors() : undefined,

We can create the refExpressionErrors here

babel/packages/babel-parser/src/parser/expression.js

Lines 785 to 791 in 0b29b5c

	node.arguments = this.parseCallExpressionArguments(
	tt.parenR,
	state.maybeAsyncArrow,
	base.type === "Import",
	base.type !== "Super",
	node,
	);

and pass down the refExpressionErros to parseCallExpressionArguments. After we are sure async() is a call expression, we should check refExpressionErrors like we did elsewhere. An example is parsing left hand-side expression before =:

babel/packages/babel-parser/src/parser/expression.js

Line 1530 in 0b29b5c

this.checkExpressionErrors(refExpressionErrors, true);

[tony-go]

Hi 👋 guys

I'm hacking around the parser and try to understand the flow for our bug case.

Thanks a lot @JLHwung for your pointers, it helps a lot.

I'll keep you informed.

[existentialism]

@tony-go hit us up on slack if you have any questions!

[tony-go]

Hey @JLHwung I tried async({ __proto__: x, __proto__: y }) with the repl from my pull request.

It seems to work there, but in the parser test I add a case and I got:

Expected non-recoverable error message: Redefinition of __proto__ property. (1:22). But instead parsing recovered from errors: [
      {
        "loc": {
          "line": 1,
          "column": 22
        },
        "pos": 22,
        "code": "BABEL_PARSER_SYNTAX_ERROR",
        "reasonCode": "DuplicateProto"
      }
    ]

Do you have an idea about why ? (I'll dig it anyway)

[nicolo-ribaudo]

It's because we already have the tracking logic for non-async arrow functions, and you are making the parser use the same existing logic also for async arrows.

[JLHwung]

Hey @JLHwung I tried async({ __proto__: x, __proto__: y }) with the repl from my pull request.

It seems to work there, but in the parser test I add a case and I got:

Expected non-recoverable error message: Redefinition of __proto__ property. (1:22). But instead parsing recovered from errors: [
      {
        "loc": {
          "line": 1,
          "column": 22
        },
        "pos": 22,
        "code": "BABEL_PARSER_SYNTAX_ERROR",
        "reasonCode": "DuplicateProto"
      }
    ]

Do you have an idea about why ? (I'll dig it anyway)

It should throw, which means you have fixed this issue. The error comes from the fact we are enabling errorRecovery for all the parser tests so instead of an error is thrown, Babel parser will recovery from the errors, and record the errors to the AST.

You can try removing options.json and re-run the test, the fixture runner will automatically generate output.json.

[tony-go]

Thanks a lot guys 👏

I just pushed the new test case ^^

I probably understand that our update allows to catch this error, my misunderstanding was more on the fact in one case we use:

• this.checkExpressionErrors that'll throw
• this.raise (If I understand well 😆 )

Maybe do you guys have an idea ?

[nicolo-ribaudo]

Usually use use this.raise, which immediately reports the error. However, when parsing async ({ foo33 = 1 }) there is a problem: when we find =, we don't know yet if it's an error or not. async ({ foo33 = 1 }) => {} is valid, while async ({ foo33 = 1 }); is not. For this reason, instead of using .raise we record them in ExpressionErrors and then, when we are sure that it's not an async function, we throw the errors we recorded.

[tony-go]

Hi @nicolo-ribaudo 👋

Hope you're doing right ^^

I fully understand why we use a ref (ExpressionErrors) to store the potential error and raise it only if it's necessary.

But at the end of the day this.checkExpressionErrors will:

if (shorthandAssign >= 0) {
  this.unexpected(shorthandAssign); // will throw something
}
if (doubleProto >= 0) {
  this.raise(doubleProto, Errors.DuplicateProto); // will return something
}

I think that the REPL handle both of them similarly. But it seems ambiguous for me. But I probably miss something, let me know in case.

Note: So sorry if my questions are a bit boring 🙏 - I'm just tryin to better understand the code base. Thanks a lot for your time ^^

[nicolo-ribaudo]

Your question is a really good one, actually 😉

@babel/parser has an errorReovery option (which is enabled by default in tests) that makes it push some errors to an ast.errors array. We don't push all of these errors because it's not always easy to build a Babel AST after some invalid syntax.

For example, we could parse ({ foo = 2 }); as if it was ({ foo: 2 }); (so we could recover by replacing = with :), but then what would we do for ({ foo: { x: 3 } = {} });?

1. When it would be hard to create a valid AST that probably reflects the user intention, we still throw (this.unexpected())
2. When it's easy to still create an AST, we use this.raise and continue parsing.

In general, "2" is more similar to linting errors (valid syntax but which is disallowed by the spec via Early Errors), while "1" are actual syntax errors (tokens that aren't allowed in that position).

[tony-go]

Thanks a lot for this level of detail @nicolo-ribaudo ^^

[nicolo-ribaudo]

Thank you for the PR!