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 the type schema for null-aware spread operations consistent #54828

Closed
stereotype441 opened this issue Feb 5, 2024 · 4 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

stereotype441 commented Feb 5, 2024

The type schema used by the compiler front end to perform type inference on the operand of a null-aware spread operator (...?) in map and set literals will be made nullable, to match the behavior of list literals (and to match the behavior of the analyzer).

Although this is technically a breaking change, it's not expected to have any effect on real-world code.

Background

When the compiler needs to perform type inference on the operand of a spread operator (... or ...?), it uses a type schema that's based on the context of the surrounding literal. For example, consider the following code:

List<List<int>> f(Iterable values) => [...values.map((x) => [])];

Since no element type is specified for the empty list [], the compiler needs to infer one. It does so by taking the return type of f (List<List<int>>), extracting its element type (List<int>), wrapping it in Iterable (Iterable<List<int>>), and then using that as the type schema for performing type inference on values.map((x) => []). This in turn causes [] to be type inferred using the type schema List<int>, so [] is interpreted as <int>[].

If the spread operator had been null-aware (...? instead of ...), then the type schema used to type infer values.map((x) => []) would have been Iterable<List<int>>? instead of Iterable<List<int>>. (In other words, a nullable type schema would have been used, because the null-aware spread operator can handle the possibility of its operand evaluating to null). This doesn't have any effect on values.map((x) => []) (because map always returns a non-null value), but it's possible to construct contrived examples where there's a user-visible effect. For example, here's some highly contrived code that detects the type schema using a generic function:

T detectTypeSchema<T>(T t) {
  print(T);
  return t;
}

main() {
  List<int> x1 = [...detectTypeSchema([])];
  List<int> x2 = [...?detectTypeSchema([])];
}

This prints:

Iterable<int>
Iterable<int>?

Which shows that type inference is using a type schema of Iterable<int>? when inferring the generic type parameter for detectTypeSchema in the declaration of x2.

However, as of the current version of Dart, the same thing does not happen with set or map literals. This code:

main() {
  Set<int> x1 = {...detectTypeSchema({})};
  Set<int> x2 = {...?detectTypeSchema({})};
  Map<int, String> x3 = {...detectTypeSchema({})};
  Map<int, String> x4 = {...?detectTypeSchema({})};
}

prints this result:

Iterable<int>
Iterable<int>
Map<int, String>
Map<int, String>

This is clearly an oversight; the compiler should use a nullable type schema for null-aware spread operators in all kinds of collection literals, not just lists.

Intended change

The type schema used by the compiler front end to perform type inference on the operand of a null-aware spread operator (...?) in map and set literals will be made nullable, to match what currently happens in list literals. This will make the compiler front end behavior consistent with that of the analyzer.

This change will cause the test code above to print the following result:

Iterable<int>
Iterable<int>?
Map<int, String>
Map<int, String>?

Justification

In addition to the advantages of making the compiler front-end more self-consistent, and making it more consistent with the analyzer, the change makes reasonable sense from first principles. With few exceptions, the intention of the type schema is to capture what values would be permissible for a given expression to take on. Since null-aware spread operations accept null, it makes sense for them to use a nullable type schema.

Expected impact

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

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, in the code above, the change exerts its effect through the type that's inferred for the generic function detectTypeSchema. To restore the old functionality, the user simply needs to specify explicit types for the generic function. For example:

main() {
  Set<int> x1 = {...detectTypeSchema<Iterable<int>>({})};
  Set<int> x2 = {...?detectTypeSchema<Iterable<int>>({})};
  Map<int, String> x3 = {...detectTypeSchema<Map<int, String>>({})};
  Map<int, String> x4 = {...?detectTypeSchema<Map<int, String>>({})};
}
@stereotype441 stereotype441 added the breaking-change-request This tracks requests for feedback on breaking changes label Feb 5, 2024
@itsjustkevin
Copy link
Contributor

@vsmenon @Hixie @grouma breaking change request incoming.

@grouma
Copy link
Member

grouma commented Feb 5, 2024

SGTM.

@Hixie
Copy link
Contributor

Hixie commented Feb 6, 2024

Fine by me.

@devoncarew devoncarew added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Feb 6, 2024
@vsmenon
Copy link
Member

vsmenon commented Feb 7, 2024

lgtm

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

6 participants