Implement assert() messages

[munificent]

The DEP committee feels Seth's proposal for an optional message to assert() is far enough along to be ready for a real implementation.

the implementation should be gated behind a flag called "assert-message".
The implementations can and should now remove the flag.

This is the main tracking bug. Each implementation's specific bug is below:

• VM (Implement assert() messages behind a flag on VM #24215) - CL https://codereview.chromium.org/2564623003/ (old cl)
• dart2js (Implement assert() messages behind a flag on dart2js #24216) - CL https://codereview.chromium.org/1325843003/
• analyzer (Implement assert() messages behind a flag in analyzer #24217)
• analyzer cli (Exposing option to enable support for asserts w/ messages #25141)
• analysis server (Request to add an option to enable assert + message support in analysis server #24276)
• DDC (Implement assert() messages in dev_compiler #27342)
• dart_style (Implement assert() messages behind a flag in dart_style dart_style#411)

[munificent]

Hey, @jmesserly or @vsmenon, do you want to file a relevant bug for DDC?

[sethladd]

@gbracha can you provide any updates from the DEP's discussion on whether the optional param can be type annotated with dynamic or String ?

Thanks!

[sethladd]

From a chat with @gbracha : The type annotation for the optional parameter will be a String.

[sethladd]

Bumping to 1.14, not a critical 1.13 thing

[sethladd]

Not sure what the status of this is.

[munificent]

As far as I can tell:

• The landed patch to dart2js allows any object and it is converted to a string when the AssertionError exception is printed.
• The not-landed patch to analyzer reports a static warning if the message is not assignable to a string.

I believe the consensus opinion on the DEP committee is that any object should be allowed.

[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

[bwilkerson]

Are we leaving the support behind a flag for 1.50, or are we removing the flag?

[sethladd]

Curious, what was implemented? https://github.com/sethladd/dep_assert_with_optional_message/blob/master/proposal.md or something else?

[bwilkerson]

Something more general. The message is allowed to be any expression (not just a string literal).

[mit-mit]

Marking VM complete based on https://codereview.chromium.org/2564623003/

[munificent]

Not quite there yet: #24215 (comment)

[mit-mit]

Moving to 1.22, we need to get this completed.

[mit-mit]

Removing behind a flag from title also now that we are ready to not have this flagged

[dgrove]

Now that #24215 is closed, is there any reason that this should not be closed as well?

[mit-mit]

@munificent do we have tests in place?

Also, we should spec somewhere what we actually added (which sounds like it was different from the DEP).

[eernstg]

That's the spec of 'General expressions in assert', which just allows you to use an expression rather than a conditionalExpression as the single argument to assert. This issue is about passing two arguments to assert, the second of which would be used as a message. The discussions were about the typing of that second argument, I believe, and the evaluation (insist on string? just call toString()?).

[mit-mit]

Did some basic testing passing this program to a recent build:

main() {
  assert(2 > 1, 'Math is broken');
  var v;
  assert(v != null, 'v is null');
}

• dartanalyzer: OK
• dartfmt: OK
• dart2js: OK
• ddc: OK
• dart: Fails, rejecting the second argument unless passed flag, logged VM: remove flag for new assert with message feature #28136

[munificent]

@munificent do we have tests in place?

Yes, but they are a little scattered:

• The dartjs folks added tests/compiler/dart2js_extras/assert_with_messages.test
• The VM folks added test/language/assert_message.test
• For DDC, I added test/language_strong/assert_with_messages.test
• Dartfmt, as usual has its own tests.
• Analyzer too.

The latter two make sense to be different. I think we should eventually unify the first three. A lesson I took away from this is that the language team should try to manage testing a new feature so that we don't pawn it off on the implementers and get redundant or spotty tests.

[stereotype441]

A lesson I took away from this is that the language team should try to manage testing a new feature so that we don't pawn it off on the implementers and get redundant or spotty tests.

I like the sound of this. It would go a long way toward helping to avoid confusion when different teams have different ideas about to interpret the spec, or how corner cases should be handled.

[munificent]

I like the sound of this. It would go a long way toward helping to avoid confusion when different teams have different ideas about to interpret the spec, or how corner cases should be handled.

Yeah, I totally agree. Leaf and I talked about this yesterday.

At the same time, I don't want the language team to implement all of the tests. Aside from having limited bandwidth, I think it's really useful to get help from people such as yourself who are really skilled at coming up with all of the tricky edge cases that we want to test.

So I'm thinking it should be a collaborative process, but the language team should probably own setting up the framework that the implementers can put tests into.

[stereotype441]

Sounds good to me.

[mit-mit]

Closing this for 1.22 as the last item (#28136) landed!