dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Parsing in tools and spec disagree #26602

Closed eernstg closed 4 years ago

eernstg commented 8 years ago

According to the grammar fragments in the language specification (v. 1.11), we can derive as follows (where A stands for a construct that we need not worry about because it may be omitted or it is one or more unchosen alternatives):

  fieldInitializer ::= A? identifier '=' conditionalExpression A*
  conditionalExpression ::= ifNullExpression A?
  ifNullExpression ::= logicalOrExpression A*
  logicalOrExpression ::= logicalAndExpression A*
  logicalAndExpression ::= equalityExpression A*
  equalityExpression ::= relationalExpression A? | A
  relationalExpression ::= bitwiseOrExpression A? | A
  bitwiseOrExpression ::= bitwiseXorExpression A* | A
  bitwiseXorExpression ::= bitwiseAndExpression A* | A
  bitwiseAndExpression ::= shiftExpression A* | A
  shiftExpression ::= additiveExpression A* | A
  additiveExpression ::= multiplicativeExpression A* | A
  multiplicativeExpression ::= unaryExpression A* | A
  unaryExpression ::= postFixExpression | A
  postFixExpression ::= .. | primary (A* | A)
  primary ::= functionExpression | A
  functionExpression ::= formalParameterList functionBody

This shows that a fieldInitializer can be x = () { y = 3; }.

Yet, the following program is rejected during parsing:

var y;

class A {
  var x;
  A() : x = () { y = 3; };
}

main() => new A();

This applies to dart, dart2js, and dartanalyzer, all in version 1.17.0-dev.6.0.

floitschG commented 8 years ago

Since all tools agree, this should maybe be addressed by the spec, disallowing it.

bwilkerson commented 8 years ago

I couldn't find any text in the spec explaining why, but analyzer has an explicit test in order to disallow exactly this case, with the following un-helpful comment:

    // Function expressions aren't allowed in initializer lists.
lrhn commented 8 years ago

I believe the problem is that the grammar is harder to parse if you allow unparenthesized function literals.

   C(y) : x = (y) { print(y); };

You can't see whether x is assigned a the value of variable or a function literal until you see the final ;. I don't think the grammar is actually ambiguous. A constructor needs a final body or a semicolon, and the initializer list continues on a comma, so looking after the potential function body always tells you whether it is a function body or a constructor body. You just can't do it with one-token look-ahead.

We should either specify the restriction: initializer list expressions must not contain something that looks like a constructor body - however we say that, fx anything is allowed if it's parenthesized because then we know it's not the constructor body, so it's not just "no functions expressions in initializers".

Alternatively we just accept the current grammar and make the parsers match it. I think the VM parser may be the biggest challenge because they try hard to be look-ahead-1. Dart2JS should not have a problem since it matches braces while tokenizing so it can quickly check the token after the closing brace before determining the next step. If we switch to a single front-end, then we only need to fix it in one place.

bwilkerson commented 8 years ago

@lrhn Thanks for reminding me why this was disallowed.

Analyzer also does brace matching during tokenization, and could use the same technique. However, allowing this would also have implications for recovery in the face of certain errors (such as a missing semicolon). I don't believe that the problem is insurmountable, but we should take it into account when deciding what to do.

munificent commented 7 years ago

Speculatively moving this to area-specification on the hunch that what we already implemented is what we want and that we should tweak the spec to follow it. If we end up deciding otherwise, we can retarget this.

eernstg commented 6 years ago

No milestone now: This correction is part of a larger update of grammar related parts of the specification, based on Dart.g.

eernstg commented 4 years ago

Closing, the language specification is being updated to include the restriction in this PR: https://github.com/dart-lang/language/pull/866.