Add optional message argument to assert statements in the VM.

Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.

Fixes issue #24215
BUG= http://dartbug.com/24215

[Lasse Reichstein Nielsen, 2015-08-28 18:51:58]

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

[Lasse Reichstein Nielsen, 2015-08-28 18:51:59]

https://codereview.chromium.org/1307363005/diff/1/runtime/lib/errors_patch.dart
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/1307363005/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:47: "'$_failedAssertion' $_messageString";
The format is negotiable.

https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc#newcode50
runtime/vm/parser.cc:50: DEFINE_FLAG(bool, assert_message, false, "Allow message
in assert statements");
Should it be "enable_assert_message" instead?

[Ivan Posva, 2015-08-28 22:37:48]

Thanks for verifying that the proposed DEP is implementable. I would like to
hold off on committing this feature until the standards process is further along
and other implementations have caught up.

-Ivan

[Bob Nystrom, 2015-09-01 15:22:29]

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

[Bob Nystrom, 2015-09-01 15:22:30]

Obvious drive-by comment... tests?

[sra1, 2015-09-02 17:55:51]

On 2015/08/28 22:37:48, Ivan Posva wrote:
> Thanks for verifying that the proposed DEP is implementable. I would like to
> hold off on committing this feature until the standards process is further
along
> and other implementations have caught up.
> 
> -Ivan

Why hold off if it is under a flag?

Committing the change makes it much easier for the community to try the feature
and give feedback on the proposal before it is frozen into the standard.  After
all, in addition to knowing something can be implemented, we need to know if it
*should* be implemented.

[sra1, 2015-09-02 18:00:27]

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

[sra1, 2015-09-02 18:00:27]

https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors....
File sdk/lib/core/errors.dart (right):

https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors....
sdk/lib/core/errors.dart:100: final Object message;
1. Is the intention to also give TypeError a message field?

2. This is a breaking change for anyone who implements AssertionError or
TypeError as they have to now implement get:message.  I'm not bothered by that
but I would like some clarity on what aTypeError.message is expected to return. 
The same as toString()?

[Lasse Reichstein Nielsen, 2015-09-03 07:42:04]

https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors....
File sdk/lib/core/errors.dart (right):

https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors....
sdk/lib/core/errors.dart:100: final Object message;
On 2015/09/02 18:00:27, sra1 wrote:
> 1. Is the intention to also give TypeError a message field?

No, but it's an inadvertent consequence of making AssertionError have a field
and TypeError being defined as subclassing AssertionError.
I.e., I'm aware of it, and didn't find a way to avoid it (short of not making
TypeError an AssertionError).

> 2. This is a breaking change for anyone who implements AssertionError or
> TypeError as they have to now implement get:message.  

For the record, it is *not* a breaking change.
No existing code will stop working.

It will make some libraries be outdated and cause warnings,
and if you start using the "message" field on objects from those libraries, your
code will not work, but that won't happen by itself.

> I'm not bothered by that
> but I would like some clarity on what aTypeError.message is expected to
return. 
> The same as toString()?

Probably null since you don't have a way to specify a message for type
assertions.
Which suggests that making the message public might not be such a great idea
anyway. That, or making TypeError an AssertionError was the mistake.

[sethladd, 2015-09-03 14:58:38]

FWIW here's dart2js' CL https://codereview.chromium.org/1325843003/

[Ivan Posva, 2015-09-04 06:39:31]

LGTM and ready to submit once dart2js and analyzer are ready to commit.

Thanks,
-Ivan

https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc#newcode50
runtime/vm/parser.cc:50: DEFINE_FLAG(bool, assert_message, false, "Allow message
in assert statements");
On 2015/08/28 18:51:59, Lasse Reichstein Nielsen wrote:
> Should it be "enable_assert_message" instead?

Please coordinate across the implementations. Since this is only temporary, I am
fine with either flag name.

https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:9159: if (!I->flags().asserts() &&
!I->flags().type_checks()) {
This line here testing the flag configuration is wrong. I am in process of
fixing it, but we might need a change in the test harness to be able to actually
write a functioning regression test. Thanks for touching code that has not been
touched in a long while.

[Paul Berry, 2015-09-07 22:46:29]

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

[Paul Berry, 2015-09-07 22:46:29]

FWIW here's the analyzer's CL: https://codereview.chromium.org/1309543011/

However we probably shouldn't land any of these changes until the spec changes
have been agreed upon (https://codereview.chromium.org/1324933002), especially
the question of whether assertion messages are required to be strings.

[Paul Berry, 2015-09-07 22:48:25]

paulberry@google.com changed reviewers:
- paulberry@google.com

[sra1, 2015-09-17 03:52:33]

The dart2js implementation is committed:
https://codereview.chromium.org/1342213003/

You might like to modify the dart2js-specific tests to make some tests under
tests/language.
https://codereview.chromium.org/1342213003/diff/160001/tests/compiler/dart2js...

[Lasse Reichstein Nielsen, 2015-11-05 16:52:21]

The newest proposal requires errors in the message expression to be caught. The
rewrite is no longer:
  if (assertCondition(<condition>)) throwAssert(<message>)
but more like
  if (assertCondition(<condition>)) {
     var messageObjec;
     try { 
        messageObject = <message>;
     } catch (_) {}
     throwAssert(messageObject);
  }

That requires much more rewriting in the parser (by someone who knows how to do
a proper try/catch).

[Ivan Posva, 2015-11-05 19:17:10]

On 2015/11/05 16:52:21, Lasse Reichstein Nielsen wrote:
> The newest proposal requires errors in the message expression to be caught.
The
> rewrite is no longer:
>   if (assertCondition(<condition>)) throwAssert(<message>)
> but more like
>   if (assertCondition(<condition>)) {
>      var messageObjec;
>      try { 
>         messageObject = <message>;
>      } catch (_) {}
>      throwAssert(messageObject);
>   }
> 
> That requires much more rewriting in the parser (by someone who knows how to
do
> a proper try/catch).

Please file a bug about this so we don't lose track.

-Ivan

[Lasse Reichstein Nielsen, 2016-11-22 12:59:01]

Reopen issue with updated implementation.

The implementation must have a problem, it crashes on
tests/compiler/dart2js_extras/assert_writh_message_test.dart

PTAL.

[Lasse Reichstein Nielsen, 2016-11-22 12:59:09]

Description was changed from

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.

Fixes issue #24215
BUG= http://dartbug.com/24215
==========

to

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.

Fixes issue #24215
BUG= http://dartbug.com/24215
==========

[Lasse Reichstein Nielsen, 2016-11-23 08:34:23]

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

[Lasse Reichstein Nielsen, 2016-11-23 08:34:24]

@asiva for an actual VM developer.

[Lasse Reichstein Nielsen, 2016-11-23 11:27:42]

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

[Lasse Reichstein Nielsen, 2016-11-23 11:27:45]

@hausner because it's parser related.

[Lasse Reichstein Nielsen, 2016-11-24 09:37:41]

https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_...
File tests/language/assert_message_test.dart (right):

https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_...
tests/language/assert_message_test.dart:77: assert(await true, await 2);
This test fails now. It seems to be putting the `2` into the test condition
insted of the message, which gives a type error since it's not a bool.

[Lasse Reichstein Nielsen, 2016-11-24 15:13:37]

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

https://codereview.chromium.org/1307363005/diff/140001/sdk/lib/core/errors.da...
sdk/lib/core/errors.dart:99: /** Message describing the assertion error. */
Still not absolutely sure this should be public.
It might be better to keep it private for now, we can always make it public
later.

https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_...
File tests/language/assert_message_test.dart (right):

https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_...
tests/language/assert_message_test.dart:77: assert(await true, await 2);
That is definitely the problem. If I await either the condition or the message,
it works correctly. If I await both, the message value ends up in the function
call wrapping the condition.

[siva, 2016-11-28 02:14:43]

Lasse is this CL ready for a review or are you asking us to take over the
implementation?

[Lasse Reichstein Nielsen, 2016-11-28 06:29:15]

I *thought* it was ready, until I added a test that crashed :)
If you can take over, that would be awesome. I don't think my chances of finding
a subtle bug are that good.

[hausner, 2016-12-09 00:59:08]

I created https://codereview.chromium.org/2564623003/
to check in this work.

[siva, 2016-12-21 00:24:05]

Can this CL be closed.

[Lasse Reichstein Nielsen, 2016-12-21 13:11:59]

On 2016/12/21 00:24:05, siva wrote:
> Can this CL be closed.

Yes, you created a new CL, so this one is stale. I'll close it.