Update range errors to agree on the numbers.

Update range errors to agree on the numbers.

Also ensure that typed-data errors are consistent with other lists.

Fixes issue #24295
BUG= http://dartbug.com/24295
R=floitsch@google.com, iposva@google.com, sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/d755dd65b94d7fa21233070e90f84b9edc73fb2c

[Lasse Reichstein Nielsen, 2015-09-08 10:07:37]

lrn@google.com changed reviewers:
+ iposva@google.com, sra@google.com

[floitsch, 2015-09-08 10:54:18]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-09-08 10:54:19]

LGTM.

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/native_typed_data.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/native_typed_data.dart:1921: int
_checkValidRange(int start, int end, List list) {
I'm afraid that this will get polymorphic in JS.
accessing list.length on different arrays could then get expensive.

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:400: explanation = ": list is empty";
I don't think we should make the assumption that the receiver is a list. Could
be anything...

[Lasse Reichstein Nielsen, 2015-09-08 11:38:04]

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/native_typed_data.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/native_typed_data.dart:1921: int
_checkValidRange(int start, int end, List list) {
So passing the length as a parameter would be better?

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/core/errors.dart#ne...
sdk/lib/core/errors.dart:400: explanation = ": list is empty";
Good point, reworded.

[floitsch, 2015-09-08 13:53:25]

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/native_typed_data.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/native_typed_data.dart:1921: int
_checkValidRange(int start, int end, List list) {
On 2015/09/08 11:38:03, Lasse Reichstein Nielsen wrote:
> So passing the length as a parameter would be better?

Yes. That's what I would do.

[sra1, 2015-09-08 17:49:20]

For dart2js you currently must make all the logic work in diagnoseIndexError.

If you don't go via diagnoseIndexError, the optimizer will change the program's
behaviour.
Fully optimized indexing is compiled to JS property indexing with some explicit
checks. The compiler generates calls to 'ioore' for failed checks and 'ioore'
calls diagnoseIndexError.

We should probably change ioore to call the indexer.
It is not a trivial change because the call from ioore has a set of receiver
types that cannot be precisely inferred by type inference without changing that
too. We might end up generating too many indexers.

The above issue aside, I would not want this change because the indexers are
larger this way. It is also a speed issue - JS engines will be less likely to
inline the larger indexers.
dart2js will have a hard time inlining this version without a lot of unnecessary
code, e.g. 18 copies of the string "Not a valid list index".
The current scheme is fast, compact and consistent.

I agree that consistency helps users.
There are various axes of consistency:

VM vs dart2js
checked vs production
optimized operation vs indirect call (i.e. no bugs in special compiler tricks)

can we get indexers to be consistent over all eight boxes with no code bloat
except possibly an extra case or two in diagnoseIndexError?

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/native_typed_data.dart (right):

https://codereview.chromium.org/1318943005/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/native_typed_data.dart:1913: void
_checkValidIndex(int index, List list) {
You must be careful using code from true native classes like Float32List and
fake ones like Float32x4List. Not only is 'list' polymorphic, it is polymorphic
between intercepted classes and 'plain Dart' classes so any method calls with
require a two-level dispatch with a getInterceptor call, which is going to harm
performance.

This is why _checkIndex is a *member* on the native-only hierarchy - to avoid
any possibility of a getInterceptor call in the indexer. The difference between
passing something as a regular parameter and as 'this' is that in the 'this'
case the interceptor has already been computed as teh call site or earlier.
(Hopefully the only getInterceptor call has been hoisted out of the loop that
calls the indexer).

In addition to being a member function, the original form of _checkIndex was
written so that the actual checks are inlined into the many copies of
operator[], but nothing else is.

The 'throw diagnoseIndexError(this, index)' achieves several things:

1. Since diagnoseIndexError returns the Error object, the JS call stack has no
extra frames (other than _checkIndex if it is not inlined, e.g. in checked
mode). This is nice since you don't have to skip a lot of frames to get to your
program.

2. dart2js is much more conservative when inlining into throw expressions - it
is going to be slow so it might as well be small. diagnoseIndexError is not
duplicated by inlining, making the program smaller. That is where all the
unminifiable strings are.

3. since diagnoseIndexError is on the failure path, it can tolerate polymorphic
call sites and getInterceptor calls that are not acceptable on the fast path.

The x4 classes were unnecessarily different because some of the code was copied
from the VM implementations. I support making them more regular.

Can you make wider use of the 'throw diagnoseIndexError' pattern?
Would the VM benefit from never having to optimize the cold path?

[Lasse Reichstein Nielsen, 2015-09-09 10:31:40]

PTAL. Output of swarm is now almost identical except for the start/end names on
range errors and the improved error message for indexing the empty list.

[sra1, 2015-09-09 17:16:05]

LGTM!

Thanks!

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:392: String get _errorName => "RangeError";
Was this supposed to be "IndexError"?

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:399: return ": no indices are valid";
I find this a tiny bit confusing.
It would be nice if the error said the list was empty, but that assumes the
indexable is a List.

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
File tests/corelib/list_test.dart (right):

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
tests/corelib/list_test.dart:53: list[list.length];
Could also test that an index outside the 2^32 range gives the same kind of
error as one below that range.

Are there any tests for negative index?

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
tests/corelib/list_test.dart:99: testRangeErrors(const [], "const-list");
Also new List.unmodifiable([])

[Ivan Posva, 2015-09-10 05:15:00]

This still needs some work. For example you need to ensure that all calls to
Exceptions::ThrowRangeError have been updated since the meaning of it has
changed in the previous CL in hidden ways.

Also the previous CL has made kRange as an ExceptionType obsolete, but it was
not removed. This will invariably lead to bit rot and should be removed.

Thanks,
-Ivan

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/array.cc
File runtime/lib/array.cc (right):

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/array.cc#ne...
runtime/lib/array.cc:59: Smi::Handle(Smi::New(istart)),
Please use the start handle which is already around.

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/array.cc#ne...
runtime/lib/array.cc:68: Smi::Handle(Smi::New(icount)),
ditto.

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/growable_ar...
File runtime/lib/growable_array.dart (left):

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/growable_ar...
runtime/lib/growable_array.dart:73: Lists.indicesCheck(this, start, end);
What about the other uses of indicesCheck? Why did these change here, but the
other uses did not? Ideally we can then remove the Lists.indicesCheck entirely
from the dart:internal library.

[Lasse Reichstein Nielsen, 2015-09-10 10:08:54]

Good point Ivan.
I went through the other uses of throwRangeError and fixed where necessary
(there are only 10 in total, so it's manageable).

Removed the kRange type (and reused the name for kRangeRange which was now
unnecessarily long).

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/growable_ar...
File runtime/lib/growable_array.dart (left):

https://codereview.chromium.org/1318943005/diff/60001/runtime/lib/growable_ar...
runtime/lib/growable_array.dart:73: Lists.indicesCheck(this, start, end);
Will do.

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:392: String get _errorName => "RangeError";
No, this was introduced as a helper class for a special group of range errors.
It's not important that it's a separate class unless you actually care about it.

https://codereview.chromium.org/1318943005/diff/60001/sdk/lib/core/errors.dar...
sdk/lib/core/errors.dart:399: return ": no indices are valid";
Exactly my problem (and what I started out saying). It does read rather
awkwardly.

Suggestions would be appreciated!

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
File tests/corelib/list_test.dart (right):

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
tests/corelib/list_test.dart:53: list[list.length];
On 2015/09/09 17:16:05, sra1 wrote:
> Could also test that an index outside the 2^32 range gives the same kind of
> error as one below that range.

Good point.

> Are there any tests for negative index?

Probably somewhere, but I doubt they check the actual error. I'll add some here.

https://codereview.chromium.org/1318943005/diff/60001/tests/corelib/list_test...
tests/corelib/list_test.dart:99: testRangeErrors(const [], "const-list");
Good idea. Done.

[Ivan Posva, 2015-09-11 09:12:51]

LGTM with comments

-Ivan

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc
File runtime/lib/array.cc (right):

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc#n...
runtime/lib/array.cc:59: Smi::Handle(Smi::New(istart)),
start

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc#n...
runtime/lib/array.cc:68: Smi::Handle(Smi::New(icount)),
count

https://codereview.chromium.org/1318943005/diff/100001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (left):

https://codereview.chromium.org/1318943005/diff/100001/runtime/vm/exceptions....
runtime/vm/exceptions.h:54: kRangeRange,
Thanks!

[Lasse Reichstein Nielsen, 2015-09-11 11:05:43]

Committed patchset #6 (id:100001) manually as
d755dd65b94d7fa21233070e90f84b9edc73fb2c (presubmit successful).

[srdjan, 2015-09-25 20:32:17]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2015-09-25 20:32:17]

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc
File runtime/lib/array.cc (right):

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc#n...
runtime/lib/array.cc:70: src.Length() - istart);
Shouldn't this be "src.Length()" only.

[Lasse Reichstein Nielsen, 2015-09-25 23:28:22]

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc
File runtime/lib/array.cc (right):

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc#n...
runtime/lib/array.cc:70: src.Length() - istart);
Probably. Maybe it would be simpler to pass `end` instead of `count` if that is
what the user is providing.

[Lasse Reichstein Nielsen, 2015-09-25 23:29:15]

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc
File runtime/lib/array.cc (right):

https://codereview.chromium.org/1318943005/diff/100001/runtime/lib/array.cc#n...
runtime/lib/array.cc:70: src.Length() - istart);
... but that would mean changing "count" above to "end".
With the name "count", then this is the limit.