Add support for assert statements with messages to the analyzer.

Add support for assert statements with messages to the analyzer.

Support is disabled by default.

[Paul Berry, 2015-09-02 15:10:19]

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

[Paul Berry, 2015-09-02 15:10:19]

Note: since the spec and VM changes to support this feature are still under
review (https://codereview.chromium.org/1324933002 and
https://codereview.chromium.org/1307363005), I've made the following
assumptions:

- Syntactically, assertion messages are allowed to be any expression (contrary
to the proposed spec change, which requires a string literal).

- It is a static warning if the assertion message is not assignable to String
(contrary to the proposed VM implementation, which auto-converts to a string).

I don't intend to commit the CL until these assumptions have been validated.

[Brian Wilkerson, 2015-09-02 15:49:16]

It would be good to have some parser tests.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:686: {this.comma, Expression message}) {
None of the other AST node constructors take optional arguments, and I would
prefer to be consistent. Also, please put them in their lexical position.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:1059: AstNode
visitAssertStatement(AssertStatement node) => new AssertStatement(
This needs to be updated to clone the new children.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:1914: bool
visitAssertStatement(AssertStatement node) {
This needs to be updated to compare the new children.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:9796: AstNode
visitAssertStatement(AssertStatement node) => new AssertStatement(
This needs to be updated to map/clone the new children.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:13036: bool
visitAssertStatement(AssertStatement node) {
This needs to be updated to replace the message.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:18049: Object
visitAssertStatement(AssertStatement node) {
This needs to be updated to print the new children.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:4319: if (enableAssertMessage &&
_matches(TokenType.COMMA)) {
An alternative would be to always parse the message and produce an error if the
message isn't currently enabled. That might help with recovery, but I'm not sure
which is better.

[Paul Berry, 2015-09-02 17:20:04]

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:686: {this.comma, Expression message}) {
On 2015/09/02 15:49:15, Brian Wilkerson wrote:
> None of the other AST node constructors take optional arguments, and I would
> prefer to be consistent. Also, please put them in their lexical position.

If I do so, will this constitute a breaking change to the analyzer API?  Or are
we assuming that clients aren't supposed to call the AST constructors directly?

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:4319: if (enableAssertMessage &&
_matches(TokenType.COMMA)) {
On 2015/09/02 15:49:16, Brian Wilkerson wrote:
> An alternative would be to always parse the message and produce an error if
the
> message isn't currently enabled. That might help with recovery, but I'm not
sure
> which is better.

I like this idea.  I'll rework the CL.

[Brian Wilkerson, 2015-09-02 17:35:26]

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:686: {this.comma, Expression message}) {
I wish I could answer that question definitively, but here are my thoughts. The
AST structure is clearly part of the public API. While I would like to define
all of the node types as interfaces and make the current classes be
implementations of those interfaces and provide a factory (or builder) interface
for clients to use when creating instances, that isn't the current state of the
code base. So, I don't have any reason to think that clients are not using the
constructors (even though I'm fairly sure that very few if any clients are
creating AST structures directly).

If we're being conservative, which seems to be wise, then yes, this is a
breaking change.

That isn't necessarily a bad thing. Almost all of the uses of the constructor in
our code needed to be examined and updated and changing the constructor's
signature would have forced that (and several comments pointed out places that
were missed because the signature wasn't changed). I suspect that the same is
true of client code and that we might actually be doing clients a favor to force
them to examine their code carefully instead of allowing them to assume that
everything is fine.

[Paul Berry, 2015-09-02 17:44:20]

On 2015/09/02 17:35:26, Brian Wilkerson wrote:
>
https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/ast.dart (right):
> 
>
https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/ast.dart:686: {this.comma, Expression message})
{
> I wish I could answer that question definitively, but here are my thoughts.
The
> AST structure is clearly part of the public API. While I would like to define
> all of the node types as interfaces and make the current classes be
> implementations of those interfaces and provide a factory (or builder)
interface
> for clients to use when creating instances, that isn't the current state of
the
> code base. So, I don't have any reason to think that clients are not using the
> constructors (even though I'm fairly sure that very few if any clients are
> creating AST structures directly).
> 
> If we're being conservative, which seems to be wise, then yes, this is a
> breaking change.
> 
> That isn't necessarily a bad thing. Almost all of the uses of the constructor
in
> our code needed to be examined and updated and changing the constructor's
> signature would have forced that (and several comments pointed out places that
> were missed because the signature wasn't changed). I suspect that the same is
> true of client code and that we might actually be doing clients a favor to
force
> them to examine their code carefully instead of allowing them to assume that
> everything is fine.

Ok.  Sadly this means that we're going to have to go through another round of
version bumping (test, dart_style, linter, and dev_compiler will all have to
have to be updated to adapt to the breaking change) before we'll be able to
expose this functionality through analysis_server and analyzer_cli.  I guess I'm
glad I got started on the implementation ASAP :)

[Paul Berry, 2015-09-07 19:44:30]

PTAL

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:686: {this.comma, Expression message}) {
On 2015/09/02 17:35:26, Brian Wilkerson wrote:
> I wish I could answer that question definitively, but here are my thoughts.
The
> AST structure is clearly part of the public API. While I would like to define
> all of the node types as interfaces and make the current classes be
> implementations of those interfaces and provide a factory (or builder)
interface
> for clients to use when creating instances, that isn't the current state of
the
> code base. So, I don't have any reason to think that clients are not using the
> constructors (even though I'm fairly sure that very few if any clients are
> creating AST structures directly).
> 
> If we're being conservative, which seems to be wise, then yes, this is a
> breaking change.
> 
> That isn't necessarily a bad thing. Almost all of the uses of the constructor
in
> our code needed to be examined and updated and changing the constructor's
> signature would have forced that (and several comments pointed out places that
> were missed because the signature wasn't changed). I suspect that the same is
> true of client code and that we might actually be doing clients a favor to
force
> them to examine their code carefully instead of allowing them to assume that
> everything is fine.

Thinking back to our discussions with the Seattle folks last time we made a
breaking change to Analyzer, one strategy that was recommended (and which was
very compelling IMO) was: as a backwards compatible change, make a new API
(possibly with an ugly name) and deprecate the old one.  Then, at a later date
when enough backwards compatible changes have accumulated, make a single
backwards-breaking change that deletes the deprecated API and renames the new
one to the old name.  I've tried doing that here.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:1059: AstNode
visitAssertStatement(AssertStatement node) => new AssertStatement(
On 2015/09/02 15:49:15, Brian Wilkerson wrote:
> This needs to be updated to clone the new children.

Done.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:1914: bool
visitAssertStatement(AssertStatement node) {
On 2015/09/02 15:49:15, Brian Wilkerson wrote:
> This needs to be updated to compare the new children.

Done.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:9796: AstNode
visitAssertStatement(AssertStatement node) => new AssertStatement(
On 2015/09/02 15:49:16, Brian Wilkerson wrote:
> This needs to be updated to map/clone the new children.

Done.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:13036: bool
visitAssertStatement(AssertStatement node) {
On 2015/09/02 15:49:15, Brian Wilkerson wrote:
> This needs to be updated to replace the message.

Done.

https://codereview.chromium.org/1309543011/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/ast.dart:18049: Object
visitAssertStatement(AssertStatement node) {
On 2015/09/02 15:49:15, Brian Wilkerson wrote:
> This needs to be updated to print the new children.

Done.

[Brian Wilkerson, 2015-09-07 21:01:49]

> ... as a backwards compatible change, make a new API
> (possibly with an ugly name) and deprecate the old one.  Then, at a later date
> when enough backwards compatible changes have accumulated, make a single
> backwards-breaking change that deletes the deprecated API and renames the new
> one to the old name.

While I understand the value of their suggestion, I'm still not convinced that
this is a good idea in the case of syntactic changes. All of the missed code
changes that I caught in the last review would have been caught by the compiler
if the constructor had been changed to have additional required parameters. (I
can't help but wonder what additional missed changes I didn't catch that the
compiler could have.)

I think we might be doing our clients a disservice, trading off an explicit
breaking change for an implicit breaking change that's harder to detect. For
example, the formatter needs to be updated, but by not making this a breaking
change there won't even be a hint that something significant changed. While it's
true that most clients could roll forward without compilation errors (even the
breaking change to the constructor would only affect clients that are creating
AST nodes, of which there are probably few), at least they'd have some
indication that there was a breaking change that might affect them.

With reservations, then, I'll give it a LGTM.