proposal: `unsafe_iterable_method`

scheglov commented 1 year ago

Description

Using first, last, single, [] methods on Iterable can lead to an exception.

Details

This happened in the linter itself, see e22fdaee0eecfe3b14a650d271d07ce332a4b984, and in the analysis server, see https://github.com/dart-lang/sdk/issues/52226. Also saw one crash because of using unprotected [] in google3 just today.

Kind

This guards against errors.

Bad Examples

void f(List<int> values) {
  values.first;
  values.last;
  values.single;
  values[0];
}

Good Examples

import 'package:collection/collection.dart';
void f(List<int> values) {
  values.firstOrNull;
  values.lastOrNull;
  values.singleOrNull;
  values.elementAtOrNull(0);
}

Discussion

This lint is not for everybody, some people might be just fine relying on external knowledge "I know that at this point the list is not empty". But I view it the same as types or null safety - we have to prove that the value has the required type, or is not null.

Discussion checklist

[ ] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
[x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
[ ] If there's any prior art (e.g., in other linters), please add references here.
[ ] If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
[x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

pq commented 1 year ago

💯💯💯

I'm emphatically in support of this.

Would love to get input from language/library folks too though.

/cc @lrhn @eernstg

eernstg commented 1 year ago

I think it makes sense to indicate that certain operations may give rise a dynamic error, especially in the case where there is an alternative where the error situation is made explicit (here: by yielding null rather than throwing), and other mechanisms in the language will ensure that an error is handled (here: standard null safety mechanisms).

However, it also seems likely to me that there would be way too many false positives. Don't developers use ad-hoc reasoning all the time to establish that a certain collection isn't empty? For instance, in a Dart compiler, the list of types in the implements clause could be known to be non-empty because it's a syntax error to have an implements clause with no types in it. So if the list itself isn't null then it is a non-empty list. Surely every single application domain would have a large number of examples like that: "That list isn't empty because I just know those lists are never empty!!!". ;-)

Moreover, firstOrNull returning null could mean that the iterable is empty, but it could also mean that the first element in that iterable is null.

We may use myIterable is Iterable<Object> to confirm that it's the former (but if the element type is nullable then we just don't know, and we'd need to call isEmpty). If we do call isEmpty anyway then we might very well want to use first rather than firstOrNull, because first will not throw based on the iterable being empty, and the return type of first is the iterable element type E rather than E?.

Maybe only lint the occurrences of first etc. in the case where the iterable element type is non-nullable?

scheglov commented 1 year ago

You are right that there are cases when we implicitly know that there is always at least one element in the collection. Indeed, there are examples of this in the analyzer itself.

It would be nice to express this as a type, e.g. NotEmptyList. And make this list impossible to construct, unless you provide at least one element. Maybe with two variations, depending on whether you know for sure the first, or the last element.

abstract class NotEmptyList<T> {
  T get first;
  T get last;
}

class HeadList<T> implements NotEmptyList<T> {
  @override
  final T first;
  final List<T> tail;

  HeadList(this.first, this.tail);

  @override
  T get last => tail.lastOrNull ?? first;
}

class TailList<T> implements NotEmptyList<T> {
  final List<T> head;

  @override
  final T last;

  TailList(this.head, this.last);

  @override
  T get first => head.firstOrNull ?? last;
}

Although at this point this turns such lint from "not for everybody" into "for almost nobody".

dart-lang / linter