NNBD_TOP_MERGE(FutureOr<int?>, FutureOr*<int*>) produces incorrect result .

[iarkh]

Dart VM version: 2.8.0-edge.aadcb4418b1a7ccbb74a7cc925ad55020ce4a924 (Thu Feb 6 02:00:49 2020 +0000) on "linux_x64"

According to the NNBD Spec NNBD_TOP_MERGE(FutureOr<int?>, FutureOr*<int*>) should be FutureOr<int?>.

Please run the following source:
testlib_out.dart:

// @dart=2.6
import "dart:async";
class A<T> { Type getType() => T; }
class B extends A<FutureOr<int>> {}

test.dart:

import "testlib_out.dart";
import "dart:async";

Type typeOf<X>() => X;

class C extends A<FutureOr<int?>> {}
class D extends A<FutureOr<int>> {}

class E extends B implements C {}

main() {
  print(typeOf<FutureOr<int?>>() == E().getType());
  print(typeOf<FutureOr<int>> () == E().getType());
}

It produces the following output:

$ dart --enable-experiment=non-nullable test.dart
false
true

I.e. NNBD_TOP_MERGE result is FutureOr<int> instead of FutureOr<int?> here.

[chloestefantsova]

It appears that NNBD_TOP_MERGE works as expected for the given two types. I've added this case to our test cases for NNBD_TOP_MERGE: https://dart-review.googlesource.com/c/sdk/+/134823.

It seems that the observed behavior here is due to the run-time semantics of Type and related checks rather than due to NNBD_TOP_MERGE.

[crelier]

I am not sure that the expectations are correct here.
My understanding is that NNBD_TOP_MERGE is used to verify the signature of an overriding method, or to verify a signature when statically checking a method call. But NNBD_TOP_MERGE is not editing the source to change a signature.
The repro in this issue simply extracts the signature of method getType(), which in no way should be modified by NNBD_TOP_MERGE.
We have an instance constructed by E().
E extends B which extends A<T> with T == FutureOr<int>.

We call method getType() declared on the base class A.
This method has signature () => T, which returns the type argument of A, i.e. FutureOr<int*>*.

[eernstg]

@crelier wrote:

NNBD_TOP_MERGE is not editing the source to change a signature.

It is indeed a tricky issue whether the 'implements A<T>' relationship should use the type that is obtained from NNBD_TOP_MERGE only during compile-time subtype checking (on the object, and in member accesses), or it should also be reflected in the run-time representation. The current specification is probably not sufficiently detailed to justify any conclusions.

@crelier, is it correct to say that the approach you are describing will use the type argument specified for the superclass? For example:

// Opted in.
class A<X> { void foo() { print(X); }}
class B1 extends A<int?> {}
class B2 extends A<int> {}

// Opted out.
class C extends B1 implements B2 {}

// Opted in.
class D extends C {}

void main() {
  D().foo(); // 'int?'.
}

We will of course not achieve any notion of consistency, because the type system is unsound with respect to nullability until all code is nnbd.

But it seems more consistent to me if D().foo() prints int* (except that we might not see the *, but the intention is that X is int* in an instance of D).

After all, C is forced into resolving the conflict between implementing A<int?> and A<int> when it is migrated to nnbd, and it seems a bit fragile to insist that it should then resolve that conflict in favor of its superclass, rather than in favor of other superinterfaces. It should be possible for developers using C to rely on its interface and its supertypes, and they shouldn't be required to dive into its super-class structure in order to understand how to use it.

Note that I'm assuming that this is not prohibitively difficult to implement, or prohibitively expensive in terms of program size or speed.

Wouldn't it be possible to create each instance of D such that it has a type variable vector where X gets the value int*?

One thing that could make this difficult would be if methods in B1 and B2 are allowed to inline the value of X, because they "know" statically that X is int? respectively int. Does this kind of optimization actually happen today? If so, this would at least require such methods to be overridden by a fresh version in D where the inlined value of X is int*.

[eernstg]

Cf. dart-lang/language#841.

[crelier]

@eernstg wrote:

@crelier, is it correct to say that the approach you are describing will use the type argument specified for the superclass?

Yes, the implementation ignores implement clauses, except in type tests. Type arguments are solely defined by the chain of super types, and are not influenced by the interface types.

Technically, yes, "int?" would be printed in your example, but only if it was a valid example:

Error: 'D' can't implement both 'A<int?>' and 'A<int>'

Wouldn't it be possible to create each instance of D such that it has a type variable vector where X gets the value int*?

We could 'erase' nullability in super types of classes declared in legacy libraries. In this case, it would happen for class C (and D).

Your question about breaking a potential optimization is a good one. I do not think it is a problem, but I would have to double check.

[eernstg]

Cf. dart-lang/language#841 as well.

@crelier wrote:

Technically, yes, "int?" would be printed in your example, but only if it was a valid example:

It should be a valid example. https://github.com/dart-lang/language/pull/789/files was introduced in order to use the 'consolidated' approach whereby each class has an interface which is computed based on its direct superinterfaces, rather than traversing the entire superinterface graph.

During the discussions where we arrived at this model, we constantly used examples where a class indirectly implements conflicting superinterfaces (like List<int?> and List<int>), as well as examples where a single member has conflicting signatures (like int foo(int) and int? foo(int?)).

I just discussed this with Johnni, and it is also his understanding of the consolidated model: Each class has a set of member signatures, and a set of implemented interfaces, and that's what we refer to when considering static subtype relationships and instance method invocations etc. He also agrees that the class D in the example should be allowed.

The nnbd spec doesn't go into all details in this area, but it indicates that the legacy class should have a run-time representation where the type argument for the conflicting superinterface is the legacy type (end of this section):

The canonical interface which C is chosen to implement for the
purposes of runtime type checks is I<int*>.

In the mixin example here, I have created a class that has two applications of the same mixin with different type arguments. If the implementation expects to be able to look up the value of the type argument (say, if M contains print(X)) in a specific location then it would be difficult to support having two distinct values for X (that is int? for the application of M in class A, and int for the other one).

If there is a need to implement support in backends for having two distinct values for the type argument to M, and this is a non-trivial amount of work, and we won't need it when we arrive in pure nnbd land, it seems better to me to store the single value for that type argument which is consistent with the run-time type of the instance.

[crelier]

Reopening this issue, as it was closed prematurely (mea culpa).

I just revisited it and indeed, the example above is not rejected by CFE anymore.
By applying legacy erasure of the type arguments along the super class chain, as proposed in dart-lang/language#841, the example now prints int*. Note that this erasure applied by the VM is not yet committed, so you won't be able to reproduce it just yet.

However, the example for which this issue was opened is still behaving as before, because the erasure proposed in dart-lang/language#841 does not apply for this example.

Note that issue #40524 is related to this issue.

[eernstg]

Several things happened around dart-lang/language#841, and I'm proposing that we conclude that the type arguments passed along the superclass chain determine the run-time values of type variables. In particular, the mitigation based on NNBD_TOP_MERGE is ignored.

Consider the example from here:

// Opted in.
class A<X> { void foo() { print(X); }}
class B1 extends A<int?> {}
class B2 extends A<int> {}

// Opted out.
class C extends B1 implements B2 {}

// Opted in.
class D extends C {}

void main() {
  D().foo(); // 'int?'.
}

If we adopt the proposal I mentioned above then D().foo() will print 'int?', and that is also the current behavior of the vm (from commit a12c36d, of today).

@crelier wrote:

Note that this erasure applied by the VM is not yet committed

If we adopt the proposal I mentioned then the CL that performs this erasure should not be landed. I sincerely hope it did not take a big effort to create it.

There are several reasons why I ended up arguing that we should rely on the actual type arguments passed along the superclass chain (that is: I'm arguing that the question which is issue dart-lang/language#841 should be answered with a "no").

First, if any code is generated

// With null-safety.

Type typeOf<X>() => X;

class A<X> {
  bool get g => <X>[].runtimeType == typeOf<List<int>>();
}

class B1 extends A<int?> {
  bool get test => super.g; // Inlined, optimized to `=> false`.
}

class B2 extends A<int> {}

// Legacy.
class C extends B1 implements B2 {}

// With null-safety.
void main() {
  print(C.test);
}

If we insist that B1.test, when inherited by C, must behave as if the value of X in A is int* (which is the result of mitigation), then the program must print true. So the optimization in B1.test is suddenly invalid.

This means that we must generate overriding versions of some methods (or do something else, such that we don't end up having optimizations like the one in B1.test). But we can't just generate such an overriding version of test in C, because there could be more classes in the superclass chain from C to B1, and they could declare their own overriding versions of test, and call super.test, so we will not get the right semantics simply by generating a test in C which behaves as if A.X were int*.

By the way, we can't just use erasure along the superclass chain to obtain the appropriate result, because it's possible for the bottommost class (the role played by C in the example above) to implement A<int?> (a non-legacy type), and the inherited method may statically have evidence that it implements A<int*>. So the optimized code may rely on X being int*, and then we must perform a kind of "unerasure" along the superclass chain in order to align the value of X as seen by all reachable method implementations with the value that is used for is checks on the instance as a whole:

// With null-safety.

Type typeOf<X>() => X;

class A<X> {
  bool get g => <X>[].runtimeType == typeOf<List<int>>();
}

// Legacy.
class B1 extends A<int> {
  bool get test => super.g; // Inlined, optimized to `=> true`.
}

// With null-safety.
mixin M<X> on A<X> implements A<X> {
  bool get g => super.g;
}
class C extends B1 with M<int?> {}

void main() {
  print(C.test);
}

In this situation, C implements A<int?>, but the superclass chain contains A<int*>, so in order to make inherited method implementations behave as if the value of X is int? everywhere, we would have to undo the optimization in B1.test and "unerase" the value of X to int? up the superclass chain.

The situation is quite messy no matter which way we go, but I'm pretty sure the approach where we rely on the type arguments actually passed along the superclass chain is less messy than the version where we take the mitigated type argument value and force it into all superclasses.

[crelier]

@eernstg writes:

If we adopt the proposal I mentioned then the CL that performs this erasure should not be landed. I sincerely hope it did not take a big effort to create it.

No worries at all. I will abandon the CL and I am closing this issue, since the code behaves as expected and there is nothing to fix.