Don't permit annotations inside variable declarations.

Don't permit annotations inside variable declarations.

This addresses the root cause of dartbug.com/23176.

BUG=dartbug.com/23204
R=brianwilkerson@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45165

[Paul Berry, 2015-04-14 17:09:39]

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

[Brian Wilkerson, 2015-04-14 18:02:36]

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/ast.dart:19572: * [VariableDeclaration] does not
extend [Declaration].
I don't understand why we would disallow associating a comment with a variable
declaration.

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/parser.dart (left):

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/parser.dart:7474: CommentAndMetadata
commentAndMetadata = _parseCommentAndMetadata();
For recovery purposes, I would leave this in. If metadata is found we should
produce an error and not copy it into the AST structure.

[Paul Berry, 2015-04-14 18:47:13]

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/ast.dart:19572: * [VariableDeclaration] does not
extend [Declaration].
On 2015/04/14 18:02:36, Brian Wilkerson wrote:
> I don't understand why we would disallow associating a comment with a variable
> declaration.

To clarify, we aren't disallowing comments, we're just not recording them for
dartdoc purposes (I'll update the TODO comment to make that clearer).  And the
thing we usually call a "variable declaration" in casual usage is technically a
"variable declaration list".  A "variable declaration" is the portion of a
variable declaration list that declares a single variable.  So the kind of
comment we would no longer be recording would be a comment like this:

int /** comment */ foo;

Comments that appear prior to a variable declaration list would still be
recorded for dartdoc purposes, e.g. this would still work fine:

/** comment */
int foo;

Does that change your opinion?

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/parser.dart (left):

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/parser.dart:7474: CommentAndMetadata
commentAndMetadata = _parseCommentAndMetadata();
On 2015/04/14 18:02:36, Brian Wilkerson wrote:
> For recovery purposes, I would leave this in. If metadata is found we should
> produce an error and not copy it into the AST structure.

I suspect you are right, but I want to experiment with this to make sure that it
still fixes the root cause of dartbug.com/23176, since this error recovery path
was involved in the bad parse that led to the exception in that bug.

I'll follow up once I've had a chance to try it out.

[Brian Wilkerson, 2015-04-14 19:00:23]

LGTM as long as we're coming back to fix this later.

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/ast.dart (right):

https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/ast.dart:19572: * [VariableDeclaration] does not
extend [Declaration].
> Does that change your opinion?

Not really. I already understood that. I just don't see any reason to not record
the data. It's probably a fairly uncommon pattern to see:

  int /** comment 1 */ x, /** comment 2 */ y;

but still, it doesn't cost enough to support it to warrant not supporting it
IMO.

[Paul Berry, 2015-04-15 13:22:32]

On 2015/04/14 19:00:23, Brian Wilkerson wrote:
> LGTM as long as we're coming back to fix this later.
> 
>
https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
> File pkg/analyzer/lib/src/generated/ast.dart (right):
> 
>
https://codereview.chromium.org/1083093002/diff/20001/pkg/analyzer/lib/src/ge...
> pkg/analyzer/lib/src/generated/ast.dart:19572: * [VariableDeclaration] does
not
> extend [Declaration].
> > Does that change your opinion?
> 
> Not really. I already understood that. I just don't see any reason to not
record
> the data. It's probably a fairly uncommon pattern to see:
> 
>   int /** comment 1 */ x, /** comment 2 */ y;
> 
> but still, it doesn't cost enough to support it to warrant not supporting it
> IMO.

Ok, as discussed in person yesterday, I'm leaving the code as is and updating
the TODO comments to note the change in parser recovery behavior, so that if we
decide to revisit this later we'll be reminded of the issues involved.

[Paul Berry, 2015-04-15 13:28:55]

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