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

fasta: `Type` values obtained from a typedef should represent the underlying type, not the typedef #31359

Open
sigmundch opened this Issue Nov 11, 2017 · 11 comments

Comments

5 participants
@sigmundch
Member

sigmundch commented Nov 11, 2017

Edit by eernstg, Apr 6 2018: This is now the fasta specific issue for #32782.


We represent typedefs as function types when the typedef is used as a value. This is problematic because it breaks when using the == operator and printing.

Here is simple repro:

typedef int F();
typedef int G();

main() {
 var x = F;
 print(x);
 print(F == G);
}

Running this prints F and false on the VM, but () => int and true if using fasta.

For context - this issue was discovered while trying to compile an app with dart2js, I don't believe there is a work around on our side.

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Nov 11, 2017

Member

Fasta is correct.
The Type object corresponding to a function type must be equal to another Type object representing the same function type. Function types are structural, so F and G both represent the function type dynamic Function(), so their corresponding Type objects must be equal.

The toString of Type objects is not specified anywhere, so it can't be wrong.
To be even marginally useful, we should decide on something and do the same thing in every implementation. Either be completely opaque ("Function") or provide something easily parsable - because then people will parse them no matter how much we discourage it - say dynamic Function(), now that we actually have an in-language syntax for function types.

Member

lrhn commented Nov 11, 2017

Fasta is correct.
The Type object corresponding to a function type must be equal to another Type object representing the same function type. Function types are structural, so F and G both represent the function type dynamic Function(), so their corresponding Type objects must be equal.

The toString of Type objects is not specified anywhere, so it can't be wrong.
To be even marginally useful, we should decide on something and do the same thing in every implementation. Either be completely opaque ("Function") or provide something easily parsable - because then people will parse them no matter how much we discourage it - say dynamic Function(), now that we actually have an in-language syntax for function types.

@sigmundch

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Nov 13, 2017

Member

Thanks @lrhn for clarifying.

If that is the case, this might mean we need to fix all implementations to match the structural equality. I believe right now both the VM and dart2js have the nominal semantics. We have also seen angular apps that use typedefs as keys in DI maps and that might break if we change this.

/cc @hterkelsen - do you know how much of a breaking change this would be?

Member

sigmundch commented Nov 13, 2017

Thanks @lrhn for clarifying.

If that is the case, this might mean we need to fix all implementations to match the structural equality. I believe right now both the VM and dart2js have the nominal semantics. We have also seen angular apps that use typedefs as keys in DI maps and that might break if we change this.

/cc @hterkelsen - do you know how much of a breaking change this would be?

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Nov 13, 2017

Member

For the record, the Fasta behavior is likely the result of a mail discussion we had between the Kernel and language teams (and probably a few other people) back in January about the shortcomings of the specification around Type objects.

The specification, and class documentation, is silent on, well, everything. The entirety of the documentation on Type objects from library and specification is the sentence "Runtime representation of a type."

The specification tells us how to create Type objects as:

If d is a class or type alias T, the value of e is an instance of class Type (or a subclass thereof) reifying T.

That's vague enough that I can't decide whether F represents the declaration or the type. If it's the declaration, then the Type object really is nothing but a token that is usable by the mirror system, and we should never use it for anything else. That would make them completely useless if you don't use the mirror system
Since Type objects otherwise represent types (they are the return values of .runtimeType), I'd go with types over declarations. The current representation of typedefs seem to favor the "declaration" interpretation.

In either case, there is not enough specification to make any behavior formally right or wrong.

There is, however, desirable behavior, and if the Type object is to be useful for anything except reflection, it needs to be equal to other objects representing the same type.
Example:

typedef F();
class C<T> { get type => T; }   // Another way to extract a type.
main() { 
  print(F == new C<F>().type);  
}

If this doesn't print true, then I hope we can agree that the type object corresponding to F is truly useless.

It prints false in dart2js and true in the VM. Only halfway useless.

Another example:

typedef F();
typedef G();
main() {
  print(<F>[].runtimeType == <G>[].runtimeType); 
}

Again, I'd want this to be true. It's true in dart2js, false in the VM.

Member

lrhn commented Nov 13, 2017

For the record, the Fasta behavior is likely the result of a mail discussion we had between the Kernel and language teams (and probably a few other people) back in January about the shortcomings of the specification around Type objects.

The specification, and class documentation, is silent on, well, everything. The entirety of the documentation on Type objects from library and specification is the sentence "Runtime representation of a type."

The specification tells us how to create Type objects as:

If d is a class or type alias T, the value of e is an instance of class Type (or a subclass thereof) reifying T.

That's vague enough that I can't decide whether F represents the declaration or the type. If it's the declaration, then the Type object really is nothing but a token that is usable by the mirror system, and we should never use it for anything else. That would make them completely useless if you don't use the mirror system
Since Type objects otherwise represent types (they are the return values of .runtimeType), I'd go with types over declarations. The current representation of typedefs seem to favor the "declaration" interpretation.

In either case, there is not enough specification to make any behavior formally right or wrong.

There is, however, desirable behavior, and if the Type object is to be useful for anything except reflection, it needs to be equal to other objects representing the same type.
Example:

typedef F();
class C<T> { get type => T; }   // Another way to extract a type.
main() { 
  print(F == new C<F>().type);  
}

If this doesn't print true, then I hope we can agree that the type object corresponding to F is truly useless.

It prints false in dart2js and true in the VM. Only halfway useless.

Another example:

typedef F();
typedef G();
main() {
  print(<F>[].runtimeType == <G>[].runtimeType); 
}

Again, I'd want this to be true. It's true in dart2js, false in the VM.

@hterkelsen

This comment has been minimized.

Show comment
Hide comment
@hterkelsen

hterkelsen Nov 13, 2017

Member

I'll try to get some numbers on if we have customers making use of the "declaration interpretation"

Member

hterkelsen commented Nov 13, 2017

I'll try to get some numbers on if we have customers making use of the "declaration interpretation"

@sigmundch sigmundch added the p3-low label Nov 29, 2017

@sigmundch

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Nov 29, 2017

Member

we are still investigating whether the fix should be in dart2js instead - I've marked this as "low" to indicate somehow that this is not something the FE team should actively look into yet.

Member

sigmundch commented Nov 29, 2017

we are still investigating whether the fix should be in dart2js instead - I've marked this as "low" to indicate somehow that this is not something the FE team should actively look into yet.

@kmillikin kmillikin added this to Incoming in Dart Front End Jan 3, 2018

@jensjoha jensjoha removed this from Incoming Untriaged in Dart Front End Jan 4, 2018

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Mar 22, 2018

Member

Is this issue fixed (as in F==G being true) by switching everybody to the common front-end, or do we still need back-ends to change?

Member

lrhn commented Mar 22, 2018

Is this issue fixed (as in F==G being true) by switching everybody to the common front-end, or do we still need back-ends to change?

@eernstg

This comment has been minimized.

Show comment
Hide comment
@eernstg

eernstg Mar 22, 2018

Member

I suspect that the current title ('Do not convert typedefs used as values to a type') is misleading given that the goal is now to ensure that Type entities obtained from type aliases should represent the underlying (structural) type rather than being opaque tokens for the type alias itself. I'll adjust the title accordingly.

Member

eernstg commented Mar 22, 2018

I suspect that the current title ('Do not convert typedefs used as values to a type') is misleading given that the goal is now to ensure that Type entities obtained from type aliases should represent the underlying (structural) type rather than being opaque tokens for the type alias itself. I'll adjust the title accordingly.

@eernstg eernstg changed the title from Do not convert typedefs used as values to a type to `Type` values obtained from a typedef should represent the underlying type, not the typedef Mar 22, 2018

@sigmundch

This comment has been minimized.

Show comment
Hide comment
@sigmundch

sigmundch Mar 22, 2018

Member

Yes - I think we can close this issue as working as intended

Member

sigmundch commented Mar 22, 2018

Yes - I think we can close this issue as working as intended

@eernstg

This comment has been minimized.

Show comment
Hide comment
@eernstg

eernstg Mar 23, 2018

Member

Are you sure it is working? I just checked, and here's an excerpt from a trybot log, showing that the configuration includes dart2js --use-kernel --strong , which I interpret to mean that it is relevant for 'area-front-end' and 'Customer-dart2js':

Running "dart2js" command: DART_CONFIGURATION=ReleaseIA32 out/ReleaseIA32/dart-sdk/bin/dart2js --sync-async --generate-code-with-compile-time-errors --test-mode --allow-mock-compilation --categories=all --minify --use-kernel --strong --packages=/b/swarming/w/ir/.packages /b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/enum_syntax_test_07.dart --out=/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_enum_syntax_test_07/out.js
FAILED: dart2js-d8 release_ia32 language_2/type_alias_equality_test/01
Expected: Pass
Actual: RuntimeError
--- Command "dart2js" (took 866ms):
DART_CONFIGURATION=ReleaseIA32 out/ReleaseIA32/dart-sdk/bin/dart2js --sync-async --generate-code-with-compile-time-errors --test-mode --allow-mock-compilation --categories=all --minify --use-kernel --strong --packages=/b/swarming/w/ir/.packages /b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/type_alias_equality_test_01.dart --out=/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
exit code:
0
stdout:
Compiled 8,847,518 characters Dart to 26,765 characters JavaScript in 0.86 seconds
Dart file (/b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/type_alias_equality_test_01.dart) compiled to JavaScript: out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
--- Command "d8" (took 72ms):
DART_CONFIGURATION=ReleaseIA32 /b/swarming/w/ir/third_party/d8/linux/d8 out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
exit code:
1
stdout:
out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:263: Expect.equals(expected: <aU>, actual: <aV>) fails.
          throw e;
          ^
Expect.equals(expected: <aU>, actual: <aV>) fails.
    at Object.b (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:303:3)
    at Object.ah (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:715:24)
    at Object.ai (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:714:3)
    at a0 (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:718:17)
    at action (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:275:31)
    at eventLoop (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:258:9)
    at self.dartMainRunner (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:276:5)
    at /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:836:39
    at init.currentScript (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:832:55)
    at /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:835:84
--- Re-run this test:
python tools/test.py -m release -c dart2js -r d8 -a ia32 --strong --minified --dart2js-with-kernel --use-sdk --output-directory /b/swarming/w/ioVoJYyK --dart2js-batch --reset-browser-configuration language_2/type_alias_equality_test/01
Done dart2js-d8 release_ia32 language_2/type_alias_equality_test/01: fail

The message Expect.equals(expected: <aU>, actual: <aV>) fails seems to imply that the comparison is concerned with type alias names rather than the underlying type.

The failing trybots were dart2js-linux-d8-kernel-minified-try, ddc-linux-release-chrome-try, vm-kernel-mac-release-x64-try, vm-linux-release-x64-try, and vm-mac-release-x64-try (there were some other failures, but they all fail on the test I'm adding in this CL: 'type_alias_equality_test.dart'), so we might need to create a meta bug, or change this to a meta bug, such that we can handle the issue with other tools (e.g., ddc) as well.

Member

eernstg commented Mar 23, 2018

Are you sure it is working? I just checked, and here's an excerpt from a trybot log, showing that the configuration includes dart2js --use-kernel --strong , which I interpret to mean that it is relevant for 'area-front-end' and 'Customer-dart2js':

Running "dart2js" command: DART_CONFIGURATION=ReleaseIA32 out/ReleaseIA32/dart-sdk/bin/dart2js --sync-async --generate-code-with-compile-time-errors --test-mode --allow-mock-compilation --categories=all --minify --use-kernel --strong --packages=/b/swarming/w/ir/.packages /b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/enum_syntax_test_07.dart --out=/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_enum_syntax_test_07/out.js
FAILED: dart2js-d8 release_ia32 language_2/type_alias_equality_test/01
Expected: Pass
Actual: RuntimeError
--- Command "dart2js" (took 866ms):
DART_CONFIGURATION=ReleaseIA32 out/ReleaseIA32/dart-sdk/bin/dart2js --sync-async --generate-code-with-compile-time-errors --test-mode --allow-mock-compilation --categories=all --minify --use-kernel --strong --packages=/b/swarming/w/ir/.packages /b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/type_alias_equality_test_01.dart --out=/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
exit code:
0
stdout:
Compiled 8,847,518 characters Dart to 26,765 characters JavaScript in 0.86 seconds
Dart file (/b/swarming/w/ir/out/ReleaseIA32/generated_tests/language_2/type_alias_equality_test_01.dart) compiled to JavaScript: out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
--- Command "d8" (took 72ms):
DART_CONFIGURATION=ReleaseIA32 /b/swarming/w/ir/third_party/d8/linux/d8 out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js
exit code:
1
stdout:
out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:263: Expect.equals(expected: <aU>, actual: <aV>) fails.
          throw e;
          ^
Expect.equals(expected: <aU>, actual: <aV>) fails.
    at Object.b (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:303:3)
    at Object.ah (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:715:24)
    at Object.ai (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:714:3)
    at a0 (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:718:17)
    at action (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:275:31)
    at eventLoop (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:258:9)
    at self.dartMainRunner (out/ReleaseIA32/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js:276:5)
    at /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:836:39
    at init.currentScript (/b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:832:55)
    at /b/swarming/w/ir/out/ReleaseIA32/generated_compilations/dart2js-strong-minified-sdk/multitest_language_2_type_alias_equality_test_01/out.js:835:84
--- Re-run this test:
python tools/test.py -m release -c dart2js -r d8 -a ia32 --strong --minified --dart2js-with-kernel --use-sdk --output-directory /b/swarming/w/ioVoJYyK --dart2js-batch --reset-browser-configuration language_2/type_alias_equality_test/01
Done dart2js-d8 release_ia32 language_2/type_alias_equality_test/01: fail

The message Expect.equals(expected: <aU>, actual: <aV>) fails seems to imply that the comparison is concerned with type alias names rather than the underlying type.

The failing trybots were dart2js-linux-d8-kernel-minified-try, ddc-linux-release-chrome-try, vm-kernel-mac-release-x64-try, vm-linux-release-x64-try, and vm-mac-release-x64-try (there were some other failures, but they all fail on the test I'm adding in this CL: 'type_alias_equality_test.dart'), so we might need to create a meta bug, or change this to a meta bug, such that we can handle the issue with other tools (e.g., ddc) as well.

@eernstg eernstg reopened this Mar 23, 2018

@eernstg

This comment has been minimized.

Show comment
Hide comment
@eernstg

eernstg Apr 5, 2018

Member

I've written a test for this, added in c98df5b. Status file updates are included in that commit, because several tools fail to pass the test (currently dart2js, dartdevc, fasta, and the vm fail, at least in some configurations). Given that this affects several tools, I've created a new meta-bug #32782 for this, and this issue is henceforth the fasta specific issue for #32782. I updated the initial comment on this issue to to say so. This issue used to have the label Resolution: works as intended, but the test fails as specified in 'language_2_kernel.status', so I removed that label.

Member

eernstg commented Apr 5, 2018

I've written a test for this, added in c98df5b. Status file updates are included in that commit, because several tools fail to pass the test (currently dart2js, dartdevc, fasta, and the vm fail, at least in some configurations). Given that this affects several tools, I've created a new meta-bug #32782 for this, and this issue is henceforth the fasta specific issue for #32782. I updated the initial comment on this issue to to say so. This issue used to have the label Resolution: works as intended, but the test fails as specified in 'language_2_kernel.status', so I removed that label.

@eernstg eernstg changed the title from `Type` values obtained from a typedef should represent the underlying type, not the typedef to dart2js: `Type` values obtained from a typedef should represent the underlying type, not the typedef Apr 5, 2018

@eernstg eernstg changed the title from dart2js: `Type` values obtained from a typedef should represent the underlying type, not the typedef to fasta: `Type` values obtained from a typedef should represent the underlying type, not the typedef Apr 5, 2018

@kmillikin kmillikin added this to Incoming Untriaged in Dart Front End Apr 23, 2018

@kmillikin kmillikin added the p2-medium label Sep 12, 2018

@kmillikin kmillikin removed the p3-low label Sep 12, 2018

dart-bot pushed a commit that referenced this issue Sep 14, 2018

This CL changes some status comments, but also points out an issue.
The point is that #32782 requests a fix such that
`typedef F1 = void Function(int)` and `typedef void F2(int)`
satisfy that `F1 == F2` evaluates to true. This is marked
'p3-low' for the common front end but 'p1-high' for DDC.

It seems likely to me that this could create the situation where
code is developed using DDC, is working, and then fails upon
deployment using the vm. Also, there is a single case where
`dart2js` fails in the associated `type_alias_equality_test.dart`,
so deployment on the web would also fail upon deployment, though
only in some of the cases.

However, with some input from Aske I concluded that the situation
might have arisen because there _is_ no work to do for this in the
common front end, because it will be handled by the backend (which
also explains why `dart2js` has it almost right).

This CL is just introducing a tiny change: It changes the issue
indicated for all VM related failures in said test to point to
#32783, which is presumably the right issue for backend work.

Apart from that, I've added you, the reviewers, in order to make
sure that the relevant people get this heads up. We may then decide
to land this CL, change the priority on #31359, do nothing, or
whatever turns out to be the right response. ;-)

Change-Id: I92672547d7fe795e877604c0da1e0e4579e4e04a
Reviewed-on: https://dart-review.googlesource.com/74403
Reviewed-by: Jenny Messerly <jmesserly@google.com>
@eernstg

This comment has been minimized.

Show comment
Hide comment
@eernstg

eernstg Sep 26, 2018

Member

FYI: The language specification has been clarified to specify explicitly that 'Type values obtained from a typedef should represent the underlying type, not the typedef', in 4816b60, line 10506-10508.

Member

eernstg commented Sep 26, 2018

FYI: The language specification has been clarified to specify explicitly that 'Type values obtained from a typedef should represent the underlying type, not the typedef', in 4816b60, line 10506-10508.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment