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] Fix pattern context type schema for cast patterns. #54640

Closed
stereotype441 opened this issue Jan 16, 2024 · 7 comments
Closed
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes

Comments

@stereotype441
Copy link
Member

Change Intent

The pattern context type schema for cast patterns will be changed from Object? to _ (the unknown type), to align with the specification.

Justification

When the compiler encounters a pattern variable declaration or a pattern assignment, it derives a pattern type schema from the pattern on the left hand side of the =, and uses it for type inference of the expression on the right hand side of the =. A type schema is like a type, except that it can have holes in it representing parts of the type that are not yet known. (These holes are written as _, which is called the "unknown type"). For example, in the pattern variable declaration:

var (List<num> x, [...y]) = ([1], [2]);

The pattern type schema is (List<num>, List<_>); this constrains the type of [1] to be List<num>, so it is treated as though the user wrote <num>[1]. But it doesn't place any constraint on the type of [2] (other than the fact that it must be a list of some sort), so type inference concludes that the type of [2] is List<int> (because the list contains all integers), and so it is treated as though the user wrote <int>[2].

When the patterns feature was added to Dart in 3.0, a small implementation error was made in the computation of pattern type schemas. The pattern type schema for a cast pattern is specified to be _. So, for example, in the pattern variable declaration:

var [x as int, y as double] = [1, 2.0];

The intention was for the pattern type schema to be List<_>, and so the list literal [1, 2.0] would take on the type required by its elements, namely List<num>. That is, it would be compiled as though the user had written <num>[1, 2.0].

However, what was actually implemented was that the pattern type schema for a cast pattern is Object?. So in the example above, the pattern type schema is List<Object?>, and so the list literal [1, 2.0] is interpreted as <Object?>[1, 2.0].

The proposed change will bring the implementation in line with the specification.

Impact

This change is not expected to make any difference in practice. When it was tested on the google internal code base, only a single line of code was affected at all, and the only effect on that code was that a cast pattern that was previously thought to be necessary was correctly detected by the compiler as unnecessary (resulting in a warning, not an error).

However, it's theoretically possible that the change could cause a type to be inferred differently in a way that causes a code breakage. For example, this code is accepted today:

main() {
  var (x && [y as int]) = [0]; // (1)
  x.add('foo');
}

It's accepted because the pattern type schema for the declaration at (1) is List<Object?>, and hence [0] is interpreted as <Object?>[0], so x.add('foo') is allowed.

But when the change is made, this code will be rejected, because the pattern type schema for the declaration at (1) will be List<_>, and hence [0] will be interpreted as <int>[0], and it is not allowed to add a string to a list of int.

Mitigation

In the unlikely event that some code is affected by this change, the old functionality can be restored by adding explicit type annotations. For instance, in the above example, the code could be changed to:

main() {
  var (x && [y as int]) = <Object?>[0]; // (1)
  x.add('foo');
}
@stereotype441 stereotype441 added the breaking-change-request This tracks requests for feedback on breaking changes label Jan 16, 2024
@itsjustkevin
Copy link
Contributor

@vsmenon @grouma @Hixie could you take a look at this breaking change request?

@Hixie
Copy link
Contributor

Hixie commented Jan 16, 2024

no objection

@a-siva a-siva added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jan 17, 2024
copybara-service bot pushed a commit that referenced this issue Jan 17, 2024
I discovered recently that when we implemented the "patterns" feature,
we got the pattern type schema for cast patterns wrong--it was
specified to be `_`, but what was actually implemented was
`Object?`. I've filed a breaking change request to address that:
#54640.

In the course of prototyping a fix, I was surprised to find that no
language tests or co19 tests were affected; it appears that we don't
have adequate test coverage of pattern type schemas. This CL addresses
the lack of test coverage by adding a language test.

The test case for cast patterns is currently commented out, pending
resolution of the breaking change request; after the breaking change
lands, I will update the test accordingly.

Change-Id: I0d27fd8ac65f16d21a8597586e6f76fd9a5ba86e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/346621
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Bob Nystrom <rnystrom@google.com>
@grouma
Copy link
Member

grouma commented Jan 18, 2024

SGTM.

@vsmenon
Copy link
Member

vsmenon commented Jan 23, 2024

@leafpetersen - what is your recommendation on this?

@leafpetersen
Copy link
Member

LGTM

@vsmenon
Copy link
Member

vsmenon commented Jan 26, 2024

lgtm

copybara-service bot pushed a commit that referenced this issue Jan 31, 2024
According to the patterns spec, the pattern context type schema for a
cast pattern should be `_`. What was actually implemented was
`Object?`.

This is unlikely to make a difference in practice, since (a) cast
patterns are unlikely to be used in circumstances where the pattern
context type schema makes a difference, and (b) it takes some effort
to come up with expressions whose type inference behavior is differenc
between a schema of `Object?` and a schema of `_`.

Change-Id: I695752c8c163621a34faaa8d62b2b076c8152eb0
Bug: #54640
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/346383
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
@stereotype441
Copy link
Member Author

Fixed by 3636c8a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes
Projects
None yet
Development

No branches or pull requests

7 participants