Implement messages in assert() in dev_compiler.

Implement messages in assert() in dev_compiler.

Fix #27342.

R=jmesserly@google.com

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

[Bob Nystrom, 2016-12-12 22:23:35]

Description was changed from

==========
Implement messages in assert() in dev_compiler.
==========

to

==========
Implement messages in assert() in dev_compiler.

Fix #27342.
==========

[Bob Nystrom, 2016-12-12 22:24:22]

rnystrom@google.com changed reviewers:
+ jmesserly@google.com, vsm@google.com

[Bob Nystrom, 2016-12-12 22:24:23]

The main missing piece is passing a function as the first argument:

assert(() => false, "Blah");

But that's not supported for asserts without messages in DDC, so I didn't bother
to cram it into this CL.

[Jennifer Messerly, 2016-12-12 22:36:02]

Nice!

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3825: return
_callHelperStatement('assertMessage(#, () => #);',
i'd prefer this to be an optional parameter to dart.assert, rather than a second
runtime function.

(my thought process for generating readable JS is: what would a JS user and/or
library author do?)

so this would be 'assert(#, () => #)'

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:39:
throwAssertionError() => JS('', '''(() => {
throwAssertionError([message]) => JS('', '''(() => {
  debugger;
  let error = $message != null
      ? new $AssertionErrorWithMessage($message())
      : new $AssertionError();
  $throw_(error);

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:44:
throwAssertionErrorWithMessage(message) => JS(
this can be removed per earlier suggestion

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:553: if
(!$condition) $throwAssertionError();
here's how I would change this:

assert_(condition, [message]) => JS( // ...

if (!$condition) $throwAssertionError($message);

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:557:
assertMessage_(condition, message) => JS(
this can be removed per earlier suggestions

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/js_helper.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:858: class
AssertionErrorWithMessage extends AssertionError {
do we need a special subtype for this, or was that how dart2js chose to
implement it?

the reason I ask ... it seems a bit overkill to have an entire class just
because of one optional parameter. it is easy to handle this in the toString
method. It's not like AssertionError.toString needs to be optimized with special
subclasses to allow JIT inlining or whatever ... if there's ever a type that
could afford an extra null check, this would be it :)

[Bob Nystrom, 2016-12-13 19:29:51]

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:3825: return
_callHelperStatement('assertMessage(#, () => #);',
On 2016/12/12 22:36:01, Jennifer Messerly wrote:
> i'd prefer this to be an optional parameter to dart.assert, rather than a
second
> runtime function.
> 
> (my thought process for generating readable JS is: what would a JS user and/or
> library author do?)
> 
> so this would be 'assert(#, () => #)'

Done. I wanted to distinguish between an explicit null message (which should
print "Assertion failed: null", not "Assertion failed" and no message. That
meant I couldn't do "!= null" later to decide which kind of error to show.

But I realize now that since the message is always thunked, I can check for null
because an explicit null message will show up as a thunk that returns null, not
a bare null.

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:39:
throwAssertionError() => JS('', '''(() => {
On 2016/12/12 22:36:02, Jennifer Messerly wrote:
> throwAssertionError([message]) => JS('', '''(() => {
>   debugger;
>   let error = $message != null
>       ? new $AssertionErrorWithMessage($message())
>       : new $AssertionError();
>   $throw_(error);	

Done.

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:44:
throwAssertionErrorWithMessage(message) => JS(
On 2016/12/12 22:36:02, Jennifer Messerly wrote:
> this can be removed per earlier suggestion

Done.

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:553: if
(!$condition) $throwAssertionError();
On 2016/12/12 22:36:02, Jennifer Messerly wrote:
> here's how I would change this:
> 
> assert_(condition, [message]) => JS( // ...
> 
> if (!$condition) $throwAssertionError($message);

Done.

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:557:
assertMessage_(condition, message) => JS(
On 2016/12/12 22:36:02, Jennifer Messerly wrote:
> this can be removed per earlier suggestions

Done.

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
File pkg/dev_compiler/tool/input_sdk/private/js_helper.dart (right):

https://codereview.chromium.org/2568313002/diff/1/pkg/dev_compiler/tool/input...
pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:858: class
AssertionErrorWithMessage extends AssertionError {
On 2016/12/12 22:36:02, Jennifer Messerly wrote:
> do we need a special subtype for this, or was that how dart2js chose to
> implement it?

This is how dart2js does it, but that's not why I do it here.

> the reason I ask ... it seems a bit overkill to have an entire class just
> because of one optional parameter. it is easy to handle this in the toString
> method. It's not like AssertionError.toString needs to be optimized with
special
> subclasses to allow JIT inlining or whatever ... if there's ever a type that
> could afford an extra null check, this would be it :)

If a user passes an explicit null message, I want that to print "Assertion
failed: null". That means I can't rely on checking the message for null to
decide how to display it. I could add a separate bool "_hasMessage" field or
something, but this felt a little cleaner to me.

I can't do the thunk checking here, because the thunk gets forced before we get
here. If evaluating the message parameter throws an exception, I want that to
propagate instead of the AssertionError.

[Jennifer Messerly, 2016-12-13 20:10:27]

LGTM!

[Bob Nystrom, 2016-12-13 21:32:01]

Description was changed from

==========
Implement messages in assert() in dev_compiler.

Fix #27342.
==========

to

==========
Implement messages in assert() in dev_compiler.

Fix #27342.

R=jmesserly@google.com

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

[Bob Nystrom, 2016-12-13 21:32:02]

Committed patchset #2 (id:20001) manually as
b900a3962062144c2ee7fd051f6c934918e9323b (presubmit successful).