dart-lang / language

Design of the Dart language
Other
2.67k stars 205 forks source link

const-by-default constructors #3399

Open rakudrama opened 1 year ago

rakudrama commented 1 year ago

Today, a const constructor for class Foo can be invoked as new Foo(), const Foo() or simply Foo(). In the last case, the invocation defaults to new Foo() unless the expression is in a const context, in which case it can only be const, so it defaults to const.

In the case of Flutter widget trees, the defaulting to new Foo() is the wrong choice. It would be better to default to const Foo() if at all possible.

There are several lints that try to help the developer work around the lack of const-by-default:

If the developer has a const expression and changes some deep sup-expression to be non-const, the expression is now an error. The developer removes the high const to fix the error. The lints now encourage adding const all along the side-trees of the spine of the expression tree leading to the original edit.

Example

As an example of this experience, copy the following program into dartpad.

```dart import 'package:flutter/material.dart'; /// Flutter code sample for [Divider]. void main() => runApp(const DividerExampleApp()); class DividerExampleApp extends StatelessWidget { const DividerExampleApp({super.key}); @override Widget build(BuildContext context) { return MaterialApp( home: Scaffold( appBar: AppBar(title: const Text('Divider Sample')), body: const DividerExample(), ), ); } } class DividerExample extends StatelessWidget { const DividerExample({super.key}); @override Widget build(BuildContext context) { return const Center( child: Column( children: [ Expanded( child: ColoredBox( color: Colors.amber, child: Center( child: Text('Above'), ), ), ), Divider( height: 20, thickness: 5, indent: 20, endIndent: 0, color: Colors.black, ), Align( alignment: AlignmentDirectional.centerStart, child: Text( 'Subheader', textAlign: TextAlign.start, ), ), Expanded( child: ColoredBox( color: Colors.blue, /*54*/ //color: Theme.of(context).colorScheme.primary, child: Center( child: Text('Below'), ), ), ), ], ), ); } } ```

Now use a computed color: uncomment line 54 and comment-out line 53. You will see 7 errors for 'invalid const value'. The remedy is to remove the const at line 26. You will now see 9 lint warnings to prefer const. The remedy is to add const in 4 places.

The lints have quick-fixes to help with this process, but pushing const around the code would be completely unnecessary if the widget constructors defaulted to const whenever possible.

Questions

Should const-by-default be the default behaviour? This would break identical(Object(), Object()). One can always 'escape' the behaviour with an explicit new, but this idea is probably too breaking.

If const-by-default is not the default behaviour, should the language add an opt-in syntax, e.g. putting the pseudo-keyword prefer in front of const, i.e.

class Foo {
  final List<Widget> children;
  final Color color;
  prefer const Foo({this.children = const [], this.color = Colors.black});
}

The opt-in could be via a class modifier: const class Foo { }. This makes all constructors const-by-default.

A class or constructor opt-in would nicest for Flutter - the preference for const is really an implementation detail of Flutter, so should be 'batteries included' as much as possible.

Should a const-by-default constructor that can't be const because of one argument make the other argument expressions be const if possible? e.g. should Foo(color: computed(), children: [Foo()]) make the children: list be const? Should a non-const constructor (or for that matter, a static method) be able to impose a const-by-default context on an argument by adding const or prefer const to the parameter declaration?

Another option would be to have syntax to introduce a const-by-default context, say, const?. In the big example, writing return const? Center(... would infect the whole expression with const-by-default. I think this is worse than the above ideas, mainly because the Flutter developer still has to do something.

lrhn commented 1 year ago

I actually think the "const by default context" is more viable than the "const by default constructor". The context relates to the use of the value, the constructor does not.

Both have the same issues as usual: if const is optional, not being const is not an error. One small mistake deep in a nested expression, and then the entire tree decides to not be constant, and you get no warning about that, because it's optional.

Having to write const is annoying, but it's explicit and checkable. If you write const, and it isn't, that's a compile time error. If it's optional and "by default", not being const is just working as intended.

Then there are the collection issues. If you write [1], it can mean a mutable list or a constant list. Automatically making it be either can be wrong. And we don't have any syntax for asking for a mutable list. (We could allow new [1]).

Even if we try, the context would have to propagate to the elements expressions, and then it may require analysing more than once, backtracking of a definitely-not-const element of found.

In a List<X>, X a type variable, context, a [] becomes <X>[] of not const, but <Never>[] if const.

A "const requested, but not required" context of List<List<X>> would make [[], [x], []] try to be const, visit [] first and probably conclude that it can be const, so make it <Never>[]. Then it sees [x] which cannot be const. Should it then go back and make the first element not const either? Probably not. Should it make the next [] be const anyway? That'd be a kind of consistent, even if the overall result is inconsistent. Basically, a collection literal in a "prefer to be const" would put it's elements into such a context too, then be const only if every element was. Likely any constructor invocation would put its arguments into a prefer-const context as well, even if the parameter wouldn't itself introduce a try-const context. Which means we need the opt-out of writing new, to get out of the prefer-const context.

That's a potentially serious problem, because constant evaluation happens after type inference (all evaluation does, it needs to know the types), but type inference depends on knowing whether an expression is constant. So far, that's been decidable from the context or the expression. In a "prefer-constant" context, the inference wouldn't know which approach to use, and using both (potentially transitively on sub-expressions) breaks the one-pass inference design. It's more likely that inference will make a guess, based on only available type and context information and the syntactic structure, for whether something preferred to be const, should be const. And if it guesses wrong, it'll be a compile-time error and you'll have to add const or new. If it guesses correctly, it will "just work". Which is how type inference works, so it might be adequate.

I think it's possible to do something like this, but definitely not trivial. I also think the implicit nature will make it error prone.

eernstg commented 1 year ago

I'm also worried about the readability issue and the potentially wrong/accidental choice of non-const status.

However, it would also be possible to use an IDE based remedy: We could have two transformations, say, lower-const and raise-const, that would transform an expression in a way that does most of the editing.

lower-const would work on the selection which would be an expression that has const as its first token; it would remove the top-level const and add const to every directly nested expression. For instance, it would change const [C(), C()] to [const C(), const C()]. If lower-const is applied to an expression whose first token isn't const then it is applied recursively to every subexpression that does start with const.

Similarly, raise-const would work on the selection which should be an expression that does not have const as its first token. It would then add const at the top level of the target expression and remove all nested occurrences of const.

For instance, using the code from the original posting of this issue:

Original expression ```dart const Center( child: Column( children: [ Expanded( child: ColoredBox( color: Colors.amber, child: Center( child: Text('Above'), ), ), ), Divider( height: 20, thickness: 5, indent: 20, endIndent: 0, color: Colors.black, ), Align( alignment: AlignmentDirectional.centerStart, child: Text( 'Subheader', textAlign: TextAlign.start, ), ), Expanded( child: ColoredBox( color: Colors.blue, /*54*/ //color: Theme.of(context).colorScheme.primary, child: Center( child: Text('Below'), ), ), ), ], ), ); ```

Now we edit the code such that color: Colors.blue becomes color: Theme.of(context).colorScheme.primary. As a consequence, we get an error on line 54.

Now perform `lower-const`, twice, on the top-level expression. ```dart Center( child: Column( children: [ const Expanded( child: ColoredBox( color: Colors.amber, child: Center( child: Text('Above'), ), ), ), const Divider( height: 20, thickness: 5, indent: 20, endIndent: 0, color: Colors.black, ), const Align( alignment: AlignmentDirectional.centerStart, child: Text( 'Subheader', textAlign: TextAlign.start, ), ), const Expanded( child: ColoredBox( //color: Colors.blue, /*54*/ color: Theme.of(context).colorScheme.primary, child: Center( child: Text('Below'), ), ), ), ], ), ); ```
Finally, perform `lower-const` twice again on the last `const` expression and remove `const` from line 54, or just edit those changes directly. ```dart Expanded( child: ColoredBox( //color: Colors.blue, /*54*/ color: Theme.of(context).colorScheme.primary, const child: Center( child: Text('Below'), ), ), ```

The point is that the effect of those two transformations is easy to understand, and they will perform some editing operations that are relevant to the situation described in this issue, as well as other situations where there is a need to manipulate the occurrences of const in a large expression.

munificent commented 1 year ago

Fundamentally, I think of this as a performance problem. Flutter and Flutter users want build() methods and tree diffing to be fast. We want the language to make that as easy and non-error-prone as possible. This is one possible language change that would possibly help. But by how much? And for which applications and where?

I generally dislike the approach that Dart takes to const, but I'm very hesitant to make performance-driven changes without good performance data we can use to evaluate it.

lrhn commented 1 year ago

I don't see any "zero effort" approach that will not have pitfalls.

If the client of an API doesn't have to do anything, and will still gets as many constant expressions as possible, then it's still possible that they get zero, and it won't be noticed.

This sounds more like a lint.

So say that if a parameter or variable (or expression context in general) is marked with @preferConst, then expressions used there can be checked for whether they are "maximally constant". If not, the linter can warn, and a quick-fix can rebuild the expression from the bottom up, choosing const everywhere possible. The same quick-fix can be applied to any marked-constant expression which has an error because it cannot be constant: Rebuild to be maximally constant, but no more.

An expression is maximally constant iff

An expression is made maximally constant (whether it's currently a constant expression or not) by traversing every subexpression:

(Update the const modifiers to avoid nested ones, if it wasn't done along the way.)

We can allow an explicit new in new Foo() to opt out of being made constant, if there is a need for that. (We could allow new [..] and new {...} for collections too, if we wanted to, but they do nothing, so we'd be wasting syntax. Probably better to assume collections should always be constant if possible - if they occur in a context which accepts constants, they won't be modified.)

AlexanderFarkas commented 1 year ago

I believe it would be massively breaking, since sometimes you just want different objects, even if their constructor marked const for performance reasons. Not to mention having const by default widgets conflicting with hot-reload.

I don't understand why running dart fix in your pipeline is not an option, if you really need const by default.