List.removeLast() throws an inconsistent error

[nex3]

The native List type, at least on the VM, throws a RangeError if removeLast() is called on an empty list. A list created using ListBase or ListMixin, on the other hand, throws a StateError in the same situation. The following reproduces this:

import 'dart:collection';

class CustomList extends ListBase {
  final List _inner;

  int get length => _inner.length;
  set length(int value) => _inner.length = value;

  CustomList() : _inner = [];

  operator [](int index) => _inner[index];
  operator []=(int index, value) => _inner[index] = value;
}

void main() {
  try {
    new CustomList().removeLast();
  } catch (error) {
    print(error);
  }

  try {
    [].removeLast();
  } catch (error) {
    print(error);
  }
}

I'm using 8922198.

[rakudrama]

VM:
Bad state: No element
RangeError (index): Invalid value: Valid value range is empty: -1

dart2js:
Bad state: No element
RangeError (index): Index out of range: index must not be negative: -1

[lrhn]

The best error is "Bad state: No element". The user has not provided an index explicitly, so complaining about an index is misleading. Having a "Bad state: List is empty" might be more informative than using "Bad state: No element", which is also used for other situations where the list isn't empty.

When using a range error, the implementations are apparently passing different parameters to, or using different constructors of, RangeError. To be honest, neither message is great for this case - the former is saying that there is no valid index at all (which best matches that the collection is empty), but the latter claims that a non-negative index could be valid.

[rakudrama]

I support changing the warning but it does require implementation work in
dart2js.

removeLast is specially inlined to use the same 'bounds check'
instruction as indexing, so the check can be eliminated in code like

while (stack.isNotEmpty) {
   process(stack.removeLast());
}

The changes would be to make the 'bounds check' instruction also generate
the StateError via js_runtime helper functions, and properly register those
functions since they are called only from generated code. Currently we
pretend that -1 was used as an index, which matches the VM which relies
on indexing to signal an error i.e. a[a.length-1].

The first step would be to write tests that demonstrate dart2js gives the
same error on code that is highly optimized or unoptimized.

[floitschG]

If it's easy, we should change to StateErrors.
However, since they are Errors, I don't think it has a high priority. Together with the stacktrace it's probably not too hard to figure out what's going on.

[lrhn]

It doesn't appear to be easy in dart2js, so we don't plan to make the error type consistent.