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

Migration could do a better job of Iterable.firstWhere #42382

Closed
stereotype441 opened this issue Jun 17, 2020 · 1 comment
Closed

Migration could do a better job of Iterable.firstWhere #42382

stereotype441 opened this issue Jun 17, 2020 · 1 comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). NNBD Issues related to NNBD Release nnbd-migration-correctness-example Concrete examples of the migration engine producing an incorrect result on a phase 1 package P2 A bug or feature request we're likely to work on

Comments

@stereotype441
Copy link
Member

Given the following code:

abstract class Foo {
  bool get isLarge;
}
class C {
  final List<Foo> foos;
  C(this.foos);
  Foo get firstLargeFoo => foos.firstWhere(
      (foo) => foo.isLarge, orElse: () => null);
}

Migration produces the result:

abstract class Foo {
  bool get isLarge;
}
class C {
  final List<Foo?> foos;
  C(this.foos);
  Foo? get firstLargeFoo => foos.firstWhere(
      (foo) => foo!.isLarge, orElse: () => null);
}

It's really not obvious why the type of foos gets changed to List<Foo?> (even using the tool's "trace" functionality). The reason is that Iterable<T>.firstWhere's signature is T Function(bool Function(T), {T Function() orElse}), so any value returned by orElse must be suitable for insertion in the list (even though firstWhere won't actually try to insert it in the list). In this example, orElse returns null, so the only way the migration tool can see for that to work is if the list is a List<Foo?>.

It's really unfortunate that the migration tool is changing the type of the list, because that's the sort of thing that can cause nulls to propagate to a large number of other places in the program.

A better migration would be:

abstract class Foo {
  bool get isLarge;
}
class C {
  final List<Foo> foos;
  C(this.foos);
  Foo? get firstLargeFoo => foos.cast<Foo?>().firstWhere(
      (foo) => foo!.isLarge, orElse: () => null);
}

But in order to figure out that this is possible, we have to apply the knowledge that firstWhere won't try to insert the value returned by orElse into the list. (And technically, in order for that to be sound, we'd have to look through the entire program to make sure there isn't an override of firstWhere that does do that). So if we do this at all, it's probably best to do it as a special-case optimization specific to Iterable.firstWhere rather than some general case logic.

(Note that this arose concretely when @srawlins was trying to migrate Mockito)

@stereotype441 stereotype441 added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-nnbd-migration NNBD Issues related to NNBD Release nnbd-migration-correctness-example Concrete examples of the migration engine producing an incorrect result on a phase 1 package labels Jun 17, 2020
@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Sep 8, 2020
@stereotype441 stereotype441 added area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). and removed analyzer-nnbd-migration area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 16, 2020
@stereotype441
Copy link
Member Author

This was addressed in 92587ba.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). NNBD Issues related to NNBD Release nnbd-migration-correctness-example Concrete examples of the migration engine producing an incorrect result on a phase 1 package P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

2 participants