Skip to content

NNBD_TOP_MERGE results for Object? vs dynamic/void depends on the type order in class declaration #40541

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
iarkh opened this issue Feb 8, 2020 · 4 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. NNBD Issues related to NNBD Release

Comments

@iarkh
Copy link
Contributor

iarkh commented Feb 8, 2020

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

NNBD Spec reads:

The NNBD_TOP_MERGE of two types T and S is the unique type R defined as:
NNBD_TOP_MERGE(Object?, dynamic) = Object?
And the reverse

Please run the following source code example:

class A<T> {
  void test() { print(T); }
}

class B extends A<Object?> {}
class C extends A<dynamic> {}

class D1 extends B implements C {}
class D2 extends C implements B {}

void main() {
  D1().test();
  D2().test();
}

Dart prints:

$ dart --enable-experiment=non-nullable test.dart
Object
dynam

Seems like both D1().test(); and D2().test(); should print Object here.

The same is true for NNBD_TOP_MERGE(Object?, void).

@vsmenon vsmenon added the legacy-area-front-end Legacy: Use area-dart-model instead. label Feb 9, 2020
@chloestefantsova
Copy link
Contributor

It appears that NNBD_TOP_MERGE works as expected for the given two types. It is already covered in the unit tests for NNBD_TOP_MERGE:

'Object? vs dynamic': 'Object?',
. Note that the unit test makes the direct and the reverse check, that is, both Object? vs dynamic and dynamic vs Object? are checked and the result is Object? in both cases. Similarly for Object? vs void and void vs Object?, only the result for that is void in both cases.

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

@chloestefantsova chloestefantsova added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. NNBD Issues related to NNBD Release and removed legacy-area-front-end Legacy: Use area-dart-model instead. labels Feb 10, 2020
@crelier
Copy link
Contributor

crelier commented Apr 10, 2020

The previous comment is correct. NNBD_TOP_MERGE does not modify how methods are looked up and does not modify the value of type parameters, at least not until dart-lang/language#841 is adopted (but it would not apply to this example).
See related discussions in #40524 and #40553.

@crelier
Copy link
Contributor

crelier commented Apr 20, 2020

Since the consensus is that mitigation proposed in dart-lang/language#841 will not be implemented, this issue becomes invalid and there is nothing to fix in the VM.

@crelier crelier closed this as completed Apr 20, 2020
@eernstg
Copy link
Member

eernstg commented Apr 21, 2020

Just to clarify: The point here is that NNBD_TOP_MERGE does not determine the value of T during an execution of A.test, that value is determined directly by the type arguments passed through the superclass chain. So in D1().test(), T has the value Object? as seen from the body of test, and in D2().test(), T has the value dynamic as seen from the body of test.

It is also true that both D1 and D2, due to mitigation, are considered to implement A<Object?>. But that property is used for tests like D1() is SomeType and for static checks on SomeType x = D1();.

So if a test is supposed to determine what NNBD_TOP_MERGE does in different situations, it cannot rely on accessing a type argument like T in a method like A.test, because the value computed by NNBD_TOP_MERGE is not guaranteed to be the same value as the one seen by A.test.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

5 participants