Sdk: Finish implementation of optional new and const

[peter-ahe-google]

When I originally implemented support for optional new and const, I wasn't aware about a feature that we call "magic" const, also I wasn't aware that default values of named parameters aren't implicitly const (see #32430).

Magic const is described in implicit-creation.md.

Let's look at some examples, assume these definitions are given:

class A {
  const A();

  int get length => 0;
}
const dynamic a = const A();
const dynamic b = "";
class C {
  final x;
  const C(this.x);
}

Is this expression a compile time constant:

C(a)

According to the magic-const feature, it is. So this means that the compiler should treat this as:

const C(a)

Unfortunately, that's not what I implemented. Because there's no context that requires a const, I implemented treating it as:

new C(a)

Obviously, this is a bug, and we want to fix it immediately. However, we estimate that it will take some time, and when the fix lands, it will be a breaking change. Why? It has to do with canonicalization of compile-time constants. This is the simplest example I can think of: Object() == Object(). This should evaluate to true, but we're currently evaluating it to false.

To eliminate the risk of breaking users that start relying on optional new and const, we decided to implement a stop-gap measure and report an error when we can't correctly infer const.

In case you're wondering why fixing this will take some time, the reason is that we need to implement compile-time constant evaluation in the new front-end. Previously we hadn't implemented that because the backends took care of it themselves. But to correctly pick const or new, we now need to evaluate the (potentially) constant expressions. For example:

C(b.length) // Is a compile-time constant.
C(a.length) // Isn't a compile-time constant.

[peter-ahe-google]

Unfortunately, I haven't gotten to this yet. Next week is vacation.

[peter-ahe-google]

@stefantsov will take a look at this next week.

[stefantsov]

The issue is being handled in CL 48481.

[stefantsov]

Hmm... I'm blocked by a strange issue: when I try to create an instance of a Visitor subclass as a field in the BodyBuilder class or even as a local variable, the following test starts crashing:

python tools/test.py -m release -c dartk service/type_arguments_test

[stefantsov]

I'm investigating. Will try to resolve the issue before the weekend.

[stefantsov]

So, I've created a GitHub issue for the surprising behavior described above https://github.com/dart-lang/sdk/issues/32717. I also implemented a work-around. Working on fixing other minor issues.

[stefantsov]

CL 48481 landed. With it all of the tests that were marked as CompileTimeError due to the "implementation limit" are now passing. In addition to that, some other tests started passing as well. There is still some work to be done to insert new and const in more places where it's possible. I think it's quite doable in a couple of days. After that we may close this issue.

[dgrove]

Dropping to P1 now that CL 48481 has landed.

[tvolkert]

Linking to https://github.com/dart-lang/sdk/issues/32737 for archeology's sake

[DanTup]

Is this one a known/expected issue? Using flutter master (dated 30 hours ago):

compiler message: lib/stock_row.dart:40:29: Error: The keyword 'const' or 'new' is required here. Due to an implementation limit, the compiler isn't able to infer 'const' or 'new' here.
compiler message:                     bottom: BorderSide(color: Theme.of(context).dividerColor))),
compiler message:                             ^

[kmillikin]

@DanTup that's (probably) expected. We're still not done implementing this feature. As Dima said:

There is still some work to be done to insert new and const in more places where it's possible. I think it's quite doable in a couple of days. After that we may close this issue.

We had a few days off due to the Easter holidays, but we're back to working actively on this issue.

[DanTup]

I thought that might be the case, but since I had it on my screen I figured it was worth pasting in in case it'd been missed. Thanks!

[stefantsov]

I'm actively working on the constness evaluator. CL 49122 and CL 49140 are going to fix some of the issues.

[JekCharlsonYu]

closed as duplicate of https://github.com/dart-lang/sdk/issues/32737

[tvolkert]

Not yet closed?

[JekCharlsonYu]

closed as duplicate of #32737 (one more time)