CFE is not inferring generic type for const default parameters

[vsmenon]

DDK asserts on the following. DDC does not. In the constructor of C, the CFE is instantiating B with Null (causing the error). The analyzer with T (which works).

class A<T> {
  const A();
}

class B<T> implements A<T> {
  const B();
}

class C<T> {
  final _a;

  const C([A<T> a = const B()]) : _a = a;
}

void main() {
  var c = new C<String>();
  print(c._a.runtimeType);
  assert(c._a is! A<Null>);
}

The result with DDK is:

B<Null>
Assertion failed: is not true Error

@a-siva is hitting this in package:collection.

@kmillikin

[vsmenon]

@stereotype441 @leafpetersen @lrhn - Paul says this is a bug in analyzer.

Here's the actual repro with collection:

import 'package:collection/equality.dart';

void main() {
  var eq = const ListEquality<String>().hash(["hello", "world"]);
}

It throws with CFE:

Type 'String' is not a subtype of type 'Null' in strong mode Error
    at Object.dart.throw (/Users/vsm/ddctest/node_modules/dart_sdk.js:4902:27)
    at Object.dart.castError (/Users/vsm/ddctest/node_modules/dart_sdk.js:4830:15)
    at Object.dart.as (/Users/vsm/ddctest/node_modules/dart_sdk.js:4801:15)
    at Function.check_Null [as _check] (/Users/vsm/ddctest/node_modules/dart_sdk.js:43361:15)
    at DefaultEquality.new.hash (/Users/vsm/ddctest/siva3.js:185:9)
    at ListEquality.new.hash (/Users/vsm/dart/sdk/third_party/pkg/collection/lib/src/equality.dart:173:15)
    at siva3.main (/Users/vsm/ddctest/org-dartlang-app:/siva3.dart:5:18)

[lrhn]

Paul is correct. You cannot write const B<T>() as the parameter default value since T is not a compile-time constant. We do not evaluate default values per invocation. We could change that and not require the expression to be constant at all, but we currently don't.
So, const B<T>() is invalid whether the T is explicit or inferred, so we don't infer T. The only valid type to infer is const B<Null>() - the one const expression that is assignable to B<T> for any T, so that will be the const expression post-inference.

(We actually do not consider parameter default values as an implicit const context where you can omit const because we are keeping the door open to making it non-const).

[stereotype441]

In the package:collection example, the way the analyzer bug manifests is that the constructor for ListEquality, which looks like this:

class ListEquality<E> implements Equality<List<E>> {
  const ListEquality([Equality<E> elementEquality = const DefaultEquality()]) ...;
}

gets inferred as:

class ListEquality<E> implements Equality<List<E>> {
  const ListEquality([Equality<E> elementEquality = const DefaultEquality<E>()]) ...;
}

That's not legal, since E is not const. The correct Dart 2.0 behavior (which the front end implements) is to infer a type parameter of Null:

class ListEquality<E> implements Equality<List<E>> {
  const ListEquality([Equality<E> elementEquality = const DefaultEquality<Null>()]) ...;
}

Unfortunately, that causes a (correct) strong mode runtime error, because ListEquality.hash calls DefaultEquality.hash, and DefaultEquality.hash has signature:

int hash(E e);

Since E is Null, soundness requires that we don't allow anything but null to be passed to hash, so the front end inserts a runtime type check. There's a similar problem with DefaultEquality.equals.

One possible solution would be to change the signature of DefaultEquality.equals and DefaultEquality.hash like so:

bool equals(Object e1, Object e2);
int hash(Object e);

[vsmenon]

The suggested solution (use Object in DefaultEquality) seems to do the trick.

[lrhn]

That seems like it would work - a class that implements Equality<Null> and accepts Object on any argument is really the equivalent of Equality<dynamic> in Dart 1. It's assignable to any Equality<T> and it's callable with any argument. It might be a little surprising that new DefaultEquality<int>().equals("a", "b") works (but then == was always defined to accept any object).

We could even make DefaultEquality always implement Equality<Null>, instead of having a type argument, but I guess that's a little too clever (and breaking).

It's annoying that we have to make this workaround to dodge the type system, but luckily it's fairly localized.

[nex3]

It does seem, intuitively, like inferring DefaultEquality<E> should work here, even if we don't allow fully non-const default arguments. E is bound by a const context—at the point at which the const constructor is invoked, E is known at compile-time.

[lrhn]

There are multiple issues that prevent this.

• Default values are compile-time constants, so the expression must always evaluate to the same value. That's a general rule for compile-time constant expressions.
• Default values are single values, they are not evaluated once per invocation (same thing, but at the conceptual level - we can't document a value in the DartDoc if it changes per invocation).
• And finally, a const constructors can also be invoked with new, so there isn't necessarily a const context at all.

All of these prevents the default value from both depending on E and being compile-time constant.

[leafpetersen]

All of these prevents the default value from both depending on E and being compile-time constant.

Sort of. There is a notion of a potentially const expression to handle this at the term level, right? So why not at the type level? Wouldn't that work here?

[dgrove]

Moved milestone since the workaround has been committed.

[lrhn]

The notion of potentially constant only applies to initializer lists and it only allows string/int/bool computations. Object creations are always actually constant (aka. "compile-time constant" rather than "potentially constant").

This is an object creation expression in a default value position. We expect default values to be actually constant so each invocation gets the same default value.
Since E differs between invocations, it can't be part of an actually constant expression.
Since the constructor can be called in non-constant expressions too, there is no limit to the number of types E can be bound to.
We could change default values to being "default expressions", and evaluate them each time the function is called (with obvious optimization if the value happens to be a compile-time error), but that's a much bigger change that we decided not to do at this point (we are keeping the door open, though).

Since it's a const constructor, it would still require an expression that can be evaluated at compile time to create a different non-int/bool/String object each time, which is not something we have.
That's another change we decided not to do (the "auto const in initializer list" feature which would treat Foo(x) as new Foo(x) when the constructor is invoked with new and as const Foo(x) when the instructor is invoked with const. That would allow potentially constant object creation expressions, with the caveat that we then had to decide how to handle infinite recursion at compile-time.

So, potentially constant type expressions are possible, but they won't apply to constant object creation expressions anyway. That's why I didn't introduce them - they would only be good for is T and as T, if we add those as potentially constant expressions, not const C<T>(...) or const <T>[...].

[vsmenon]

Assigning to myself to retriage. What do we need to resolve by the next milestone?

[jmesserly]

I found a similar issue (in a flutter application) involving generic functions used as default values. In that case, CFE does appear to instantiate the generic function, although Dart VM doesn't seem to do that correctly at runtime.

Here's the pattern it used:

typedef F<T>(T t);

_defaultCallback<T>(T t) => t;

class C<T> {
  final F<T> callback;
  // the behavior is the same with or without `const` here
  // the flutter app did not have `const`
  const C({this.callback = _defaultCallback});
}

main() {
  var c = new C<int>();
  print(c.callback(42));
}

The Kernel IR does contain instantiate (and the program works in DDK):

main = test::main;
library from "file:///Users/jmesserly/scratch/test.dart" as test {
  typedef F<T extends core::Object> = (T) → dynamic;
  class C<T extends core::Object> extends core::Object {
    generic-contravariant final field (test::C::T) → dynamic callback;
    const constructor •({(test::C::T) → dynamic callback = test::_defaultCallback<test::C::T>}) → void
      : test::C::callback = callback, super core::Object::•()
      ;
  }
  static method _defaultCallback<T extends core::Object>(test::_defaultCallback::T t) → dynamic
    return t;
  static method main() → dynamic {
    test::C<core::int> c = new test::C::•<core::int>();
    core::print((c.{test::C::callback} as{TypeError} (core::int) → dynamic).call(42));
  }
}
constants  {
}

whereas Dart with preview-dart-2 yields:

Unhandled exception:
type '(T) => dynamic' is not a subtype of type '(int) => dynamic'
#0      main (test.dart)
#1      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#2      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

Note the uninstantiated generic function type.

... I'd file this as a VM bug but I'm not sure the intended behavior of the language? Should CFE reject my example at compile time?

In the meantime I worked around the problem by using a constructor initializer:

typedef F<T>(T t);

_defaultCallback<T>(T t) => t;

class C<T> {
  final F<T> callback;
  const C({F<T> callback}) : callback = callback ?? _defaultCallback;
}

main() {
  var c = new C<int>();
  print(c.callback(42));
}

[lrhn]

Should CFE reject my example at compile time?

Yes.

You can reduce the example to:

dynamic _defaultCallback<T>(T t) => t;
class C<T> {
  final dynamic Function(T) callback;
  const C({this.callback = _defaultCallback});
}

The expression _defaultCallback is a generic specialization tear-off (the static type of the plain _defaultCallback is generic, so it doesn't match the type of the callback field, so it must be an instantiation of that generic method that is "torn off"), so the question is which instantiation.

It can't be _defaultCallback<T> because T is not available in a const expression.
That's what the IR contains, and it's just wrong. Not allowed, should cause a compile-time error if someone managed to write it manually, and definitely not something we should infer for you.

The resulting type must be assignable to dynamic Function(T) for any T, so the only solution is _defaultCallback<Object> (or another top type, or, in general, the bound of T). That would be a valid instantiation. I don't know if we can generally promise to find that for you.

In any case, using a private generic function, which isn't used for anything else, for the default value is unnecessary - the default value is a single value, one instantiation, so you should just write that non-generic function directly, dynamic _defaultCallback(Object t) => t;.

As I see it, generic method instantiation should be treated like inference where we infer a type argument, like _defaultCallback<T>, even if we currently can't write that syntactically. Whether the expression is then valid depends on the invisible inferred type variable. (I do hope we get the syntax in the near future).

With that in mind, the rewrite shouldn't work any better, and it's an accident that it works.
The callback ?? _defaultCallback expression should not be potentially constant when _defaultCallback is implicitly inferred as _defaultCallback<T>. The value of _defaultCallback should be a compile-time constant expression when the constructor is invoked with const, and it isn't when it depends on T.

The accident here is that we haven't actually mentioned the generic-method-tearoff case, with its invisible type argument, in the constants section, and that you don't actually invoke the constructor with const. We should fix the former of those.

[jmesserly]

@lrhn -- does any of that change if the constructor is not marked const?

In the real flutter app code, the constructor was not marked const, but VM still had a problem with the default argument value. I presume it's valid to reference T from a non-const constructor's initializer list? In that case, my workaround seems okay?

[lrhn]

does any of that change if the constructor is not marked const?

It changes nothing for the default value expression. Default value expressions have to be constant no matter where they occur, whether it's a plain method, a const constructor or a non-const constructor.

It does change what the initializer expression is allowed to do, and in a non-const constrcutor it can do a per-invocation instantiation/tear-off that uses T, so yes, in that case the workaround is perfectly fine.

[vsmenon]

@lrhn @leafpetersen - what's actionable here? Is there a bug in analyzer, CFE, or both?

[jmesserly]

Seems like it's a bug in both, regardless :)

If these should be errors, both Analyzer/CFE are missing that logic.

If these should be inferred, it sounds like they'll have to be inferred as something that doesn't depend on <T>. So it would infer a = const B<Null>() in your example, and this.callback = _defaultCallback<Object> in my example (assuming instantiations of top-level generic functions are const expressions). I don't think Analyzer or CFE is inferring the right thing for both cases. (Perhaps implemented by substituting ? for type params and taking the least closure, then using that as the context type for inference.)

[vsmenon]

@leafpetersen will turn this into analyzer and CFE bugs.

[vsmenon]

@leafpetersen - have you had a chance to look at this? Do we need for Beta4?

[leafpetersen]

Filed separate issues as below.

CFE specific issues:

• default parameter: CFE does not reject non-const partial instantiations for default parameter values #32912

Analyzer specific issues:

• default parameter: Analyzer does not reject non-const partial instantiations for default parameter values #32913
• least closure: Analyzer does not eliminate type variables from const values #32918

[leafpetersen]

Closing this since the individual issues have been filed.