Make List constructor give better error messages for non-int arguments

Make List constructor give better error messages for non-int arguments

BUG= http://dartbug.com/15986
R=herhut@google.com, iposva@google.com, sra@google.com

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

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

[Lasse Reichstein Nielsen, 2015-07-07 13:08:01]

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

[Lasse Reichstein Nielsen, 2015-07-07 13:08:02]

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:85: Expect.throws(fn, checked ? null :
test,
null should be ((e)=>e is TypeError), but dartj2s seems to bypass the type check
and call the constructor directly in some cases.

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:99: (e) => e is ArgumentError, "list2");
I wanted to check for RangeError here, but it seems some parts of dart2js
bypasses the js_array code and throws the JS exception directly (which is
converted to ArgumentError).

[Ivan Posva, 2015-07-08 08:42:32]

VM changes: LGTM -ip

[herhut, 2015-07-08 08:59:58]

lgtm. 

Could you file a bug on the wrong behavior of dart2js wrt. calling the
constructor directly?

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:85: Expect.throws(fn, checked ? null :
test,
On 2015/07/07 13:08:02, Lasse Reichstein Nielsen wrote:
> null should be ((e)=>e is TypeError), but dartj2s seems to bypass the type
check
> and call the constructor directly in some cases.

Yes, this happens early on in handleNewSend in the SSABuilder. That code is
wrong and should be fixed.

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:99: (e) => e is ArgumentError, "list2");
On 2015/07/07 13:08:02, Lasse Reichstein Nielsen wrote:
> I wanted to check for RangeError here, but it seems some parts of dart2js
> bypasses the js_array code and throws the JS exception directly (which is
> converted to ArgumentError).

See above.

[Lasse Reichstein Nielsen, 2015-07-08 09:03:39]

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:85: Expect.throws(fn, checked ? null :
test,
I'm not sure it's necessarily wrong, it's throwing in all the right places, just
not as informative an error as possible. That's still acceptable.

I won't force dart2js to throw the more precise error if it makes the code
shorter to not do so (and if it would make the code shorter to go through the
constructor, it will probably happen anyway).

[herhut, 2015-07-08 09:05:48]

On 2015/07/08 09:03:39, Lasse Reichstein Nielsen wrote:
>
https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
> File tests/corelib/growable_list_test.dart (right):
> 
>
https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
> tests/corelib/growable_list_test.dart:85: Expect.throws(fn, checked ? null :
> test,
> I'm not sure it's necessarily wrong, it's throwing in all the right places,
just
> not as informative an error as possible. That's still acceptable.
> 
> I won't force dart2js to throw the more precise error if it makes the code
> shorter to not do so (and if it would make the code shorter to go through the
> constructor, it will probably happen anyway).

Well, it might not throw for too large values. At least that should be fixed.

[sra1, 2015-07-08 09:57:00]

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

[sra1, 2015-07-08 09:57:01]

lgtm

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

https://codereview.chromium.org/1214723009/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/js_array.dart:29: // the value is not a UInt32.
Give a better error message.
I believe this change causes optimized code to behave differently to
unoptimized. If so, don't do that.

If we decide we want this better error message (js does say the argument is not
a valid array length) it must be made in conjunction with fixing the optimizer.
It is too hard currently to remove the range check. This prevents us from
replacing the constructor in some cases.
We probably don't want to do this now.

https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_array.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_array.dart:218: checkGrowable('addAll');
Keep this line first. It is effectively a receiver type check that might have
been accomplished by having multiple implementation types.

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:97: testThrowsOrTypeError(() => new
List([]), (e) => e is ArgumentError, 'list');
Have two versions of each of these:

    new List(x)
    new List(confuse(x))

Can you split the test so that constructor argument errors should be in a
different test file to concurrent modification errors?

[Lasse Reichstein Nielsen, 2015-07-08 10:52:31]

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

https://codereview.chromium.org/1214723009/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/js_array.dart:29: // the value is not a UInt32.
Give a better error message.

> I believe this change causes optimized code to behave differently to
> unoptimized. If so, don't do that.

That's already the case, depending on whether you consider the message of an
ArgumentError to be significant.
If optimized code goes directly to the JS Array constructor, it will give an
error message like "RangeError: Invalid array length" as an ArgumentError, but
the existing code gave "Lenght must be a non-negative integer: $length".
This changes the error to be a different ArgumentError again (because RangeError
is an ArgumentError) but I don't see it as much more different.


> 
> If we decide we want this better error message (js does say the argument is
not
> a valid array length) it must be made in conjunction with fixing the
optimizer.
> It is too hard currently to remove the range check. This prevents us from
> replacing the constructor in some cases.
> We probably don't want to do this now.

https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_array.dart (right):

https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_array.dart:218: checkGrowable('addAll');
On 2015/07/08 09:57:01, sra1 wrote:
> Keep this line first. It is effectively a receiver type check that might have
> been accomplished by having multiple implementation types.

Acknowledged.

[Ivan Posva, 2015-08-14 05:53:37]

On 2015/07/08 10:52:31, Lasse Reichstein Nielsen wrote:
>
https://codereview.chromium.org/1214723009/diff/1/sdk/lib/_internal/js_runtim...
> File sdk/lib/_internal/js_runtime/lib/js_array.dart (right):
> 
>
https://codereview.chromium.org/1214723009/diff/1/sdk/lib/_internal/js_runtim...
> sdk/lib/_internal/js_runtime/lib/js_array.dart:29: // the value is not a
UInt32.
> Give a better error message.
> 
> > I believe this change causes optimized code to behave differently to
> > unoptimized. If so, don't do that.
> 
> That's already the case, depending on whether you consider the message of an
> ArgumentError to be significant.
> If optimized code goes directly to the JS Array constructor, it will give an
> error message like "RangeError: Invalid array length" as an ArgumentError, but
> the existing code gave "Lenght must be a non-negative integer: $length".
> This changes the error to be a different ArgumentError again (because
RangeError
> is an ArgumentError) but I don't see it as much more different.
> 
> 
> > 
> > If we decide we want this better error message (js does say the argument is
> not
> > a valid array length) it must be made in conjunction with fixing the
> optimizer.
> > It is too hard currently to remove the range check. This prevents us from
> > replacing the constructor in some cases.
> > We probably don't want to do this now.
> 
>
https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
> File sdk/lib/_internal/js_runtime/lib/js_array.dart (right):
> 
>
https://codereview.chromium.org/1214723009/diff/20001/sdk/lib/_internal/js_ru...
> sdk/lib/_internal/js_runtime/lib/js_array.dart:218: checkGrowable('addAll');
> On 2015/07/08 09:57:01, sra1 wrote:
> > Keep this line first. It is effectively a receiver type check that might
have
> > been accomplished by having multiple implementation types.
> 
> Acknowledged.

Is this still an active review?

Thanks,
-Ivan

[Lasse Reichstein Nielsen, 2015-08-14 11:44:23]

Committed patchset #3 (id:40001) manually as
33c76638268e3e24fb7a73a8f6f141f38272e8dd (presubmit successful).

[srdjan, 2015-08-14 18:00:54]

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

[srdjan, 2015-08-14 18:00:55]

DBC

https://codereview.chromium.org/1214723009/diff/40001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/1214723009/diff/40001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:249: assert(this.length == i || (throw new
ConcurrentModificationError(this)));
What is the rule with COncurrentModificationErrors: should they be thrown only
in checked mode or always?

https://codereview.chromium.org/1214723009/diff/40001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/40001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:13: assert((checked = true));
This will fail in dartanalyzer. See code for better computation of checked mode.

[Lasse Reichstein Nielsen, 2015-08-14 22:11:02]

https://codereview.chromium.org/1214723009/diff/40001/sdk/lib/collection/list...
File sdk/lib/collection/list.dart (right):

https://codereview.chromium.org/1214723009/diff/40001/sdk/lib/collection/list...
sdk/lib/collection/list.dart:249: assert(this.length == i || (throw new
ConcurrentModificationError(this)));
There is no rule as such.
We have made them checked-mode-only in some cases (mainly on lists) because it
allows dart2js to generate better code.

[Lasse Reichstein Nielsen, 2015-08-17 08:28:25]

https://codereview.chromium.org/1214723009/diff/40001/tests/corelib/growable_...
File tests/corelib/growable_list_test.dart (right):

https://codereview.chromium.org/1214723009/diff/40001/tests/corelib/growable_...
tests/corelib/growable_list_test.dart:13: assert((checked = true));
On 2015/08/14 18:00:55, srdjan wrote:
> This will fail in dartanalyzer. See code for better computation of checked
mode.

See which code?

And why will it fail?

[Lasse Reichstein Nielsen, 2015-08-17 11:01:17]

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