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

const constructors can't have asserts #24841

Closed
Hixie opened this issue Nov 5, 2015 · 10 comments
Closed

const constructors can't have asserts #24841

Hixie opened this issue Nov 5, 2015 · 10 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). customer-flutter type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Nov 5, 2015

In Flutter we are trying very hard to make our Widget subclasses all have const constructors, because we want to make that entire class hierarchy be immutable by convention, and the rule "you have to have a const constructor" makes it easy to verify that you didn't screw that up. (It also means that widget subtree descriptions, even relatively elaborate ones, can end up being all evaluated at compile time rather than runtime.)

We are also trying very hard to catch errors in arguments passed to Widget classes as close to the relevant source line as possible. The best way we've found to do this is to have asserts in the constructors. We're checking all kinds of things -- that your arguments aren't null, that they're in the right numeric range, that you have provided either this or that argument but not both (or not neither), that the map you passed in has the important keys we need, etc.

Currently these two desires are in conflict. You can't have any asserts if your constructor is const, even if the assert condition itself is a compile-time-evaluatable const expression.

(Track implementation here: #27141)

@lrhn
Copy link
Member

lrhn commented Nov 6, 2015

First of all, I would not use assert for user-reachable errors. I tend to use assert for checking invariants, and if user chosen parameters can reach and invalidate an assert, it means my code is bugged.
If the problem is the argument, then I'll throw an ArgumentError, and do that in both production and checked mode.

Second, you are using const constructors to enforce final fields, not for actual compile-time construction. That's probably why you have a problem - using const constructors have more effects than just enforcing fields to be final, for example not allowing constructor bodies. If it was just for compile-time construction, it likely wouldn't make sense to do parameter checking, and it's the use of const that is tripping you up. So, in the immortal words of doctor Kronkheit: "Don't do that".

(Another solution would be to allow throwing in potentially compile-time constant expressions, then a "throw not-compiler-time-const-expression" would be a compile time error if reached during const construction, and a runtime throw if reached when called using new.)

@lrhn lrhn added Type-Enhancement area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). labels Nov 6, 2015
@Hixie
Copy link
Contributor Author

Hixie commented Nov 6, 2015

We don't want to do this in production code because these checks can be quite expensive. What we really want is static checks at compile or analyze time, but for some of the checks we really just have to run code (e.g. inspecting maps for consistency, checking trees for duplicates, etc).

We want const for both enforcing final fields and compile-time construction.

We're open to any solution, obviously; compile-time asserts are just the shortest obvious path from what we do now to something that would address our needs:

  • as-early-as-possible (ideally analyzer or compiler) detection of contract violations in constructor arguments.
    • most contracts can be expressed as static checks (e.g. range checks, non-null).
    • some contracts need imperative code to be verified (e.g. tree walks).
    • we don't want to pay the costs for this in production mode (checked mode is ok if necessary).
  • as-early-as-possible (ideally compiler) conversion of immutable data structures into their final form.
    • specifically, Widget instances are large immutable data structures that can be reused frequently (worst case, every frame), and there's a lot of them (maybe hundreds or thousands).
    • for other data types, we usually use const for this.
  • as-early-as-possible (ideally analyzer) detection of Widget subclasses that have any mutable fields.

We're happy to ditch const or assert or classes or anything, just looking for the best solution to the problem!

@Hixie
Copy link
Contributor Author

Hixie commented Feb 18, 2016

To give some context here: where we can use const, we can get upwards of 10x performance improvements over where we can't use const. The only thing stopping us from making huge parts of the Flutter framework const are the fact that we can't put asserts in const constructors.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
@lrhn
Copy link
Member

lrhn commented Aug 15, 2016

Any solution to asserts in const constructors will not allow you to check maps for consistency or trees for duplicates in a const constructor - there is still a "no general code executed at compile-time" restriction.

You can only check that a compile-time constant expression is true at compile-time.

@Hixie
Copy link
Contributor Author

Hixie commented Aug 15, 2016

We're open to any solution, the problem space and our priorities within that space is what is described in my Nov 6, 2015 comment. As you say, pure const asserts wouldn't solve everything in that list. It would solve everything except the second bullet of the first bullet, I think. That's a huge improvement over where we are now. Of course if we can do even more, that would be even better. :-)

@lrhn
Copy link
Member

lrhn commented Aug 24, 2016

I have landed an experimental implementation of assert in initializer lists, including const initializer lists, for the VM. It is guarded behind a flag --assert-initializer.

To make it really useful, you probably need to wait for the analyzer to recognize the syntax (#27142).

@eseidelGoogle
Copy link

The analyzer bug is #27142 (for anyone else following along at home)

@sethladd
Copy link
Contributor

sethladd commented Jan 6, 2017

Tracking issue for implementation: #27141

@sethladd
Copy link
Contributor

This is done, yes? Can we close this?

@munificent
Copy link
Member

There's still some work to do for all of our tools to support this (#30968) but I'm going to go ahead and close this now since Flutter has what it needs and we have a tracking bug for the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). customer-flutter type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants