dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

Null safety feedback: How to use firstWhere? #42947

Open xvrh opened 4 years ago

xvrh commented 4 years ago

EDIT - best solutions so far:

import 'package:collection/collection.dart';

void main() {
  var list = ['a', 'b', 'c'];
  var d = list.firstWhereOrNull((e) => e == 'd');
}
extension IterableExtension<T> on Iterable<T> {
  T? firstWhereOrNull(bool Function(T element) test) {
    for (var element in this) {
      if (test(element)) return element;
    }
    return null;
  }
}

void main() {
  var list = ['a', 'b'];
  var d = list.firstWhereOrNull((e) => e == 'd');
}

Original question:

I want to search a list and return null when the element is not found

void main() {
  var list = ['a', 'b', 'c'];

  String? d = list.firstWhere((e) => e == 'd', orElse: () => null);
}

https://nullsafety.dartpad.dev/2d0bc36ec1f3a5ade5550c0944702d73

With null safety, I get this error:

main.dart:4:62: Error: A value of type 'Null' can't be assigned to a variable of type 'String'.
  String? d = list.firstWhere((e) => e == 'd', orElse: () => null);

The orElse parameter of firstWhere is declared not nullable:

E firstWhere(bool test(E element), {E orElse()?})

Why not?

E? firstWhere(bool test(E element), {E? orElse()?})

What is the recommended way to search a collection with null safety enabled?

julemand101 commented 4 years ago

My guess is that the return type of firstWhere are E and it would be confusing if it was changed to E? since the method are described with the following for the case of no element returning true in the test:

If no element satisfies test, the result of invoking the orElse function is returned. If orElse is omitted, it defaults to throwing a StateError.

A workaround (I hope there are better solutions...) you could do is to make an extension on Iterable like this:

void main() {
  var list = ['a', 'b', 'c'];

  String? d = list.firstWhereOrNull((e) => e == 'd');
  print(d); // null
}

extension FirstWhereOrNullExtension<E> on Iterable<E> {
  E? firstWhereOrNull(bool Function(E) test) {
    for (E element in this) {
      if (test(element)) return element;
    }
    return null;
  }
}

Alternative you could change the list to String? or cast it before calling firstWhere but these does not seem like good solutions from my perspective.

mraleph commented 4 years ago

I am going to transfer this to dart-lang/language.

mraleph commented 4 years ago

Duplicate of dart-lang/language#836

mraleph commented 4 years ago

/cc @leafpetersen @eernstg @lrhn

pedromassango commented 4 years ago

What is the recommended way to search a collection with null safety enabled?

My gues is: return E if it exist or throw (NoSuchElementException) if it doesn't exist and if orElse was not provided.

This make sense. If you want to return null then just use the orElse callback. Dart is becoming non-nullable by default so "by default" we should not expect null values.

lrhn commented 4 years ago

One way would be

List<X> list = ...; 
X? firstOrNull = list.cast<X?>.firstWhere((v) => test(v!), orElse: () => null);

In practice, I'd go for an extension method.

xvrh commented 4 years ago

Maybe the extension can be in the SDK itself then?

extension IterableExtension<E> on Iterable<E> {
  E? findFirst(bool Function(E) test) {}

  E? findLast(bool Function(E) test) {}

  E? findSingle(bool Function(E) test) {}
}
pedromassango commented 4 years ago

Maybe the extension can be in the SDK itself then?

extension IterableExtension<E> on Iterable<E> {
  E? findFirst(bool Function(E) test) {}

  E? findLast(bool Function(E) test) {}

  E? findSingle(bool Function(E) test) {}
}

@lrhn can this be a option? Also Dart can take advantage of extension to create built-in helper functions like Kotlin does, it provide a bunche of extension right inside the language.

stereotype441 commented 4 years ago

FWIW, this situation is troublesome for the migraiton tool too. See https://github.com/dart-lang/sdk/issues/42382

stereotype441 commented 4 years ago

Note that a similar problem applies to singleWhere (@yjbanov ran into this today)

munificent commented 4 years ago

I've also run into this when migrating tests. I like the idea of the above extension methods and would be happy to see them in the core library if that's feasible.

lrhn commented 4 years ago

It's not impossible. I was targeting them for package:collection (https://github.com/dart-lang/collection/pull/135), and I'm a little disinclined to add extension methods for something that really should be real methods in the library where the real methods would fit in. (On the other hand, if we ever get interface default methods, I guess you would be able to upgrade extension methods to interface default methods)

mnordine commented 4 years ago

I've used extension methods for the following, following convention with try:

This makes the intent clear when reading code with these methods.

stereotype441 commented 4 years ago

FWIW I'm also running into this when migrating protobufs.

rodion-m commented 3 years ago

Maybe the extension can be in the SDK itself then?

extension IterableExtension<E> on Iterable<E> {
  E? findFirst(bool Function(E) test) {}

  E? findLast(bool Function(E) test) {}

  E? findSingle(bool Function(E) test) {}
}

Why these methods are still not in core? firstWhere in null-safety context at the moment is the pain.

lukepighetti commented 3 years ago

firstWhere in a null safe project is proving to be problematic.

lrhn commented 3 years ago

If you are null safe, then you can use package:collection's new firstWhereOrNull (FAQ).

sebthom commented 3 years ago

@lrhn is there also a workaround for dart-async Stream.firstWhere? https://api.dart.dev/stable/2.12.1/dart-async/Stream/firstWhere.html

lrhn commented 3 years ago

There is no similar method in package:async (yet), so you'll have to make do with:

extension <T> on Stream<T> {
  Future<T?> get firstWhereOrNull async {
    await for (var e in this) {
      return e;
    }
    return null;
  }
}
Levi-Lesches commented 3 years ago

I also have the issue with Streams.

It feels counter-intuitive to me that null-safety is enabled by switching types (T? to T), but with functions like firstWhere, we have to use an entirely different method to get the behavior we want.

While we're on the topic of unintuitive, I'd say that the fact that firstWhere throws is surprising, especially when compared to Map.[] which returns null and List.indexOf which returns -1. The intuition is that null is the N/A sort of value, and it should be used more uniformly, though I understand that there's probably historical context. I also understand that making firstWhere not throw would be a massive breaking change, but that's just my two cents. If there's a way we can change it, I think it should be done.

Using orElse and/or having a try/catch waiting for a StateError both seem like null-checks with extra steps. orElse can be replaced by ?? and a try/catch can be replaced by an if(result == null). In other words, what could be

final int largeNumber = nums.firstWhere((int value) => value > 50) ?? 1000;

is instead:

final int largeNumber =  nums.firstWhere((int value) => value > 50, orElse: () => 1000);

or even worse:

int largeNumber;
try {
  largeNumber = nums.firstWhere((int value) => value > 50);
} on StateError {
  largeNumber = 1000;
}

and that's before null-safety became an issue.

cedvdb commented 3 years ago

Adding a dependency for such basic feature is pushing it

lukepighetti commented 3 years ago

It feels counter-intuitive to me that null-safety is enabled by switching types (T? to T), but with functions like firstWhere, we have to use an entirely different method to get the behavior we want.

I have also run into this several times, where a type could easily be T and the user could make it nullable if they wish, but it's forced to T?. It's like someone powered through the migration without really taking a moment to ponder the alternatives.

0biWanKenobi commented 3 years ago

So, almost a year later, the solution is "write ugly code" or "add a dependency"? 😞

the-thirteenth-fox commented 3 years ago

+1

Scheideba commented 3 years ago

You need to import the collections.dart import and then use firstWhereOrNull(). For some reason the firstWhere((e)..., OrElse) no longer works

On Sat, Jun 5, 2021, 23:28 Jason Fox @.***> wrote:

+1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/42947#issuecomment-855335426, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADI55Z4FXJEZBVDHTC5GDBDTRL2QBANCNFSM4PVJB5JQ .

jadaamer commented 3 years ago

i solved this issue by returning an empty list on "OrElse", then i checked if the id is empty

List a=[id,name];

var isExist = a.firstWhere((element) => element.id == id, orElse: () => a( id: "", name: ""));

    if (isExist.id == "") {
      //do something
    }
Levi-Lesches commented 3 years ago

Right, but this is kind of the issue here.

The problem is "what value can we return to show we didn't find any value"?

Your solution is "some new object which is 'empty' and means 'no value'"

But we already have null! Let us use it!

gitaaron commented 3 years ago

Can't you just cast null to the type you are expecting? In the original example -

String? d = list.firstWhere((e) => e == 'd', orElse: () => null as String);
lrhn commented 3 years ago

Casting null as String will throw in sound null-safe mode, so it's not a long-term strategy.

gitaaron commented 3 years ago

Casting null as String will throw in sound null-safe mode, so it's not a long-term strategy.

Thanks - I will add an extension for 'firstWhereOrNull ' as per the previous suggestion.

Levi-Lesches commented 3 years ago

@lrhn, I'm trying to figure out why things are the way they are: If breaking changes weren't an issue, would it make sense to change the signature of Iterable.firstWhere to return E?, then allow orElse to return E? as well? Or even better, make it return null if orElse is omitted, instead of throwing an error, but I can see how that might be too breaking (since there's no warning that the try/catch would be useless, but changing to nullable E will give errors and warnings everywhere).

lrhn commented 3 years ago

It would be possible to change firstWhere to return E?. It might even be desirable, and then I'd remove the orElse entirely and let you rely on ?? to introduce the default value. Parameters like orElse are generally a liability in null safety.

However, it would be annoying for people who definitely know that there will be a match. They'd have to write something like (myList.firstWhere(somethingCertain) ?? unreachable) to throw (with a helper like Never get unreachable => throw UnsupportedError("This isn't happening!");).

It's probably better to have both E firstWhere(bool Function(E) test) and E? firstWhereOrNull(bool Function(E) test) (or tryFirstWhere as someone suggested, it just has bad discoverability in auto-complete because it doesn't start with firstWhere).

Levi-Lesches commented 3 years ago

This is the code from api.dart.dev on firstWhere:

E firstWhere(bool test(E element), {E orElse()?}) {
  for (E element in this) {
    if (test(element)) return element;
  }
  if (orElse != null) return orElse();
  throw IterableElementError.noElement();
}

If it weren't for that throw at the end, Dart would complain that this actually returns E?. So it boils down to "is throwing an acceptable return type for firstWhere?" IMO, I think it isn't. firstWhere is used so often, that it's not accurate to say that "not finding the element is an error". Plenty of people use it to check, which is why there are issues like this one. null is the perfect compromise, because those who want to handle failure can use ??, and those who just want to throw an error on failure can use !, or ??. It might be more annoying for those who need the error, but ! isn't so bad, and including those who expect a failure is probably worth it.

gitaaron commented 3 years ago

It would be possible to change firstWhere to return E?. It might even be desirable, and then I'd remove the orElse entirely and let you rely on ?? to introduce the default value. Parameters like orElse are generally a liability in null safety.

However, it would be annoying for people who definitely know that there will be a match. They'd have to write something like (myList.firstWhere(somethingCertain) ?? unreachable) to throw (with a helper like Never get unreachable => throw UnsupportedError("This isn't happening!");).

It's probably better to have both E firstWhere(bool Function(E) test) and E? firstWhereOrNull(bool Function(E) test) (or tryFirstWhere as someone suggested, it just has bad discoverability in auto-complete because it doesn't start with firstWhere).

My train of thought was I would prefer to treat the default as nullable and include a helper method for those that want to throw if an element is not found. ie/ firstWhereGuaranteed . However, that seems like more typing that just firstWhere()! which I think is equally as understandable and goes more with conventions. So I would agree with @Levi-Lesches' previous comment.

lrhn commented 3 years ago

The one issue with just making the return type nullable is that you can no longer distinguish a null from finding no value satisfying where, and where null is a value which satisfies the where.

If you have firstWhere which throws on not finding a match, it differs from list.firstWhere(test)! which returns null on non-match or a match of null.

Not sure how often you'll be doing firstWhere(test) where test accepts null, but our experience with Map.operator[] suggests that people do store null values in collections.

gitaaron commented 3 years ago

A case where 'null' could be returned as an element of the list seems like more of a reason to make the return of 'firstWhere' nullable.

Currently if I run the following -

  List l = [1,2,null];
  int a = l.firstWhere((el) => el==null);
  int b = a + 4;
  print(b);

I believe I get a runtime exception. But if I were to change it to -

  List l = [1,2,null];
  int a? = l.firstWhere((el) => el==null);
  int b = a + 4;
  print(b);

I believe I get a compile time exception which I would imagine is more preferable and in the spirit of null safety.

gitaaron commented 3 years ago

I am having a hard time understanding why I would want to do something differently if the List contained a null vs. not finding anything. I guess if I wanted to check if null is in the list?

Edit: The two scenarios I can some up with are -

  1. I want to throw an exception if null is in the list.
  2. I want to see if null is in the list.

In either case I would use a forEach.

Second Edit : In the first case I could also do .firstWhere()! if I wanted so I guess that it is not a use case for differentiating unless I wanted to be explicit about the error I am throwing.

Levi-Lesches commented 3 years ago

A case where 'null' could be returned as an element of the list seems like more of a reason to make the return of 'firstWhere' nullable.

This is another important use-case. Using firstWhere to directly compare to null is not what I had in mind (use contains for that), but comparing the elements to another nullable variable can be useful.

The two scenarios I can some up with are -

  1. I want to throw an exception if null is in the list.
  2. I want to see if null is in the list.
if (myList.contains(null))  // throw or do something else

Not sure how often you'll be doing firstWhere(test) where test accepts null, but our experience with Map.operator[] suggests that people do store null values in collections.

I think that Map.[] strengthens the argument here, since Map already returns null ambiguously (if the value is null or if the key is not present), and people can manage the difference. That's because they have alternatives -- Map.cotainsKey is usually used if the map holds null values. So we'd need an equivalent for iterable. Maybe add a bool argument to firstWhere: shouldThrow to force an error if no value is found?

E? firstWhere(bool test(E element), {bool shouldThrow = false}) {
  for (E element in this) {
    if (test(element)) return element;
  }
  if (shouldThrow)
    throw IterableElementError.noElement();
}

So now you have this table of return types:

Value of shouldThrow Element is found Element is not found
true E throws
false E E?

So if your elements can be null (E == E?), you set throwError to true to have a clear signal if the element wasn't found. If you don't care, like how people use Map.[] today, you can simply let it return null. I think that's the perfect compromise between being able to handle "not found" gracefully while also allowing a stricter fallback. @lrhn, thoughts?

sebthom commented 3 years ago

In Java/Spring you would declare one method getFirstWhere(...) that throws an Exception in case no element was found and one method findFirstWhere(...) which would return null in case no element was found. Depending on your use case/expectation of the existence of an element you would choose one or the other.

The disadvantage of having a shouldThrow parameter is that you always have E? as return type.

Levi-Lesches commented 3 years ago

The disadvantage of having a shouldThrow parameter is that you always have E? as return type.

I'm assuming you have an iterable of non-nullable elements, because otherwise you'd want to have an E? returned. So now you know that findWhere will never return null when it finds the element, so you can just use ! instead of shouldThrow. shouldThrow is just to resolve ambiguity between "found a nullable result" and "didn't find anything".

Here's what you should do when:

List type You expect a result You're just checking
List<int> int element = firstWhere(condition)! int? element = firstWhere(condition)
List<int?> int? element = firstWhere(condition, shouldThrow: true) int? element = firstWhere(condition)
sebthom commented 3 years ago

I personally don't like this approach as you either have to use an unsafe cast or you have verbosity (shouldThrow:true). Anyways I don't think it matters much as I doubt the return type of firstWhere will ever be changed at this point since this would break existing code for very little benefit. I think adding a second method like firstWhereOrNull or however it is called is probably more sensible if not alone for the fact that it is backwards compatible.

Levi-Lesches commented 3 years ago

True this would be a breaking change, but we shouldn't sacrifice long-term API clarity just for that. Imagine if this had been done during the null-safety implementation: we would have made this change in our migrations and be done with it. Now that we're past that, this would probably have to wait for the next major version, but it should still be done. Besides, by making the return type nullable the compiler can catch cases that need to be fixed (and maybe even fix them with dart fix), instead of having to go find them manually. Updating code isn't the worst thing in the world, we just did it for null safety; this should have been part of that IMO.

As to your first points, both are better than what we currently have today:

To demonstrate (assuming we never had firstWhereOrNull):


List<int> nonNullable;
List<int?> nullable;
bool Function(int) condition;

// Throw if the element is not in a non-nullable list
int element = nonNullable.firstWhere(condition)!;  // new
int element = nonNullable.firstWhere(condition);  // current

// Check for an element in a non-nullable list (very common)
int? element = nonNullable.firstWhere(condition);  // new
int? element;  // current
try {
  element = nonNullable.findWhere(condition);
} catch { }

// Throw if an element is not in a nullable list
int? element = nullable.firstWhere(condition, shouldThrow: true);  // new
int? element = nullable.firstWhere(condition);  // current

// Checking for an element in a nullable list
int? element = nullable.firstWhere(condition);  // new
int? element = nullable.firstWhere(condition, orElse: () => null);  // current 
sebthom commented 3 years ago

@Levi-Lesches We can also compare all three approaches. Introducing a shouldThrow parameter would result in mandatory code changes in all four cases. Adding a firstWhereOrNull() is backwards compatible, i.e. needs no changes but allows you to gradually improve your code in three cases. I also like to point you to https://softwareengineering.stackexchange.com/questions/362900/parameter-to-control-whether-to-throw-an-exception-or-return-null-good-practic

List<int> nonNullable;
List<int?> nullable;
bool Function(int) condition;

// Throw if the element is not in a non-nullable list
int element = nonNullable.firstWhere(condition)!;  // needed change with shouldThrow semantic
int element = nonNullable.firstWhere(condition);   // NO change with firstWhereOrNull()
int element = nonNullable.firstWhere(condition);   // current

// Check for an element in a non-nullable list (very common)
int? element = nonNullable.firstWhere(condition);       // needed change with shouldThrow semantic
int? element = nonNullable.firstWhereOrNull(condition); // optionally improved with firstWhereOrNull()
int? element;  // current
try {
  element = nonNullable.findWhere(condition);
} catch { }

// Throw if an element is not in a nullable list
int? element = nullable.firstWhere(condition, shouldThrow: true);  // needed change shouldThrow semantic
int? element = nullable.firstWhere(condition); // NO change with firstWhereOrNull()
int? element = nullable.firstWhere(condition); // current

// Checking for an element in a nullable list
int? element = nullable.firstWhere(condition);        // needed change with shouldThrow semantic
int? element = nullable.firstWhereOrNull(condition);  // optionally improved with firstWhereOrNull()
int? element = nullable.firstWhere(condition, orElse: () => null);  // current 

Just to be clear, I also see firstWhereOrNull() as a workaround and think that it was a language design error to make the nullability of firstWhere's return type depending on the nullability of the list. firstWhere's return type should be nullable and never throw an exception.

Levi-Lesches commented 3 years ago

I'll note that where you added needed change, the code always improved:

  1. Using ! makes it clear there will be a runtime error if the assumption is incorrect. That is not obvious currently
  2. The second example is much shorter than the current firstWhere behavior
  3. Using shouldThrow makes it clear there will be a runtime error if the assumption is incorrect. That is not obvious currently.
  4. The last example is much shorter than the current firstWhere behavior

That's not even mentioning the potential to use ?? and other null operators, or the safer-by-default behavior by not throwing.

I didn't include firstWhereOrNull because I agree that it's a nice temporary solution that doesn't break much. I'm advocating to eventually change firstWhere so that it is clearer and more useful in the long-term.

lukepighetti commented 3 years ago

Just to throw a wrench in the works we could have an operator that converted errors into null values.

[].first ?????????????????????? == null 😅

Levi-Lesches commented 3 years ago

Actually, some have suggested the !! operator for just that. It's like ?? but instead of triggering on a null value, it triggers on an error. I can't find the issue for it, but I'm more in favor of https://github.com/dart-lang/language/issues/361)

lrhn commented 3 years ago

An inline "catch expression" is an interesting idea. I have thought about it a few times, but the grammar needed to express the error type and capture variables gets cumbersome. Just ignoring all of that and reacting to any throw can definitely be shorter. I fear it might not be what people actually want, and it might hide programming errors. Say int.parse(str.substring(start, end)) !! 0 is intended to catch FormatException, but if you somehow manage to have incorrect start/end indices, it will also happily hide the RangeError you get from substring. The style guide recommends against catches without on-types for a reason.

(Also, the operator should definitely be ?!? and be pronounced "WAT".)

Wdestroier commented 3 years ago

import 'package:collection/src/iterable_extensions.dart'; shows a warning in VScode, pls find a fix soon.

inline catch is probably not the best solution for this case, but it's a great feature.

Creating an object to pass to singleWhere gets more annoying as the object complexity grows.

final entry = map.entries.singleWhere((entry) => false, orElse: () => MapEntry(ComplexObject1({
  complexObject2: ComplexObject2({ 
    id: ObjectId('');
   }),
  complexObject3: ComplexObject3({ ... }),
}), ComplexObject1({
  complexObject2: ComplexObject2({ 
    id: ObjectId('');
   }),
  complexObject3: ComplexObject3({ ... }),
})));

if (entry.key.complexObject2.id.asString() != '') {
  // Yay, it's not `null`
}
esDotDev commented 2 years ago

It would be nice to get firstWhereOrNull soon, it's really getting annoying having to write code like:

BasicPushNotification? pinned = notifications
        .cast<BasicPushNotification?>()
        .firstWhere((n) => n!.isPinned == true, orElse: () => null);

Yuck.

lukepighetti commented 2 years ago

Actually, some have suggested the !! operator for just that. It's like ?? but instead of triggering on a null value, it triggers on an error. I can't find the issue for it, but I'm more in favor of dart-lang/language#361)

!(isValid()!! ?? false) coming soon to an IDE near you