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

Can't initialize constants from other constants #29098

Closed
Hixie opened this issue Mar 17, 2017 · 9 comments
Closed

Can't initialize constants from other constants #29098

Hixie opened this issue Mar 17, 2017 · 9 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Mar 17, 2017

class Baz {
  const Baz();
}

class Bar {
  const Bar(this.baz);
  final Baz baz;
}

class Foo {
  const Foo(this.bar);
  final Bar bar;
}

const Foo foo = const Foo(const Bar(const Baz()));

const Baz baz = foo.bar.baz; // <-- why doesn't this work? 😡

void main() { }
@lrhn
Copy link
Member

lrhn commented Mar 17, 2017

This doesn't work because foo.bar is not a compile-time constant expression.

The reason we don't make foo.bar a compile-time constant expression, when foo is a compile-time constant expression and bar is a final field, is that it breaks field/getter symmetry.
We can't make a getter invocation a compile-time constant expression (it executes code!), so if you rewrote Foo to

class Foo {
  final Bar _bar;
  const Foo(Bar bar) : _bar = bar;
  Bar get bar { log("Bar accessed"); return _bar; }
}

then it would break the client code. This change would now be a breaking change, even if it doesn't change the API of the class at all.

We want that rewrite to be valid and non-breaking, so we can't just make final field access a compile-time constant expression.

So, what if we allowed you to mark fields as constant?

class Foo{ 
  const Bar bar;
  const Foo(this.bar);
}

It'd just be a final field, but with a marker that says that it can be used in a constant expression.

At that point, we could make access to bar a compile-time constant expression because the author of the class has opted into it and documented that it is a breaking change to make it non-const.

The disadvantage is the cognitive overhead. Whenever you make a const class, you must now consider, for every field, whether you want to make it constant. The safe default is to not make it const, because you can always change it to const in the future, or you can change it to a getter in the future. Making every final field const will lock you into the current design. Also, the next request will likely be "const getters" which can only contain potentially constant expressions, just to be able to get out of the corner that some authors will have painted themselves into.

It's all definitely doable, but it's not clear that it's worth its own complexity. So far we have chosen not to do it.

@lrhn lrhn closed this as completed Mar 17, 2017
@lrhn lrhn added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug labels Mar 17, 2017
@Hixie
Copy link
Contributor Author

Hixie commented Mar 17, 2017

For what it's worth, this causes problems in Flutter at the moment. We want as many of our widgets to be instantiated const as possible, because it results in dramatic performance improvements (10x in some cases, because it allows us to recognise that an entire subtree hasn't changed and we can therefore shortcircuit huge tree walks). Some of these widgets are configured from the fields of other widgets. Unfortunately, even though everything is const and known at compile time, we can't actually get the data from A to B because it goes through a field accessor and this bug therefore blocks it.

This change would now be a breaking change

Everything is a breaking change. Adding a field is a breaking change (someone might have implementsed your class). Adding an assert is a breaking change (it might fire). Marking something as @deprecated is a breaking change (it has broken our build when our dependencies do that). Changing the whitespace in a comment is a breaking change (because it increases the time to download the file to just above a timeout threshold, or because someone is using substring to parse your source code to generate theirs).

Changing a field to a getter is a breaking change even without allowing const access through a getter, because it changes the behaviour of the member. I don't really buy this as an argument. Field/getter symmetry is already not a thing in practice.

@lrhn
Copy link
Member

lrhn commented Mar 17, 2017

There are breaking changes and breaking changes. Not everything is a breaking change, or even potentially a breaking change. Refactoring is exactly about doing semantics preserving restructuring. It's based on some changes being safe and non-breaking.

There are things that clients of your code can reasonably depend on, and things that are not reasonable (unless you go out of your way to promise them anyway).

You can always change the behavior of your class, and then you are responsible for the breakage. if your class is intended for subclassing, or for use as a mixin, there are things you can no longer change without breaking clients that are using your class as intended.

If you break someone who depends on the whitespace of your code, they are responsible. I'm not saying it's not annoying, but you couldn't reasonably be expected to predict that.

Changing a field to a getter or vice versa, while keeping the visible behavior, is not a breaking change. That is a quite deliberate choice in the Dart language. It means that you can evolve your class without locking yourself into a specific implementation. When you can't do that, you get something like the Java "always have a private field and public get/set methods" coding style. We are trying to avoid that.

We definitely do not want to make all final fields of const classes locked into being fields.
Adding const instance fields is a possible solution, and I'm sure we'll be reconsidering it again.

@Hixie
Copy link
Contributor Author

Hixie commented Mar 17, 2017

I'm not convinced that there's a difference between a breaking change that breaks something documented as unstable and a breaking change that breaks something implied to be stable due to some language policy. In either case, your client is broken. In either case, you risk losing them.

@lrhn
Copy link
Member

lrhn commented Mar 17, 2017

You do have a point. That is why it is so important to know which changes are not breaking changes in any way. Switching between a getter and field (with the same visible semantics, obviously) is always a non-breaking change. That's a good thing.

@Hixie
Copy link
Contributor Author

Hixie commented Mar 17, 2017

Switching between a getter and field (with the same visible semantics, obviously) is always a non-breaking change.

It's not, actually.

Take this code:

class A {
  final int _x = 1;
  int get x => _x;
}

class B extends A {
  @override
  int get x => super.x + 1;
}

void main() {
  print(new B().x); // prints 2
}

...which compiles cleanly, executes cleanly, and has no analyzer warnings.

Now remove the getter and replace it with a field. The analyzer fails with a STRONG_MODE_INVALID_FIELD_OVERRIDE error.

class A {
  final int x = 1;
}

class B extends A {
  @override
  int get x => super.x + 1; // STRONG_MODE_INVALID_FIELD_OVERRIDE
}

void main() {
  print(new B().x);
}

@lrhn
Copy link
Member

lrhn commented Mar 18, 2017

Yes, that is a problem with strong mode (#28117). It'll be fixed.
It is considered a problem exactly because it breaks the getter/field symmetry.

@Hixie
Copy link
Contributor Author

Hixie commented Mar 18, 2017

Another way this could be solved, rather than the const field issue, is by allowing compile-time-evaluatable functions to be considered const. So for example if a getter just returned a field, it could be const. That would solve many other problems too, for example it would allow const asserts to do much more elaborate tests, and would allow const Widgets to be created from lists via List.generate, etc.

@lrhn
Copy link
Member

lrhn commented Mar 18, 2017

I'm sure we can extend const more, but not without cost. We could make some functions markable as const, basically those where the body is just returning a potentially constant expression. Again, it's not something that should happen automatically, you need to opt into being const-usable, otherwise changing the function implementation will be a breaking change.

I don't think List.generate will ever be considered const (it contains a loop - loops and function calls are the two things that we can't generally treat as compile-time constant because we can't determined whether they will terminate).

It's a trade-off, though. Making more expression const, just because we can, isn't a goal in itself. Keeping it simple, understandable and explainable is also a goal.

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). closed-not-planned Closed as we don't intend to take action on the reported issue type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

2 participants