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] Make type inference of e1 ?? e2 consistent. #55436

Closed
stereotype441 opened this issue Apr 11, 2024 · 11 comments
Closed

[Breaking Change] Make type inference of e1 ?? e2 consistent. #55436

stereotype441 opened this issue Apr 11, 2024 · 11 comments
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

The rules used by the compiler front end to perform type inference on if-null expressions (expressions of the form e1 ?? e2) will be changed to match the behavior of the analyzer. The change is as follows: when the context for the entire e1 ?? e2 expression is dynamic, the context for e2 will be the static type of e1.

This change is expected to have low impact on real-world code. But in principle it could cause compile-time errors or changes in runtime behavior.

Background

When the compiler needs to perform type inference on an expression, it does so using a type schema known as the "context". A type schema is a generalization of the normal Dart type syntax, in which _ (called the "unknown type") can appear where a type is expected.

In the analyzer, any time an expression would be analyzed with a context of dynamic, the context is coerced to _ before performing the analysis; this causes contexts of dynamic and _ to behave identically1. In the compiler front end, dynamic is coerced to _ when analyzing a generic invocation, but dynamic and _ behave differently for a few expression types. This breaking change addresses one of those differences, which is in the behavior of if-null expressions (expressions of the form e1 ?? e2).

The current behavior for if-null expressions is as follows. If the context for e1 ?? e2 is K, then type inference first analyzes e1 using the context K?. Then, it analyzes e2 using the context L, where L is computed as follows:

  1. If K is _, then L is the static type of e1.
  2. If K is dynamic, then in the compiler front end, L is dynamic; in the analyzer, L is the static type of e1.2
  3. Otherwise, L is K.

Intended Change

The above rules will be changed so that in the compiler front end, if K is dynamic, then L will be the static type of e1, as it is in the analyzer.

Justification

Any difference in type inference behavior between the analyzer and the compiler front end is a bug. To reduce the impact of the bug fix, it makes sense to standardize on either the analyzer's behavior or the compiler front end's behavior. In this case, standardizing on the analyzer behavior is better, since the analyzer is more self-consistent (it always treats contexts of dynamic and _ the same). Once this change and #55418 are made, there will not be any scenarios in which the compiler front end treats dynamic and _ contexts differently.

Expected Impact

A prototype of this change caused zero test failures in Google's internal codebase, so the impact is expected to be low for real-world code.

But it is theoretically possible for a program to behave differently with the change. Here is a contrived example:

main() {
  List<int>? x = null;
  dynamic y = x ?? [];
  print(y.runtimeType);
  y.add('str');
}

Today, this program prints List<dynamic> and then runs to completion; with the change, it will print List<int> and then terminate with an exception.

  • Since the declaration of y has an explicit type of dynamic, the context dynamic is used for type inference of the expression x ?? [].
  • The first step in type inferring x ?? [] is to type infer x using a context of dynamic? (which is the same as dynamic). Since x is simply a reference to a local variable, its static type is the type of that local variable, List<int>?.
  • The second step is to type infer []. Today, the compiler front end does this using a context of dynamic, so the list is inferred to be <dynamic>[]. With the change, the list will be inferred using a context of List<int>?, so it will be inferred to be <int>[].
  • As a result, print(y.runtimeType) will print List<int> instead of List<dynamic>.
  • And an attempt to add a string to y will cause a runtime failure.

Mitigation

In the unlikely event that some real-world customer code is affected, the effect will be limited to type inference. So the old behavior can be restored by supplying explicit types. For example, the above example could be changed to:

main() {
  List<int>? x = null;
  dynamic y = x ?? <dynamic>[];
  print(y.runtimeType);
  y.add('str');
}

(Note that [] has been changed to <dynamic>[].)

Footnotes

  1. This coercion doesn't happen when dynamic appears more deeply inside the context; for example, a context of List<dynamic> is not changed to List<_>.

  2. This is a consequence of the fact that the analyzer coerces dynamic to _, therefore rule 1 applies.

@stereotype441 stereotype441 added the breaking-change-request This tracks requests for feedback on breaking changes label Apr 11, 2024
@stereotype441 stereotype441 changed the title [Breaking Change] Make type inference e1 ?? e2 consistent. [Breaking Change] Make type inference of e1 ?? e2 consistent. Apr 11, 2024
@itsjustkevin
Copy link
Contributor

@Hixie @grouma @vsmenon for review as a follow on to #55418.

@Hixie
Copy link
Contributor

Hixie commented Apr 11, 2024

This is just about how the type on the RHS of ?? is inferred, not how the type of the whole expression is inferred, right?

As in:

T? nope<T>() => null;

void main() {
  var x = nope<int>() ?? ''; // x is inferred to be Object rather than int, even after this change
}

Assuming I understand correctly, SGTM.

@vsmenon
Copy link
Member

vsmenon commented Apr 11, 2024

lgtm

@lrhn
Copy link
Member

lrhn commented Apr 11, 2024

(Shouldn't the context type of e2 be NonNull(S) where S is the static type of e1, not just the type itself? It does seem to consistently be the static type itself, but that means that

void main() async {
  List<int>? maybeList = null as dynamic;
  var x = maybeList ?? (await Future.value([]));
  print([x].runtimeType);
}

gives x a static type of List<int>? for no good reason. Probably not something to fix in this "consistency CL", since it's something both implementations agree on today.)

@devoncarew devoncarew added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Apr 11, 2024
@stereotype441
Copy link
Member Author

@Hixie

This is just about how the type on the RHS of ?? is inferred, not how the type of the whole expression is inferred, right?

Correct, it's just about how the RHS of ?? is inferred.

As in:

T? nope<T>() => null;

void main() {
  var x = nope<int>() ?? ''; // x is inferred to be Object rather than int, even after this change
}

Yes. In fact, this code is unaffected by the change for two reasons: (1) because the type of the RHS ('') is always String, regardless of the context in which it's inferred, and (2) since the variable declaration has an implicit type, the context for the whole if-null expression nope<int>() ?? '' is _, not dynamic.

@stereotype441
Copy link
Member Author

@lrhn

(Shouldn't the context type of e2 be NonNull(S) where S is the static type of e1, not just the type itself? It does seem to consistently be the static type itself, but that means that

void main() async {
  List<int>? maybeList = null as dynamic;
  var x = maybeList ?? (await Future.value([]));
  print([x].runtimeType);
}

gives x a static type of List<int>? for no good reason.

Yeah, considering that this is an example of an aspirational context, I agree. It makes sense to do something as close as possible to what we think the user probably wants, and NonNull(S) seems like a better model of what the user probably wants than S.

Probably not something to fix in this "consistency CL", since it's something both implementations agree on today.)

Agreed 😃

@stereotype441
Copy link
Member Author

@grouma ping

@grouma
Copy link
Member

grouma commented Apr 29, 2024

cc @leonsenft who will handle breaking change requests for ACX going forward.

@leonsenft
Copy link

LGTM assuming

A prototype of this change caused zero test failures in Google's internal codebase

holds.

@stereotype441
Copy link
Member Author

LGTM assuming

A prototype of this change caused zero test failures in Google's internal codebase

holds.

I will recheck before landing the change.

@itsjustkevin
Copy link
Contributor

@stereotype441 lock it in!

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
Status: Complete
Development

No branches or pull requests

8 participants