Sdk: Lint request: Do not use optional new/const

[mit-mit]

Dart 2 introduces support for optional new/const. Some teams prefer to consistently only have required new/const keywords in their code, and leave out all the rest. To enforce this, a lint is requested.

[mit-mit]

cc @a14n any interest in looking at this?

[a14n]

@eernstg, @lrhn : quick question for the following example

class A {
  const A(o);
}
main() {
  A([]);
}

Is A([]) equivalent to const A(const []) or new A([]) ?

BTW it would be great to have examples on https://github.com/dart-lang/sdk/blob/master/docs/language/informal/implicit-creation.md.

[floitschG]

A([]) is equivalent to new A([]) since [] is not in a const context, and there will be a fresh list. As such, the A(...) call does not have just const expressions as arguments and is therefore a new call.

Edit:
To make this a const expression you have two options:
const A([]) or A(const []).

There is, unfortunately, no great rule to prefer one over the other. Depending on the context you really want the whole expression to be a const in which case having the const in the outer position makes sense. However, if the outer const should just be incidental (nice bonus) and there might be children that aren't const (but currently happen to be const) then the inner one would be nicer.

Take as example a Flutter tree:

return SomeWidget(  // <== `const` here
      margin: EdgeInsets.symmetric(vertical: 10.0),
      child: SomeOtherWidget(
        crossAxisAlignment: CrossAxisAlignment.start,
        children: <Widget>[   // <=== or here.
           ...
        ],
      ),
    );

Here we could either have a const on the children of SomeOtherWidget or on the SomeWidget (assuming that they have const constructors).

If I put a const on SomeWidget then I will not need to think about it inside the children anymore. However, if I change my structure in such a way that just one child expression depends on a non-const expression (like a field or a parameter to the build method) then I need to remove the const from SomeWidget and walk through my children to see if some of them would benefit from const.

On the other hand, if I move them to the children I might end up having more const than necessary.

[a14n]

@floitschG thanks for your explanations.

[lrhn]

To elaborate on what Florian said, the logic behind const insertion is very similar to type inference.

In Dart 2 type inference, if the context requires a specific type, that is the type we try to make the expression have. If the context doesn't care, then we build the type of the expression from the types of the sub-expressions.

For const/new insertion, it's similar.
If the context requires a const value, we will try to make it one by inserting const in front of everything that needs it, and if the result still isn't a const value, it's an error anyway.
If the context doesn't require a const value, then we evaluate the sub-expressions normally, and if all the arguments to a const constructor are const, we make that a const construction too.

The distinction matters for list and map literals. For classes with const constructors, if the constructor arguments are constant, then the object is immutable anyway, but a list literal with constant elements is not naturally immutable, and we won't make it so by accident. So const C([]) inserts const in front of [] because it would be an error otherwise, but C([]) does not because you might want a mutable list stored in C. For C(const []), you will get an immutable C instance, the only difference between new and const is the identity of the immutable instance. In that case, we just make it const for you, because it's convenient - no more writing the const in const Duration(seconds: 1).

(We have one exception: optional parameter default values. We reserve the option to allow non-const values in that position in the future, and for that reason, we won't consider it a "const requiring context" for const inference, even if it is currently still an error if the value isn't constant).

[eernstg]

Interesting! One simple, syntactic rule of thumb might be useful: When we want opportunistically const values ("the more the better, but it's not required to be all const") we could put a const on every composite literal (list or map, currently) where it is possible and correct according to the intended behavior of the program; when we cannot tolerate anything else than a const value we would put a const at top level, if needed (in that situation we'd often be in a const context, anyway, like e in const x = e;). So the rule would be "best effort: bottom-up on composite literals; _must_ be const: top-down".

[a14n]

Thanks for all your explanations.

Could you check the following list to validate if my expectations are good.

class A {
  const A([o]);
}
main() {
  A();                       // new A()
  A([]);                     // new A([])
  final v1 = A();            // new A()
  const v2 = A();            // const A()
  final v3 = A([]);          // new A([])
  const v4 = A([]);          // const A(const [])
  final v5 = A(const []);    // const A(const [])
  final v6 = const A([]);    // const A(const [])
  final v7 = A([const A()]); // new A([const A()])
  const v8 = A([A()]);       // const A(const [const A()])
  final v9 = const A([A()]); // const A(const [const A()])
}

[a14n]

Re-reading #32553 I'm not sure about my expectations.

[eernstg]

Almost:

class A {
  const A([o]);
}
main() {
  A();                       // const A(), because it's possible.
  A([]);                     // new A([]), because `[]` is only const when forced by context.
  final v1 = A();            // const A(), because it's possible.
  const v2 = A();            // const A(), both possible and forced.
  final v3 = A([]);          // new A([]), because of non-forced `[]`.
  const v4 = A([]);          // const A(const []), forced by being initalizer for `v4`.
  final v5 = A(const []);    // const A(const []), because it's possible to add the outer `const`.
  final v6 = const A([]);    // const A(const []), the inner `const` is forced by the outer one.
  final v7 = A([const A()]); // new A([const A()]), the list is not forced `const`, stays mutable.
  const v8 = A([A()]);       // const A(const [const A()]), forced by being initializer for `v8`.
  final v9 = const A([A()]); // const A(const [const A()]), forced by outermost `const`.
}

[a14n]

Thanks!

Last one :

final v = A(A());   // const A(const A())  ?

[eernstg]

Right! ;-)

[leafpetersen]

We are not shipping the automatic const insertion part of this feature. All constructor calls with no explicit new or const are made new unless they appear in a const context. Updated examples below:

class A {
  const A([o]);
}
main() {
  A();                       // new A(), because not in const context.
  A([]);                     // new A([]), because not in const context.
  final v1 = A();            // new A(), because not in const context.
  const v2 = A();            // const A(), because in const context.
  final v3 = A([]);          // new A([]), because not in const context..
  const v4 = A([]);          // const A(const []), forced by being initalizer for `v4`.
  final v5 = A(const []);    // new A(const []), because not in const context..
  final v6 = const A([]);    // const A(const []), the inner `const` is forced by the outer one.
  final v7 = A([const A()]); // new A([const A()]), the list is not forced `const`, stays mutable.
  const v8 = A([A()]);       // const A(const [const A()]), forced by being initializer for `v8`.
  final v9 = const A([A()]); // const A(const [const A()]), forced by outermost `const`.
  final v = A(A());   // new A(new A()), because not in const context.
}

[a14n]

We are not shipping the automatic const insertion part of this feature.

Is it just a first step or is it totally removed?
What's the reason of this decision?

[leafpetersen]

When it went to implementation, it became clear that there were serious concerns around how to make this work as specified with modular or platform independent compilation. As specified, you have to do constant evaluation before you can insert new and const, which means that in some important scenarios you can't insert new and const until essentially link time. We did some quick exploration of alternatives (e.g. https://dart-review.googlesource.com/c/sdk/+/50080) but none of the options that were short term feasible were entirely satisfactory. Given our extremely short time horizon for landing a change, we felt that changing the spec was a large implementation risk, and had a large risk of us specifying a bad feature.

I think it unlikely that we will ship the feature as originally specified in the future, but we may consider looking for an alternative way to get similar functionality at some point.

[mit-mit]

Hi @a14n are you still looking at this?

[a14n]

@mit-mit the lint is ready but not merged for now (see dart-lang/linter#936). Basically I tried to remove optional const in the flutter codebase and faced an issue with flutter analysis. I'm waiting for someone to figure out what's the underlying problem and fix it to go further (cc @pq @bwilkerson @devoncarew ).

[a14n]

(link to flutter issue updated to https://github.com/flutter/flutter/issues/17429)