Analyzer does not reject non-const partial instantiations for default parameter values

[leafpetersen]

This is the analyzer specific issue for the default parameter issue from #32415 .

Partial instantiations of generic methods are only const objects if their type arguments are const types. The analyzer should statically reject the following program, but currently doesn't.

dynamic _defaultCallback<T>(T t) => t;
class C<T> {
  final dynamic Function(T) callback;
  // Should be statically rejected
  void foo([dynamic Function(T) f = _defaultCallback]) {}
  // Should be statically rejected
  const C({this.callback = _defaultCallback});
}

// Should be statically rejected
void bar<T>([dynamic Function(T) f = _defaultCallback]) {}

void main() {
  print(new C<int>().callback);
}

---

[eernstg]

The example program could be deemed erroneous because those generic function instantiations use T and are then not constant, but it also seems possible to allow for a certain kind of inference "slack" that we have discussed a few times: We could use a proper subtype of the type-from-context because it is an immutable entity, and instantiate it as _defaultCallback<Object>.

This differs from the canonical example of inferring const <Null>[] from const [] where a List<T> is expected and T is not constant, but given that the inferred type argument here has contravariant placement (only) it's the same kind of slack for the type as a whole when the latter uses Null and the former uses a top type like Object.

[leafpetersen]

@eernstg I admit I get lost a bit in the weeds of const, but I think the distinction in play is the difference between being a const object, and being in a const context. That is, with const B(), you can think of B() as occurring in a const context: the keyword tells you that it has to be const and so you know to close it. Whereas _defaultCallback does not occur in a const context (I think deliberately), and so it's less clear why we are justified in closing it to make it "constable". As I say, this all feels very fuzzy to me, but... that's const for you. @lrhn thoughts?

[lrhn]

I agree that it's fuzzy. The default value position must (currently) be const, but does not introduce a const context, so you have to write const [] to get a constant list.

That doesn't help you to write a const tear-off because we don't have a syntax like const _defaultCallback. Maybe we should (introducing a const context anywhere might be useful).
We also don't have syntax for _defaultCallback<Object>, so there is no pretty solution.

We could change the inference system to know that a default value should be inferred as-if it's in a const context, even if it isn't.
Or the user can change _defaultCallback to not be generic, since the genericity isn't used anyway:

dynamic _defaultCallback(Object t) => t;
...
  void foo([dynamic Function(T) f = _defaultCallback]) {}

I'd still like to make the change - it annoys me (and likely users too) when we infer something, and then immediately report an error because the thing we inferred wasn't valid. Preferably, that should never happen, I'd prefer giving an "can't infer anything here" error instead.

[eernstg]

we infer something, and then immediately report an error because the thing we inferred wasn't valid

I agree that this should simply never happen.

introducing a const context anywhere might be useful

If we do it using an argument position (so we annotate the formal parameter with the requirement that actual arguments must somehow be "consty") then we definitely need to be softer than a real constant context: They only admit expressions which are actually constant, and I'd expect a useful mechanism on parameters to be a bias towards constants, not a firm requirement. Now and then you'd want to pass something which just isn't a constant.

I suspect that such a "soft" mechanism would be useful also when it's lexically connected to the target. The intuition would be that you can request "magic const" locally by means of a suitable marker (say, a keyword in front of an expression or similar construct):

var List<Choice> choices = soft_const <Choice>[
  Choice(title: 'CAR', icon: Icons.directions_car),
  Choice(title: 'BICYCLE', icon: Icons.directions_bike),
  Choice(title: 'BOAT', icon: Icons.directions_boat),
  Choice(title: 'BUS', icon: Icons.directions_bus),
  Choice(title: 'TRAIN', icon: Icons.directions_railway),
  Choice(title: 'WALK', icon: Icons.directions_walk),
  Choice(title: someComputation(), icon: Icons.directions_walk),
];

// .. same as ..

var List<Choice> choices = <Choice>[
  const Choice(title: 'CAR', icon: Icons.directions_car),
  const Choice(title: 'BICYCLE', icon: Icons.directions_bike),
  const Choice(title: 'BOAT', icon: Icons.directions_boat),
  const Choice(title: 'BUS', icon: Icons.directions_bus),
  const Choice(title: 'TRAIN', icon: Icons.directions_railway),
  const Choice(title: 'WALK', icon: Icons.directions_walk),
  Choice(title: someComputation(), icon: Icons.directions_walk),
];

This would not create a constant list, because there is an element in there which will never be constant, but it will allow all the list elements which are able to be constant to become constant by magic.

The discussions we had about how to avoid "unless it is an error" in the specification would still apply. But given that we only get magic const under certain conditions, it should be OK to commit more firmly to which expressions are made const. So we could apply a simple local criterion to choose "must be const" vs. "will be non-const" for each subexpression, and we would then simply get an error if there is something which is committed to be const, but where const evaluation fails.

[bwilkerson]

@leafpetersen Is this still an issue that we need to address for Dart 2 Stable?

[leafpetersen]

@bwilkerson It would be good to fix this.

[MichaelRFairhurst]

I'm going to deprioritize this based on the discussions here. We're low on resources for 2.0.

It seems like whatever we do may change for some kind of "partial const" feature, or maybe const contexts of parameters needs to change. Therefore the work done now may only be valid for a short time and I think its better to focus on more well-defined bugs. It's also listed as a P2.

If anyone disagrees, do please weigh in.

[stereotype441]

Escalating to P1 since the front end has this bug too, so users are in danger of using incorrect constant values by accident. Note that prior to landing a fix for this bug we should check that no internal code will be broken by the change.

[MichaelRFairhurst]

This actually has an even simpler repro:

dynamic _defaultCallback<T>(T t) => t;                                           
const f = _defaultCallback;

This is also broken, and if we fix this, we'll fix the original repro as well, I believe.

[MichaelRFairhurst]

We have violations of this in our own SDK:

R _rootRunUnary<R, T>(                                                           
    Zone self, ZoneDelegate parent, Zone zone, R f(T arg), T arg) {              
  ...
}

...

  _ZoneFunction<Function> get _runUnary => 
    const _ZoneFunction<Function>(_rootZone, _rootRunUnary);
....

[MichaelRFairhurst]

[error] Const variables must be initialized with a constant value. (dart:_http/overrides.dart, line 9, col 24)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1245, col 48)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1247, col 48)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1249, col 48)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1251, col 48)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1253, col 48)
[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1255, col 48)
[error] Const variables must be initialized with a constant value. (dart:io/overrides.dart, line 9, col 24)

[MichaelRFairhurst]

Internal CL 230792951

[lrhn]

The examples with const f = _defaultCallback; and _runUnary are not this kind of errors. In both cases, the generic function itself is used, not a generic instantiation of it, so there is no non-constant instantiation.
The error report saying:

[error] Arguments of a constant creation must be constant expressions. (dart:async/zone.dart, line 1245, col 48)

is incorrect. In all the cases, the arguments/values are constant expressions (references to static functions with no generic instantiation). For example, the function _rootRunUnary is directly assignable to Function, so there should be no generic instantiation here.

[MichaelRFairhurst]

Thanks Lasse for the quick response and correction. That certainly makes a big difference :)

It looks like we'll have to solve #32918 first. Current code does not do a partial instantiation, I don't think, but rather merely passing a subtype check. In any case, these issues are clearly very closely related and I may just end up solving both.

[MichaelRFairhurst]

Talked to @stereotype441 and this is independent of #32918. I missed the critical part about how inference behaves differently in the const contexts, and this, as covered here, is not a context, where #32918 is.

[MichaelRFairhurst]

OK, https://dart-review.googlesource.com/c/sdk/+/91105 is ready for review, seems to catch this, and passes TAP internally except for two violations.

The two fixes: cl/231432647 and cl/231440759

I'll also prepare a "breaking change" draft before landing

[stereotype441]

@MichaelRFairhurst it looks like https://dart-review.googlesource.com/c/sdk/+/91105 was code reviewed but not landed. Can you get this landed?

[stereotype441]

After discussion with @MichaelRFairhurst, we've realized that:

• The front end implemented this change ~1 year ago without doing a breaking change announcement (oops)
• Mike's attempt to send a breaking change announcement ran into permissions issues (which is why no announcement went out)

It seems that making a breaking change announcement at this point would be silly, since the front end has prohibited this pattern for nearly a year. Any code out in the wild that's affected by this change has surely already been updated. So we think all that remains to do is to implement the correct logic in the analyzer. This is nearly complete (just need to fix some unit testing issues). However, since this no longer affects code in the wild, it doesn't seem like we should classify it as P1 anymore.