Implement assert() messages behind a flag on VM

[munificent]

This is the VM-specific issue for #24213. That issue has the details.

[lrhn]

https://codereview.chromium.org/1307363005/

[sethladd]

Thanks @lrhn !

[sethladd]

Now that dart2js has landed (https://codereview.chromium.org/1342213003/) I think we can land the VM patch?

cc @iposva-google

[stereotype441]

I'm concerned because the spec change (https://codereview.chromium.org/1324933002) hasn't landed yet, and there is an open question about the spec that doesn't seem to have been agreed upon, namely the question of whether assertion messages are required to be strings or can be any object. (I've been holding off on landing the analyzer changes waiting for this question to be resolved). IIRC, the draft spec text currently says that they need to be strings, but the draft VM implementation allows them to be any object. I haven't looked at the dart2js code to see what it does.

Can we get this question answered ASAP so that we don't start landing implementations that disagree with each other?

cc @gbracha

[sethladd]

Thanks @stereotype441 we're not in a rush to land this in the VM so yes, let's ensure we're implementing the right thing. I believe Gilad's spec proposal is indeed what we should be implementing.

[rakudrama]

On Thu, Sep 17, 2015 at 9:28 AM, Paul Berry notifications@github.com
wrote:

I'm concerned because the spec change (
https://codereview.chromium.org/1324933002) hasn't landed yet, and there
is an open question about the spec that doesn't seem to have been agreed
upon, namely the question of whether assertion messages are required to be
strings or can be any object. (I've been holding off on landing the
analyzer changes waiting for this question to be resolved). IIRC, the draft
spec text currently says that they need to be strings, but the draft VM
implementation allows them to be any object. I haven't looked at the
dart2js code to see what it does.

​The dart2js implementation currently permits the message to be any value.
I have explained in the spec change why I think this is the correct choice

• we can debate that there.

Can we get this question answered ASAP so that we don't start landing
implementations that disagree with each other?

cc @gbracha https://github.com/gbracha

—
Reply to this email directly or view it on GitHub
#24215 (comment).

[sethladd]

The DEP met today, and decided that the type annotation on the optional param should be String. Does the CL reflect that?

[lrhn]

The DEP met today, and decided that the type annotation on the optional
param should be String. Does the CL reflect that?

As commented on the dart2js bug, there are no type annotations on assert
statements.

There is no added runtime type-check on the second operand.
I guess one could be added by rewriting the second operand to
(messageExpression) as String .

[lrhn]

As commented on the dart2js bug, there are no type annotations on assert
statements.

There is no added runtime type-check on the second operand.
I guess one could be added by rewriting the second operand to
(messageExpression) as String .

Whoops, no. If we are talking type annotation, the rewrite should only be
used in checked mode, in unchecked mode it makes no difference (which also
means that the code needs to run for any object anyway, so why bother?).

[stereotype441]

Whoops, no. If we are talking type annotation, the rewrite should only be
used in checked mode, in unchecked mode it makes no difference (which also
means that the code needs to run for any object anyway, so why bother?).

However, the behavior in checked mode is the only behavior that matters because assertion failures only happen in checked mode. Quoting from the proposed spec text in https://codereview.chromium.org/1324933002:

The definition above implies that if $s$ does not evaluate to a \cd{String}, a dynamic error is
thrown, because the argument to the constructor of \cd{AssertionError} is typed as \cd{String} and we
are running in checked mode.

[lrhn]

Good point, checked mode only.

That also makes it even more clear that checking the operand as by a type assertion is the wrong thing to do. It risks throwing again in the middle of an assertion, dropping the original assertion error. I can't see any case where a user wants that.

If the real goal is just to have a static type check of the operand (as the draft CL actually writes), then the VM implementation should not be affected at all, and in practice all values must be accepted.

[sethladd]

Bumping to 1.14, not a critical 1.13 thing.

[mit-mit]

Clearing out 1.15 milestone as the last full push to dev has happened. If any changes are required before 1.15 is shipped, please file a merge request

[mit-mit]

Bumping to 1.17

[dgrove]

@a-siva any updates on this?

[munificent]

Hey, @mhausner, does that commit mean this is done? Can I close the bug?

[mhausner]

This is done.

[munificent]

🤘

[munificent]

After writing that comment, I noticed the CL was reverted. Did it get re-reverted? "Deverted"?

[mhausner]

I had to revert it because the corner case where both assert parameters are await expressions did not do the right thing. Still working on a fix.

[mit-mit]

Moving to 1.22, we need to get this completed.

[mhausner]

Should be done now.

[munificent]

\o/