Skip to content

analyzer: int2double doesn't work in const contexts #35441

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
MichaelRFairhurst opened this issue Dec 18, 2018 · 7 comments
Closed

analyzer: int2double doesn't work in const contexts #35441

MichaelRFairhurst opened this issue Dec 18, 2018 · 7 comments
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@MichaelRFairhurst
Copy link
Contributor

This was thought to be #33441, but it turns out its its own issue:

const double x = 1; // inferred as 1.0
class C {
  const C() : assert(x is double); // false
}

The cause for this is actually very simple. The visitor that evaluates an AST node for const evaluation, does not look at the node's staticType at all, and simply creates an integer. This made sense when integer expressions were always integers.

It's not a bigger issue, like inference not having been run, as we thought behind #33441. It's really easy.

We have @failingTests for this in analyzer already.

@MichaelRFairhurst MichaelRFairhurst added legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Dec 18, 2018
@bwilkerson
Copy link
Member

Partially fixed by https://dart-review.googlesource.com/c/sdk/+/87607, but still failing some cases.

@MichaelRFairhurst
Copy link
Contributor Author

I just ran into something really weird

This now passes:

const C(double x) : assert(x is double);

where it didn't before.

But this still fails:

const double x = 0;
const C() : assert(x is double);

@bwilkerson
Copy link
Member

The information needed by the first example is completely local. The information for the second isn't, and hence depends (IIRC) on having built a dependency graph so that x is evaluated before the initializer. I wonder whether the graph might not be getting built correctly for asserts in initializer lists.

@lrhn
Copy link
Member

lrhn commented Dec 19, 2018

In either case is checks are not allowed in const contexts (yet), and th eexpression of a const constructor assert must be a potentially constant expression.
So, this should not compile anywhere.

@stereotype441 stereotype441 added the P2 A bug or feature request we're likely to work on label Dec 19, 2018
@MichaelRFairhurst
Copy link
Contributor Author

@lrhn there are other ways to reproduce the problem:

const double x = 0;
const C() : assert("$x" == "0.0");

However, these are just symptoms of the real problem, which is that x was given the wrong type.

Not only does it make it easier and more reliable to test this by turning on the const updates, it also makes it more important to have fixed before the new const behavior is released. So its all related.

dart-bot pushed a commit that referenced this issue Jan 5, 2019
Change-Id: I9505598e904e4ee39b00cbae87082f5f332f3505
Reviewed-on: https://dart-review.googlesource.com/c/87612
Commit-Queue: Mike Fairhurst <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@stereotype441
Copy link
Member

@MichaelRFairhurst can we mark this as fixed now?

@MichaelRFairhurst
Copy link
Contributor Author

Yes, thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants