Skip to content

[CP] Flow analysis: Use extension type erasure for implicit is reachability. #54720

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

Closed
stereotype441 opened this issue Jan 24, 2024 · 2 comments
Assignees
Labels
cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve

Comments

@stereotype441
Copy link
Member

Commit(s) to merge

97edad1

Target

Beta

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/348200

Issue Description

When the type of the expression in a switch statement is an extension type, but a pattern used in the switch statement matches the corresponding representation type, the switch is exhaustive (because patterns are matched using runtime type checks, and extension types are erased at runtime). The exhaustiveness verifier understands this, but flow analysis does not. This can result in confusing "catch-22" situations where flow analysis leads to an error or warning, but attempting to correct that error or warning results in an "unreachable case" warning from the exhaustiveness verifier. For a detailed example of such a catch-22, see dart-lang/language#3534 (comment).

The opposite situation can also occur (if the type of the switch expression is the representation type and a pattern used in the switch statement matches the corresponding extension type).

What is the fix

To fix this issue, flow analysis was updated to take into account extension type erasure when determining reachability based on implicit is checks that occur as part of pattern matching.

Why cherry-pick

Since the problem is an interaction between extension types and patterns, and extension types have not been released, cherry-picking the fix will ensure that no stable version of Dart ships with the bug. Accordingly, the fix doesn't have to be classified as a breaking change.

If Dart 3.3 ships without the cherry pick, then we have three possible scenarios:

  • We could decide to cherry pick the fix to a patch release of Dart 3.3, resulting in the possibility that users who have already started using the "extension types" feature, and have inadvertently relied on the broken flow analysis behavior, will experience a code breakage when they upgrade their Dart installation. Fortunately, the breakage will be a compile-time error rather than a runtime error, so it is unlikely to result in users accidentally shipping buggy products.
  • We could do nothing, in which case users will experience a code breakage when Dart 3.4 is released. Furthermore, users who develop a package using Dart 3.4 and publish it on pub may find that their package has a compile time error when used with Dart 3.3 (even if the package author's SDK constraint indicates that their package should be compatible with Dart 3.3). This could be quite frustrating for package authors.
  • We could modify the existing fix so that it is conditioned on language version; this would avoid problems with packages published on pub, and it would ensure that any compile-time errors only occur when the user deliberately changes their language version (a time when there is some expectation of having to fix compile-time errors). But it would have the downside that we would be forced to maintain both the new and old behaviors in the codebase for years to come.

Note that all of these theoretical breakage scenarios will only occur if the user attempts to mix extension types and their corresponding representation types using pattern matching. This probably won't happen very often to begin with, because it's not a terribly good idea. So one could argue that the "do nothing" option isn't that bad. However, the inherent risk incurred by doing the cherry-pick is low for the same reason. So I would argue that the cherry-pick is worth doing irregardless.

Risk

low

Issue link(s)

dart-lang/language#3534

Extra Info

No response

@itsjustkevin
Copy link
Contributor

@leafpetersen could you take a look at this cherry-pick request?

@leafpetersen
Copy link
Member

I'm in favor of taking this in 3.3 if we can, otherwise in the first patch release. I think it's quite low risk.

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Jan 30, 2024
copybara-service bot pushed a commit that referenced this issue Jan 30, 2024
…achability.

Whenever a pattern performs an implicit `is` test, flow analysis
attempts to determine whether the `is` test is guaranteed to succeed;
if it is, then flow analysis considers the code path in which the `is`
test fails to be unreachable. This allows flow analysis to recognize
switch statements that are trivially exhaustive (because one of the
cases is guaranteed to match), avoiding spurious errors such as
"variable must be assigned before use" or "missing return statement".

This change upgrades the logic for computing when an `is` test is
guaranteed to succeed, so that it accounts for type erasure of
extension types. This brings flow analysis's treatment of switch
statements into closer alignment with the exhaustiveness checker,
which should reduce the risk of confusing error messages. For more
information see
dart-lang/language#3534 (comment).

Fixes dart-lang/language#3534.

Bug: dart-lang/language#3534
Change-Id: Ic3f840bbc5793aecf4f6aa4e569526e8482181fc
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/345822
Cherry-pick-request: #54720
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/348200
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve
Projects
None yet
Development

No branches or pull requests

8 participants