-
Notifications
You must be signed in to change notification settings - Fork 205
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
Comments
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. |
Fair enough, but I disagree. It's not about allowing all To take your example, since a const instance is known at compile time, we know if it is a And as such, we know if its field 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. |
@rrousselGit wrote:
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). |
Agreed. 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. |
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. ;-) |
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. |
The "making a field into a getter is a breaking change" problem is the biggest problem in my perspective. If you could do so explicitly, then it's a whole different case. Say:
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 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 We currently have one 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 |
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 if (foo is Foo) {
// `foo` is infered as type `Foo`.
} But we can't do the same for properties inside 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
} |
For the cast to be allowed, we need to know that
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). |
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 |
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 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 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 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 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 |
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 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 We can then, perhaps, later add "constant instance getters" which are getters with "potentially constant" arrow-bodies, with expressions that are constant if Nobody gets locked into something by omitting a marker, only by explicitly opting in to it. |
@lrhn wrote:
That makes sense, and doesn't seem to be too hard. |
Wouldn't that be confusing to use the 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
}
} |
@rrousselGit It may be slightly confusing. We have required you to write |
@rrousselGit wrote:
What we need to say is that for a given getter named That instance variable is guaranteed to be final (or we couldn't have a constant constructor for 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 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 class Foo {
final String s as const;
const Foo(this.s);
}
const foo = const Foo("bar");
const bar = foo.s; // OK. |
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? |
I'd certainly prefer the 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 |
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 |
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);
} |
I still believe that, at their core, const member and type promotion for members using 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
} |
@ds84182 wrote:
Right, that is indeed more familiar. However, the reason why I proposed a different syntax is that the For instance, when you can do We could of course say that this particular kind of class (the ones that have at least one |
See also dart-lang/sdk#16547. |
@eernstg Well, you wouldn't be able to access 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 |
@ds84182 wrote:
But that's exactly the point: The declaration |
About that #299 (comment) Non-nullable types will likely need this too. 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 It's technically a bit different from the original request though. |
👋 Here's a language feature request (should it be moved to https://github.com/dart-lang/language?)
Consider the following const classes:
This proposal is to allow the following:
Currently, it is impossible to assign
foo.bar
to aconst
variable and triggers aThe text was updated successfully, but these errors were encountered: