Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Wrong expectation in normalization test. #42143

Closed
crelier opened this issue Jun 1, 2020 · 5 comments
Closed

Wrong expectation in normalization test. #42143

crelier opened this issue Jun 1, 2020 · 5 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

@crelier
Copy link
Contributor

crelier commented Jun 1, 2020

A new normalization test introduced in this CL does not include requirements to run either in weak or strong mode, therefore, it runs in both modes.

On the VM (a non yet committed version including fixes), this test fails in both modes due to wrong expectations.

Weak mode:

DART_CONFIGURATION=DebugX64 out/DebugX64/dart --enable_asserts --no-null-safety --enable-experiment=non-nullable --ignore-unrecognized-flags --packages=/usr/local/google/home/regis/dart2/sdk/.packages /usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart

exit code:
255

stderr:
Unhandled exception:
Expect.notEquals(unexpected: <<S extends Future<Never>?>() => Never>, actual:<<S extends Future<Never>>() => Never>) fails.
#0      Expect._fail (package:expect/expect.dart:685:5)
#1      Expect.notEquals (package:expect/expect.dart:306:5)
#2      checkNotEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/type_builder.dart:134:10)
#3      checkTypeNotEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:19:3)
#4      SimpleBoundTests.checkAtBottomType (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:80:7)
#5      SimpleBoundTests.checkBottomTypes (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:88:5)
#6      SimpleBoundTests.check (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:130:5)
#7      main (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:195:20)
#8      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
#9      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

The bounds Future<Never>? and Future<Never> are mutual subtypes, since their nullability is ignored in weak mode.

In strong mode:

DART_CONFIGURATION=DebugX64 out/DebugX64/dart --null-safety --enable-experiment=non-nullable --ignore-unrecognized-flags --packages=/usr/local/google/home/regis/dart2/sdk/.packages /usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart

exit code:
255

stderr:
Unhandled exception:
Expect.notEquals(unexpected: <<S extends FutureOr<Future<Object>>>() => Future<Object>>, actual:<<R extends Future<Object>>() => Future<Object>>) fails.
#0      Expect._fail (package:expect/expect.dart:685:5)
#1      Expect.notEquals (package:expect/expect.dart:306:5)
#2      checkNotEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/type_builder.dart:134:10)
#3      checkTypeNotEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:19:3)
#4      SimpleBoundTests.checkAtNonNullableType (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:96:5)
#5      SimpleBoundTests.checkNonNullableTypes (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:107:5)
#6      SimpleBoundTests.check (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:131:5)
#7      main (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:195:20)
#8      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
#9      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

Here also, the bounds FutureOr<Future<Object>> and Future<Object> are mutual subtypes.
It is easy to see that Future<Object> is a subtype of FutureOr<Future<Object>>.
The reverse is less obvious. We have to show that Future<Future<Object>> is a subtype of Future<Object> and it is, because Future<Object> is a subtype of Object.

@crelier crelier added the area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test). label Jun 1, 2020
@crelier
Copy link
Contributor Author

crelier commented Jun 4, 2020

Running the test language/nnbd/normalization/generic_function_type_object_normalization_test.dart in strong mode, the VM encounters two further issues after the one reported above.

The second issue occurs at line 191:
checkTypeNotEquals2(o.i1<Never>(), o.i2<Never>());
where the two types are actually equal. There is a comment near the declaration of i1 and i2 about a CFE issue and what looks like a temporary workaround for this issue, but the workaround is not correct, as the two function types are equal.

The third issue occurs at line 191:

Expect.equals(expected: <<T extends FutureOr<T>?>() => void>, actual: <<S extends FutureOr<S>>() => void>) fails.
#0      Expect._fail (package:expect/expect.dart:685:5)
#1      Expect.equals (package:expect/expect.dart:132:5)
#2      checkEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/type_builder.dart:114:10)
#3      checkTypeEquals2 (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:13:3)
#4      fBoundedTests (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:191:3)
#5      main (file:///usr/local/google/home/regis/dart2/sdk/tests/language/nnbd/normalization/generic_function_type_object_normalization_test.dart:197:3)
#6      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
#7      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

Although the source looks like this

  void h1<T extends FutureOr<T?>?>() {}
  void h2<S extends FutureOr<S?>>() {}

  checkTypeEquals2(h1, h2);

the kernel file looks like this

    function h1<T extends asy::FutureOr<T%>? = asy::FutureOr<dynamic>?>() → void {}
    function h2<S extends asy::FutureOr<S%> = asy::FutureOr<dynamic>>() → void {}
    gen::checkTypeEquals2(h1, h2);

which explains the error reported by the VM. The nullability of the type parameters is not correct in the kernel file. Is this a known CFE issue?

@nshahan
Copy link
Contributor

nshahan commented Jun 5, 2020

cc @johnniwinther

I'm seeing the same errors from my work in progress change in DDC.

@johnniwinther johnniwinther added area-front-end Use area-front-end for front end / CFE / kernel format related issues. NNBD Issues related to NNBD Release and removed area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test). labels Jun 8, 2020
@johnniwinther
Copy link
Member

Changed to the issue to a front-end issue for the CFE problem. Will change back if there is more work needed on the test itself after the CFE part has been fixed.

dart-bot pushed a commit that referenced this issue Jun 12, 2020
In order to normalize 'X extends Never' to 'Never' (and 'X? extends Never' to 'Null'), the VM has to instantiate the bounds of function type parameters.
Instantiating these bounds lazily is not easily feasible, as the instantiators are not kept around after instantiating types.

Note that the normalization tests do not yet pass due to test issues and CFE issues, see #42143


Change-Id: I9ed6fcb9d7a11a4adbe514757ad9ac6ff7818981
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/147281
Commit-Queue: Régis Crelier <regis@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
@johnniwinther
Copy link
Member

Reopening. The CFE issue has been fixed but none of the mentioned tests were fixed by it so some work seems to be needed on the backend side.

@johnniwinther johnniwinther added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. and removed area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Jun 16, 2020
@johnniwinther johnniwinther reopened this Jun 16, 2020
@crelier
Copy link
Contributor Author

crelier commented Jun 16, 2020

I think that the remaining failures are due to wrong expectations rather than to backend problems. I sent a CL fixing the test: https://dart-review.googlesource.com/c/sdk/+/151440

@johnniwinther The CFE issue #41952 previously mentioned in a comment in the test seems to have been fixed and should probably be closed.

dart-bot pushed a commit that referenced this issue Jun 17, 2020
This fixes issue #42143

Change-Id: I11df42728f74e85071566b355d1a3bbba4182aea
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151440
Reviewed-by: Leaf Petersen <leafp@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Régis Crelier <regis@google.com>
@crelier crelier closed this as completed Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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