Skip to content

const-by-default constructors #3399

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

Open
rakudrama opened this issue Oct 12, 2023 · 5 comments
Open

const-by-default constructors #3399

rakudrama opened this issue Oct 12, 2023 · 5 comments
Labels
enhanced-const Requests or proposals about enhanced constant expressions

Comments

@rakudrama
Copy link
Member

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.

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: <Widget>[
          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.

@rakudrama rakudrama added the enhanced-const Requests or proposals about enhanced constant expressions label Oct 12, 2023
@lrhn
Copy link
Member

lrhn commented Oct 13, 2023

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
Copy link
Member

eernstg commented Oct 13, 2023

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
    const Center(
      child: Column(
        children: <Widget>[
          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.
    Center(
      child: Column(
        children: <Widget>[
          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.
          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
Copy link
Member

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
Copy link
Member

lrhn commented Oct 16, 2023

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.

  • It's use-case dependent. Doesn't apply to every constant or potentially constant expression.
  • It doesn't change semantics, it just enforces a specific choice (add more const).

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

  • It's constant, or
  • It's not a potentially constant expressions, or
  • It's a potentially constant expression, where all sub-expressions are maximally constant, but not all sub-expressions are constant.

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

  • If the expression is already validly constant, make it still be constant.
  • If the expression is not a potentially constant expression, make it not be constant.
  • If the expression is potentially constant, recursively make all subexpressions maximally constant.
    • If all subexpressions are now constant, make this expression constant too (and remove nested consts if needed/possible).
    • Otherwise make it non-constant.

(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
Copy link

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.

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