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

Infer final fields as const for const constructors #299

Open
rrousselGit opened this issue Apr 2, 2019 · 26 comments
Open

Infer final fields as const for const constructors #299

rrousselGit opened this issue Apr 2, 2019 · 26 comments
Labels
enhanced-const Requests or proposals about enhanced constant expressions

Comments

@rrousselGit
Copy link

👋 Here's a language feature request (should it be moved to https://github.com/dart-lang/language?)

Consider the following const classes:

class Bar {
  const Bar();
}

class Foo {
  const Foo();

  final Bar bar = const Bar();
}

This proposal is to allow the following:

const foo = Foo();
const bar = foo.bar;

Currently, it is impossible to assign foo.bar to a const variable and triggers a

Const variables must be initialized with a constant value.

@kevmoo kevmoo transferred this issue from dart-lang/sdk Apr 3, 2019
@munificent
Copy link
Member

This comes up a lot, but it's actually not sound to do. Consider:

class Bar {
  const Bar();
}

class Foo {
  const Foo();

  final Bar bar = const Bar();
}

class OtherFoo implements Foo {
  const OtherFoo();

  Bar get bar => new Bar(); // Definitely not a constant.
}

const Foo foo = OtherFoo();
const bar = foo.bar;

In the absence of sealed classes or non-virtual fields, just because a field is final, that doesn't mean there aren't other implementations of that class's implicit interface where the field's getter is non-const.

@rrousselGit
Copy link
Author

rrousselGit commented Apr 5, 2019

Fair enough, but I disagree.

It's not about allowing all final fields to be assignable to const — but inferring that such field is indeed a const.

To take your example, since a const instance is known at compile time, we know if it is a Foo or OtherFoo instance.

And as such, we know if its field bar points to a constant or a overridden getter.

As such when doing:

const Foo foo = OtherFoo();
const bar = foo.bar;

Then we are able at compile time to know that the second line is invalid and the compilation can safely fail.

@eernstg
Copy link
Member

eernstg commented Apr 5, 2019

@rrousselGit wrote:

we are able at compile time to know

I think @munificent's point is actually very well taken, but we could take a slightly longer detour around language design pragmatics in order to see why we may not want to promise that "we will be able to know" such things. ;-)

It would indeed be possible to eke out additional bits of expressiveness in the language of constant expressions without violating the basic rules, but each of the steps that we might take in order to do so is fraught with complexity. That's for developers, who are trying to do something and who are wondering whether there is a way to trick the compiler into accepting it; or for implementers of the Dart tools who need to support all the corners of a complex semantic space and never allow programs to step outside that space; and even for language specification folks like me who need to find a robust and correct way to say what the outer limits of that complex semantic space is.

In short, it is not obvious to me that the Dart community is better served by having a very complex specification in this area, even though the extra bits of expressiveness may be convenient to have in concrete situations.

That said, it should be noted that we keep getting requests for more expressive constant expressions, and the boundary has been pushed out a little bit several times, even as recently as late 2018 (parts of which is currently under implementation).

@rrousselGit
Copy link
Author

rrousselGit commented Apr 5, 2019

Agreed.
Such inference also implies that refactoring a final Foo foo to Foo get foo for classes with a const constructor would officially be a breaking change (it already is, but less visible).
This may be unwanted complexity.

But the assumption we make is right – and the analyzer possesses all the bits needed to check that we indeed are right.

So it feels weird not being able to – especially since some aspects of Dart already use this feature of consts, like code-generators.

When we do:

@JsonSerializable(nullable: false)
class Foo {}

Our code-generator will statically read the content of our annotation and read its fields to act accordingly.

@eernstg
Copy link
Member

eernstg commented Apr 5, 2019

I think we'll just have to accept that this is a delicate area, and we will probably keep pushing the borderline around a little bit now and then. ;-)

@munificent
Copy link
Member

To take your example, since a const instance is known at compile time

Ah, that's an interesting point! We could potentially actually look up the member at compile time to see what declaration it resolves to since we know the actual receiver object because it's constant. I'm a worried there are some circularity problems here, but maybe it would work.

I'll reopen this.

@munificent munificent reopened this Apr 5, 2019
@lrhn
Copy link
Member

lrhn commented Apr 29, 2019

The "making a field into a getter is a breaking change" problem is the biggest problem in my perspective.
For that reason alone, I'll never allow implicitly making an instance field access a const expression.

If you could do so explicitly, then it's a whole different case. Say:

class C {
  const String name;
  const C(this.name);
}
...
const C something = C("foo");
...
const somethingName = something.name;

Here we only allow "const access" to fields that are declared to be usable in constant contexts. That's an explicit and deliberate opt-in from the author's side, and they have now made it a breaking change to make the field non-const. In every other way, it's still just a final instance varible.

That still makes it problematic to override the const instance field. If a subclass can override it with a getter, then we are back in Bob's original example where const c = const D(); const name = c.name; looks perfectly reasonable, and still fails. At least it fails at compile time, but we have to create the object and check its name getter to see that it's not a const instance variable.
We could say that any const-capable class (one with a const constructor) which implements the API must make the field constant accessible, but imagine the situation:

class C {
  const String name;
  const C(this.name);
}
class D {
  String _nameOverride;
  String get name =>  _nameOverride || super.name +" Doe";
  void set name(String name) { _nameOverride = name; }
  D(String baseName) : super(baseName);
}
class E implements D {
  String get name => _name[this] ?? "Jessica Pidgeon";
  void set name(String name) { _name[this] = name; }
  const E();
  static Expando<String> _name;
}
const C gotcha = E();
const String gotchaName = gotcha.name;

Here E implements the non-const D. Since D is non-const, there is no reason to require its name getter to be constant. Heck, you can add a setter too.
But then E also shouldn't need to make name constant, and yet we use it in a place where we think (from looking at the available interface) that name is a constant getter.

We currently have one const field in the language: String.length. There is no annotation on the String interface, and likely wouldn't be even if we have constant fields, because the actual implementation is hidden in private subclasses. Each of those can make length constant without it being visible in the String API itself. If we imagine some other non-platform class doing the same, will we allow const String x = something; const int len = x.length;? There is nothing in the API that suggests this is correct, but the actual run-time value will support it. This suggests (perhaps) that we should allow any member access in a const expression, and check per object whether it succeeds or not.

Or, in short, "const-ness" of a field is not properly an interface property, it's a property of individual objects. That makes it very hard to reason about whether a field is actually constant or not. It makes it hard to document, and to find documentation for, unless we say that a const field must only be overridden or implemented using another const field. That's a very strong restriction, and one that makes no sense for non-const classes.

(We could also introduce constant getters, which are restricted to returnning expressions containing potentially constant expressions where this is considered potentially constant, then you can override a constant field with a getter, as long as it's a constant getter, but that's still restrictive).

@rrousselGit
Copy link
Author

rrousselGit commented Jun 3, 2019

As a comeback on this, what about a keyword for explicitly preventing against overriding properties of a class?

Such a feature would unlock multiple type inference abilities, including but not limited to this request.

Outside of consts, another use-case would be is inference.
Currently, we can do:

if (foo is Foo) {
  // `foo` is infered as type `Foo`.
}

But we can't do the same for properties inside foo. The following won't work:

if (foo.bar is Bar) {
  // `foo.bar` is **not** infered as type `Bar`.
}

That could translate into:

strict class Foo {
  Foo(this.bar);

  final Bar bar;
}

class Subclass extends Foo {
  @override
  Bar get bar => super.bar; // compile error, property `bar` cannot be overriden
}

@lrhn
Copy link
Member

lrhn commented Jun 3, 2019

For the cast to be allowed, we need to know that

  1. It is a field (not a getter),
  2. the field is final, and
  3. the field cannot be implemented by any other class (or at least as anything not also a final field).

That will ensure that reading the value twice will always provide the same value, which is what is needed for a test in one place to say anything about a read in another place.

The last requirement is the tricky one. It obviously prevents subclassing from overriding the field, but it also prevents any class from implementing the interface (at least without also declaring the property as a final field). That definitely prevents mocking. You cannot mock a class with such a field (at least without declaring a final field on the mock, which means that that field bypasses the mocking logic of a library like mockito).

If we allow such restrictions, then we have effectively introduced a way to prevent a class interface from being implementable, while still being subclassable. If we do that, we should probably add that as a feature directly, rather than having people add private sealed fields to avoid implementation. (I guess a private field would prevent it, but it's bad for usability when you can't see anything in the public API, and it still affects your ability to implement the interface).

@rrousselGit
Copy link
Author

rrousselGit commented Jun 3, 2019

Good point about mockito, I didn't think about that one.

As for the classes that can be extended but not implemented, there are quite a few examples on Flutter already, including State and Element.

@eernstg
Copy link
Member

eernstg commented Jun 3, 2019

I think an enhancement in this area wouldn't induce a huge amount of complexity, should we dare to explore it. Here's a rule that we could use:

An expression of the form c.id is a potentially constant expression if c is a potentially constant expression whose static type is dynamic or has a getter named id (declared explicitly, or implicitly induced by an instance variable), and it is furthermore a constant expression when c is a constant expression that evaluates to an instance of a class C that has a final instance variable named id.

This mechanism maintains the invariant that "no new constant objects are created" in the initializer list of a constant constructor, which means that we maintain the protection against a constant pool that explodes in size. A current example of the same constraint is that we don't allow const [x] and const D(x) when x is a formal parameter, but we do allow it when x is a constant variable. In particular, we would still reject [x.bar] when x is a parameter, but we would allow this:

class Bar {
  const Bar();
}

class Foo {
  final Bar bar = const Bar();
  const Foo();
}

class Foo2 {
  final Bar bar;
  const Foo2(Foo foo): bar = foo.bar;
}

const foo = Foo();
const bar = foo.bar;

We could allow c.id without checking the type of c (we will always detect the problem if the value of c doesn't actually have a final instance variable named id), but I'd prefer to apply static checks as much as possible. We may or may not include the "dynamic loophole", but I included it because we have a number of similar situations where dynamic is allowed. We would presumably support the elimination of those situations via lints.

In other words, I think we can introduce a mechanism as requested in this issue in a way which is not very complex, and which allows us to maintain the current level of protection against large constant pools.

The trade-off is that a constant constructor like the one in Foo may compile with no errors/warnings/hints, and a subtype SubFoo of Foo may exist and override bar with a getter, still no errors/warnings/hints, and then we suddenly get an error at const bar = foo.bar; because we have const Foo foo = SubFoo();.

I think it could be an OK trade-off, because those errors will all arise at compile-time, and I'm not convinced that the ability to say that "this final field cannot be overridden by a getter, but another final field is fine" will carry its own weight. Of course, that could again be a lint, maybe triggered by @keepConstant or @noExplicitGetter.

@lrhn
Copy link
Member

lrhn commented Jun 4, 2019

The problem with that is that it makes it a breaking change to change a final variable in a const class to a getter. Someone, somewhere, might be doing const C(x).id, and whether you wanted it or not, or even considered it or not, you are now stuck with keeping id as a final instance variable.

The workaround is to declare all your final fields private and have public getters for them. That's an awful workaround to force on people, so it's clear (at least to me) that the defaults are wrong here. Doing the simple thing is dangerous and costly in the long run.

I don't want to lock the author into that without their explicit consent, which is why I'd, at a minimum, require the instance variable to be marked as const.
Now, if a class declares a const variable id, it means that any class implementing that interface and having a const constructor must also have id as a const variable.

We can then, perhaps, later add "constant instance getters" which are getters with "potentially constant" arrow-bodies, with expressions that are constant if this is considered a constant.

Nobody gets locked into something by omitting a marker, only by explicitly opting in to it.

@eernstg
Copy link
Member

eernstg commented Jun 4, 2019

@lrhn wrote:

Now, if a class declares a const variable id, it means that any
class implementing that interface and having a const constructor
must also have id as a const variable.

That makes sense, and doesn't seem to be too hard.

@rrousselGit
Copy link
Author

rrousselGit commented Jun 4, 2019

Wouldn't that be confusing to use the const keyword though?

Because technically we can have methods:

class Foo {
  const Foo(this.foo);

  const String foo;

  void build() {
    const bar = foo; // that shouldn't be possible
  }
}

@lrhn
Copy link
Member

lrhn commented Jun 4, 2019

@rrousselGit
That would precisly not be allowed because the unqualified foo here is equivalent to this.foo, and this is not constant.

It may be slightly confusing.

We have required you to write static const ... to declare constant variables for all this time, even though there is no non-static constants, because it kept the door open for constant instance variables.

@eernstg
Copy link
Member

eernstg commented Jun 4, 2019

@rrousselGit wrote:

confusing to use the const keyword

What we need to say is that for a given getter named id in a class C, every subtype D of C that has a generative constant constructor must satisfy the constraint that the implementation of id is implicitly induced by an instance variable (such that we know that it won't execute code).

That instance variable is guaranteed to be final (or we couldn't have a constant constructor for D), but D could declare final T id itself or inherit it, that shouldn't matter.

So if we want to express that constraint on a getter or final variable then we could consider this member to carry a promise that "it must be possible to use this getter in a constant expression".

The keyword const might not be a very good choice for saying that, because it usually means that "this expression is constant" or "this declaration declares a constant". So in order to indicate clearly that this is a new concept I'll use as const after the declaration.

abstract class C {
  // C doesn't have to have a constant constructor, but it could have some.
  // Without a constant constructor `id`, `id2` have no special properties,
  // but they introduce constraints on each subtype that has a constant constructor.
  int get id as const;
  final id2 as const;
}

class D implements C {
  final int id; // OK. We might allow/require `as const` as documentation.
  const D(this.id);
}

class D2 implements C {
  int get id { ... } // OK, no special constraints apply in a non-constant class.
}

class D3 implements C {
  int id => someExpression; // Error.
  const D3();
}

const x = const D(42).id; // OK.
const x2 = (const D(42) as C).id; // Also OK.

Returning to the example, @lrhn already mentioned that this is not a constant expression, and the rest of the example would just be modified slightly:

class Foo {
  final String s as const;
  const Foo(this.s);
}

const foo = const Foo("bar");
const bar = foo.s; // OK.

@eernstg
Copy link
Member

eernstg commented Jun 4, 2019

Looks like it should be possible to support this kind of constant getter. But is it actually needed? Do we want to pay for it in terms of the added complexity?

@ds84182
Copy link

ds84182 commented Jun 4, 2019

I'd certainly prefer the const Type identifier; syntax. A constant field would expose a getter, like a normal field, but it would also not allow a setter and a non-constant override for implementers. For instances constructed at runtime, the constant field would act like a final field.

class Base {
  const String foo;
  const Base(this.foo);
}

class Implementor implements Base {
  const String foo; // OK
  String get foo => ...; // ERROR
  String foo; // ERROR
  set foo(String x); // ERROR
  final String foo; // WARNING (for migration purposes, can be changed to const without breakages)
}

I don't know if it's worth to allow const fields to be initialized inline (like const String foo = "bar";), it might lead to some confusion between static const and instance const.

@ds84182
Copy link

ds84182 commented Jun 4, 2019

Furthermore, since const instance variables are known not to change (regardless of subclasses and implementations), the Analyzer can allow promotions for fields, and Dart compilers can optimize loads (since they're guaranteed to not have side effects).

class ObjectBox {
  const Object value;
  const ObjectBox(this.value);
}

void foo(ObjectBox box) {
  if (box.value is String) {
    print(box.value.trim());
  }
}

Related issue for field promotion: dart-lang/sdk#35525

@ds84182
Copy link

ds84182 commented Jun 4, 2019

You can also override existing fields with a const field, as long as there's no setter involved:

abstract class Base {
  int get foo;
}

class Derived extends Base {
  const int foo;
  const Derived(this.foo);
}

@rrousselGit
Copy link
Author

I still believe that, at their core, const member and type promotion for members using is are the same issue.

If we can prevent subclasses from overriding with a getter, then it'd allow both:

const foo = bar.baz;

And:

if (foo.bar is Bar) {
  // foo.bar is inferred as Bar
}

@eernstg
Copy link
Member

eernstg commented Jun 6, 2019

@ds84182 wrote:

I'd certainly prefer the const Type identifier; syntax

Right, that is indeed more familiar.

However, the reason why I proposed a different syntax is that the const Type identifier; syntax suggests that said instance field is constant, and that is not likely to be true.

For instance, when you can do const C(some, constant, expressions) you can also do new C(some, nonconstant, expressions), in which case nonconstant could yield the value of some instance variable id which is final and which is intended to allow for constant getter, but in that case a declaration of the form const T id; or const id; would be directly misleading.

We could of course say that this particular kind of class (the ones that have at least one const Type identifier; declaration) does not admit dynamic creation. But that's a non-trivial step, and we may or may not want to do that. Moreover, it certainly seems wrong to put that kind of constraint on a class based on one of its member declarations, so we'd probably want to allow a class to be declared as const class C ... and then it would be known for the class as a whole that no dynamic instance creation is possible; and only then would const Type identifier; be allowed.

@lrhn
Copy link
Member

lrhn commented Jun 6, 2019

See also dart-lang/sdk#16547.

@ds84182
Copy link

ds84182 commented Jun 6, 2019

@eernstg Well, you wouldn't be able to access new C(some, nonconstant, expressions) in a const context anyways, since it isn't constant in the first place.

Example:

class C<T> {
  const T id;
  const C(this.id);
}

const five = C(5).id; // OK
const fiveC = C(5);
const justFive = fiveC.id; // Also OK

final runtimeFiveC = new C(5);
const anotherFive = runtimeFiveC.id; // Compile time error: runtimeFiveC is not const

const yetAnotherFive = new C(5).id; // Compile time error: cannot create an object using new

@eernstg
Copy link
Member

eernstg commented Jun 6, 2019

@ds84182 wrote:

it isn't constant in the first place.

But that's exactly the point: The declaration const T id; is misleading because new C(someNonConstant) can exist. So developers who see const T id in their lexical scope cannot use the rule "a declaration of the form const .. declares a constant", which is otherwise valid. There's no technical problem here, I just wanted to ensure that developers get a syntactic hint about the fact that the semantics is different.

@rrousselGit
Copy link
Author

About that #299 (comment)

Non-nullable types will likely need this too.
It's relatively common to have code that looks like:

if (foo.bar != null) {
  // do something with `bar`.
}

But that's something the compiler won't allow because subclasses can override the field with a getter.

So I'm even more convinced that we should avoid const keyword here, and find a clean way to prevent subclasses from overriding the field.

It's technically a bit different from the original request though.

@eernstg eernstg added the enhanced-const Requests or proposals about enhanced constant expressions label Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhanced-const Requests or proposals about enhanced constant expressions
Projects
None yet
Development

No branches or pull requests

5 participants