Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fe-analyzer-shared] Glitch in initializerExpression parse #54284

Open
eernstg opened this issue Dec 8, 2023 · 6 comments
Open

[fe-analyzer-shared] Glitch in initializerExpression parse #54284

eernstg opened this issue Dec 8, 2023 · 6 comments
Assignees
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. cfe-parser Parser issues in the CFE

Comments

@eernstg
Copy link
Member

eernstg commented Dec 8, 2023

Thanks to @modulovalue who brought up this behavior here. Consider the following program:

var x = throw 0..isEven; // OK.

class A {
  var x;
  A() : x = throw 0..isEven; // Syntax error.
}

void main() {}

This program causes the analyzer and the CFE to report a syntax error for the <initializerExpression> in the constructor, whereas the initializing expression in the top-level variable declaration is accepted.

The non-terminal <initializerExpression> has for a long time been somewhat vaguely defined because we have a rule that it cannot be a function literal. That rule was never defined precisely. (However, a step in that direction is being taken here.)

It is possible that the error above is caused by the use of <throwExpressionWithoutCascade> rather than <throwExpression>, but unless there is a specific reason to make that choice, I'd recommend that the throw expression is parsed using the "normal" non-terminal for that purpose, that is, <throwExpression>.

@jensjoha, perhaps you could say something about some of the questions related to this topic? For example:

  • How is the parsing of an <initializerExpression> and/or an <initializerList> performed, and how is it enforced that an <initializerExpression> cannot be a function literal?
  • Does the parser actually use <throwExpressionWithoutCascade> rather than <throwExpression> in an initializer list?
  • If it is the former, would it create any particular difficulties to switch over and use <throwExpression>?
@modulovalue
Copy link
Contributor

One observation that is relevant to this issue:

Consider:

class Foo {
  Foo() : a = () {}, b = 0;
}

While that program is rejected by analyzer, but not by DSP, (and it seems like analyzer exhibits intended behavior), the following is accepted by both, analyzer and DSP:

class Foo {
  Foo() : a = <T>() {}, b = 0;
}

Also, consider:

class Foo {
  Foo() : a = <T>() => 0;
}

Without <T>, analyzer and DSP both agree. With <T>, DSP rejects that program and analyzer accepts it.

@jensjoha
Copy link
Contributor

@jensjoha, perhaps you could say something about some of the questions related to this topic? For example:

  • How is the parsing of an <initializerExpression> and/or an <initializerList> performed, and how is it enforced that an <initializerExpression> cannot be a function literal?
  • Does the parser actually use <throwExpressionWithoutCascade> rather than <throwExpression> in an initializer list?
  • If it is the former, would it create any particular difficulties to switch over and use <throwExpression>?

When parsing initializers (parseInitializersOpt -> parseInitializers -> parseInitializer), if it finds identifier '=' it parses the whole thing as an expression (parseInitializerExpressionRest -> parseExpression) meaning that it goes parsePrecedenceExpression ( -> parseUnaryExpression) -> _parsePrecedenceExpressionLoop with ASSIGNMENT_PRECEDENCE where it calls parseThrowExpression(next, /* allowCascades = */ false).

Said more directly, perhaps, it (tried to) parse x = throw 0..isEven as an expression. As such we get the same behavior if we for instance do

void foo() {
  var x;
  print(x = throw 0..isEven);
}

(In both cases parenthesizing the throw is a workaround).

Whereas for the field initializer it calls parseExpression on part after the equal sign, i.e. tries to parse throw 0..isEven as an expression (which directly calls parseThrowExpression(token, /* allowCascades = */ true)).

As for disallowing function literals there's a variable mayParseFunctionExpressions which is set to false in parseInitializers. (although again parenthesizing the thing will work around that).

Wrt allowing it --- it seems a little weird that parseThrowExpression(next, /* allowCascades = */ false) is just called with false when there's actually a variable called allowCascades --- shouldn't we use that?
It was seemingly introduced in https://dart-review.googlesource.com/c/sdk/+/60600

I'll try to make that change and run the bots to see what happens.

copybara-service bot pushed a commit that referenced this issue Nov 11, 2024
…ePrecedenceExpressionLoop instead of just false

For now, see what happens.

Bug: #54284
Change-Id: I8e9f081ae3aa5caff7e155ef628e90547cc8afe6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/392561
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>
@jensjoha
Copy link
Contributor

Is there anything more to do here?

@eernstg
Copy link
Member Author

eernstg commented Nov 18, 2024

I still see this behavior (DartPad based on Dart SDK 3.7.0-123.0.dev and Flutter SDK 3.27.0-1.0.pre.438):

var x = throw 0..isEven; // OK.

class A {
  var x;
  A() : x = throw 0..isEven; // Syntax error.
}

void main() {}

The initializing <expression> in the top-level declaration should be derivable (so it's treated correctly today):

<expression> ->
<throwExpression> ->
'throw' <expression> ->
'throw' <cascade> ->
'throw' <conditionalExpression> '..' <cascadeSection> ->
'throw' '0' '..' <cascadeSelector> <cascadeSectionTail> ->
'throw' '0' '..' 'isEven'

However, we have the following in the constructor, where an <initializerExpression> must derive the same tokens:

<initializerExpression> ->
<throwExpression> -> /*... same as above ...*/ ->
'throw' '0' '..' 'isEven'

There is nothing in this derivation that brings up the special rule that "an <initializerExpression> cannot derive a <functionExpression>". I can't see any other conclusion than this: The syntax error which is being reported at the <initializerExpression> is wrong.

In particular, the following differs so much from the grammar that it will inevitably cause some parsing outcomes that don't match the grammar:

When parsing initializers ..., if it finds identifier '=' it parses the whole thing as an expression

@jensjoha
Copy link
Contributor

From the tags on 0512c58 it is first included in 3.7.0-130.0.dev.

@eernstg
Copy link
Member Author

eernstg commented Nov 18, 2024

OK, so I was checking the situation with a tool whose version was too low, sorry about that.

However, @modulovalue mention a few other cases:

class Foo1 {
  final Function a;
  final int b;
  Foo1()
      : a = <T>() {},
        b = 0;
}

class Foo2 {
  final Function a;
  Foo2() : a = <T>() => 0;
}

Both of these declarations allow the <initializerExpression> to derive a <functionExpression>, which should not occur.

This behavior is different from the specified behavior, but it could be argued that it is benign. See dart-lang/language#4163.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. cfe-parser Parser issues in the CFE
Projects
None yet
Development

No branches or pull requests

4 participants