Skip to content

Unsound treatment of label: break label; in type/flow analysis #50294

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

Closed
lrhn opened this issue Oct 25, 2022 · 3 comments
Closed

Unsound treatment of label: break label; in type/flow analysis #50294

lrhn opened this issue Oct 25, 2022 · 3 comments
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on soundness type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@lrhn
Copy link
Member

lrhn commented Oct 25, 2022

Take some ridiculous code like:

void main() {
  int x = foo();
  print(x.runtimeType);
}

int foo() {
  num x = 1.5;
  if (x is! int) {
    label: break label;
    print("Gotcha");
  }
  return x;
}

This code compiles and prints Gotcha and double.

If I change label: break label; to the supposedly completely equivalent label: { break label; }, it stops compiling, because the flow analysis correctly deduces that label: { break label; } can terminate normally.

The label: break label; is mistakenly seen as not terminating. Code immediately after it is considered dead code by the analyzer. Type promotion assumes it never completes.

This affects both analyzer and front-end.

(Guessing we are doing something special with lables, maybe storing them on the inner statement's AST node instead of making a labeled statement a proper composite statement with its own AST node, which makes it much easier to forget that they can be on any statement. Especially since it only matters for composite statements which can contain a break or continue. Which includes break statements.)

(Found while looking at dart-lang/language#2586).

@lrhn lrhn added legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) legacy-area-front-end Legacy: Use area-dart-model instead. labels Oct 25, 2022
@srawlins srawlins added P3 A lower priority bug or feature request dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec labels Oct 25, 2022
@johnniwinther
Copy link
Member

cc @stereotype441

@stereotype441
Copy link
Member

Yikes! The language team generally considers soundness bugs to be pretty high priority. It's pretty unlikely that anyone would write code like this in practice (though maybe, I don't know, a code generator might). So I'm escalating this to P2.

@stereotype441 stereotype441 added P2 A bug or feature request we're likely to work on and removed P3 A lower priority bug or feature request labels Nov 7, 2022
@asashour
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/277400

For the CFE and analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on soundness type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

5 participants