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

[vm] Incorrect FutureOr normalization on substitution #53737

Open
askeksa opened this issue Oct 12, 2023 · 2 comments
Open

[vm] Incorrect FutureOr normalization on substitution #53737

askeksa opened this issue Oct 12, 2023 · 2 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@askeksa
Copy link
Contributor

askeksa commented Oct 12, 2023

On the VM,

import 'dart:async';

void printType<T>() => print(T);

void substitute<T>() => printType<FutureOr<T>?>();

main() {
  substitute<FutureOr<String>?>();
}

prints

FutureOr<FutureOr<String>?>?

whereas it should print

FutureOr<FutureOr<String>?>

The outer FutureOr should normalize to declared non-nullable, since the type argument is nullable.

The bug is also triggered by the test added in https://dart-review.googlesource.com/c/sdk/+/320821

@askeksa askeksa added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 12, 2023
copybara-service bot pushed a commit that referenced this issue Oct 12, 2023
The type normalization rules specify that `FutureOr<T>?` is normalized
to `FutureOr<T>` when `T` is nullable. However, it's more practical
for subtype testing if the declared nullability on the runtime
representation of the `FutureOr` type reflects the true nullability
(nullable if the `FutureOr` is declared nullable or its type argument
is nullable), rather than being normalized as per the spec.

This changes the static and dynamic normalization rules in dart2wasm
thus and compensates by computing the proper spec normalization when
the type is converted to a string.

The added test exposed a number of bugs in DDC and the VM:

#53175
#53737
#53738

Change-Id: I0ad0a09fe935ccbd3eb65e6958c958d29e0bb088
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/320821
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Aske Simon Christensen <askesc@google.com>
@a-siva a-siva added P3 A lower priority bug or feature request triaged Issue has been triaged by sub team labels Oct 12, 2023
@lrhn
Copy link
Member

lrhn commented Oct 12, 2023

I'm not sure we guarantee normalization of toString, only of ==.
Is the resulting type unequal to a normalized type?

(It likely is unequal, otherwise it has to do runtime normalization anyway. Also to ensure having the same hash code.)

@askeksa
Copy link
Contributor Author

askeksa commented Oct 27, 2023

They are indeed unequal.

import 'dart:async';

bool compare<T, U>() => T == U;

bool substitute<T>() => compare<FutureOr<T>?, FutureOr<T>>();

main() {
  print(substitute<FutureOr<String>?>());
}

prints false on the VM, but it should print true.

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. P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

3 participants