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

[breaking change] Compute closures of type schemas in the upper bound computations in the CFE as specified #56466

Closed
chloestefantsova opened this issue Aug 14, 2024 · 6 comments
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes

Comments

@chloestefantsova
Copy link
Contributor

Intended Change

The implementation of the algorithm for computing the upper bound of type schemas (also known as UP) will be adjusted. It will be aligned with the specification and the implementation of it in the Analyzer: the closures of the type schemas will be computed when the parts of the type schemas are passed into the subtyping algorithm, not at the very beginning of the upper bound computation.

Rationale

There is a discrepancy between the specification of the algorithm for computing the upper bound of type schemas and its implementation in the CFE. Moreover, the Analyzer complies to the specification in that regard, making the implementation in the CFE an outlier. Specifically, the CFE computes closures of the type schemas at the very beginning of computing the upper bound instead of computing closures of the parts of the schemas only when they are passed into the subtyping algorithm.

The discrepancy is a blocker for the upcoming language update implemented at https://dart-review.googlesource.com/c/sdk/+/364721.

Expected Impact

The impact of this change is expected to be very low, since none of the effects of the discrepancy were observed so far and the Analyzer is implementing the algorithm as specified, ensuring the code that passes through both tools assumes the specified behavior of the compiler. A trial run of this change over Google's internal Dart code base caused zero breakages.

However, it's theoretically possible that some code could change behavior as a result of this change. Here is an example of a program whose behavior would change:

// File: /tmp/qwerty.dart

class A<X extends A<X>> {
  A(X x);
}

test<Y extends A<Y>>(Y y) {
  A<Object?> a = new A(y);
  return a;
}

Today this program is accepted by the Analyzer, but induces the following compile-time error when compiled by the CFE:

/tmp/qwerty.dart:8:22: Error: Inferred type argument 'A<Object?>' doesn't conform to the bound 'A<X>' of the type variable 'X' on 'A'.
 - 'A' is from '/tmp/qwerty.dart'.
 - 'Object' is from 'dart:core'.
Try specifying type arguments explicitly so that they conform to the bounds.
  A<Object?> a = new A(y);
                     ^
/tmp/qwerty.dart:3:9: Context: This is the type variable whose bound isn't conformed to.
class A<X extends A<X>> {
        ^

With the change, both tools will accept the program.

Mitigation

In the unlikely event that some code is affected by the change, the old behavior may be restored by supplying explicit types where previously they were inferred. For example, in the program above the discrepancy in UP is visible only because the UP algorithm is used in inference of the type argument in the constructor invocation of A.

@chloestefantsova chloestefantsova added area-front-end Use area-front-end for front end / CFE / kernel format related issues. breaking-change-request This tracks requests for feedback on breaking changes labels Aug 14, 2024
@itsjustkevin
Copy link
Contributor

@johnmccutchan @leonsenft @mraleph please review this breaking change request.

@mraleph
Copy link
Member

mraleph commented Aug 14, 2024

LGTM

@leonsenft
Copy link

lgtm

@itsjustkevin
Copy link
Contributor

Marking this breaking change as accepted.

@johnmccutchan
Copy link
Contributor

lgtm

@chloestefantsova
Copy link
Contributor Author

The change has landed: https://dart-review.googlesource.com/c/sdk/+/375121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes
Projects
Status: Complete
Development

No branches or pull requests

5 participants