Better messages for optimized index errors.

Better messages for optimized index errors.

Typed list, Array and String indexer methods detect index errors in their own optimized way but now defer to common code to diagnose the error.  This code is shared with the optimized 'ioore' helper, which makes the error message consistent between optimized and unoptimized code.

The main reason for doing this is to ensure that the error messages have as much information as possible to help debug off-line using captured error messages.




BUG=
R=lrn@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/82215d36f4f6163082d802508331e8b49b711b77

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

[sra1, 2015-06-12 01:56:34]

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

[sra1, 2015-06-12 02:46:45]

Patchset #2 (id:20001) has been deleted

[Lasse Reichstein Nielsen, 2015-06-12 14:24:37]

LGTM (rubberstamping the tests).

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_array.dart (right):

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_array.dart:578: if (newLength is !int)
throw new ArgumentError.value(newLength);
consider passing "newLength" as second argument.

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_array.dart:579: if (newLength < 0) throw
new RangeError.value(newLength);
Consider using new RangeError.range(newLength, 0, null, "newLength").
That will document that the value must be non-negative (as is done by
RangeError.checkNotNegative).

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:1417: //
[RangeError.checkValidIndex] with no optional arguments.
no optional parameters omitted?
no optional parameters provided?

(parameters can be optional, arguments are there or not, so optional arguments
isn't clear).

You do provide the "name" parameter.

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_string.dart (right):

https://codereview.chromium.org/1180713003/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_string.dart:19: if (index >= length) throw
diagnoseIndexError(this, index);
Would it be shorter to || the conditions and only have one call?

[sra1, 2015-06-12 22:29:05]

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/js_lib/js_array.dart (right):

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/js_array.dart:578: if (newLength is !int)
throw new ArgumentError.value(newLength);
On 2015/06/12 14:24:36, Lasse Reichstein Nielsen wrote:
> consider passing "newLength" as second argument.

Done.

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/js_array.dart:579: if (newLength < 0) throw
new RangeError.value(newLength);
On 2015/06/12 14:24:36, Lasse Reichstein Nielsen wrote:
> Consider using new RangeError.range(newLength, 0, null, "newLength").
> That will document that the value must be non-negative (as is done by
> RangeError.checkNotNegative).

I have done this for now.

The error feels wordy and indirect, and does not capture the max limit.

RangeError (newLength): Invalid value: Not greater than or equal to 0: -5

I'm considering removing the check and falling back on JS's error. This
less-informative message would permit more optimizations (e.g. generating JS 
a.length = b when all we know about b is that it is an integer).

RangeError: Invalid array length
    at JSArray.dart.JSArray.set$length (out.js:414:25)
    at dart.main (out.js:1170:27)


FWIW the VM's error is a bit misleading.

    [1].length = -3;

gives:
RangeError (index): Invalid value: Not in range 0..1, inclusive: -3
#0      List.[]= (dart:core-patch/growable_array.dart:138)
#1      List.length= (dart:core-patch/growable_array.dart:126)
#2      test (file:///home/sra/play1/bug6i6b.dart:8:7)

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:1417: //
[RangeError.checkValidIndex] with no optional arguments.
On 2015/06/12 14:24:37, Lasse Reichstein Nielsen wrote:
> no optional parameters omitted?
> no optional parameters provided?
> 
> (parameters can be optional, arguments are there or not, so optional arguments
> isn't clear).
> 
> You do provide the "name" parameter.
> 

Done.

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/js_lib/js_string.dart (right):

https://chromiumcodereview.appspot.com/1180713003/diff/40001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/js_string.dart:19: if (index >= length) throw
diagnoseIndexError(this, index);
On 2015/06/12 14:24:37, Lasse Reichstein Nielsen wrote:
> Would it be shorter to || the conditions and only have one call?

Acknowledged.
I'll investigate that later.

[sra1, 2015-06-12 22:34:10]

Committed patchset #3 (id:60001) manually as
0d4361fb6d4a4c9aa1078268c03039737f541364 (presubmit successful).

[sra1, 2015-06-13 01:22:05]

Committed patchset #5 (id:100001) manually as
f7263216e48e4348a489795a459938ce5c73f27c (presubmit successful).