fix #25739, incorrectly expanding a generic comment multiple times

fix #25739, incorrectly expanding a generic comment multiple times

R=brianwilkerson@google.com, rnystrom@google.com

Committed: https://github.com/dart-lang/sdk/commit/9bc31f7bc4ba1c5bd92111b865934b4b6a187543

[Jennifer Messerly, 2016-02-10 19:13:49]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, rnystrom@google.com

[Jennifer Messerly, 2016-02-10 19:15:07]

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

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3958: (_currentToken as
dynamic).precedingComments = t.next;
not enthused about this... suggestions? :)

[Jennifer Messerly, 2016-02-12 17:36:49]

ping in case y'all missed this CL :)

[Bob Nystrom, 2016-02-12 17:40:47]

I'll defer to Brian on this one.

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

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3958: (_currentToken as
dynamic).precedingComments = t.next;
On 2016/02/10 19:15:07, John Messerly wrote:
> not enthused about this... suggestions? :)

Yeah, this seems gross to me, but I don't know the parser well enough to suggest
an alternative.

When I was poking at this, I considered resynthesizing the token without the
comment, but that seemed like a lot of work too.

[Brian Wilkerson, 2016-02-12 17:56:55]

LGTM, and thanks for the ping (I did miss it).

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

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:3958: (_currentToken as
dynamic).precedingComments = t.next;
I just turned Token into an interface, and replaced it in the type hierarchy
with SimpleToken, which is clearly marked as internal implementation, so that
opens some options.

(1) We could add a setter to SimpleToken, although it would have to throw an
exception if the token cannot support comments.

(2) We could add an internal interface ("TokenWithComment"?), implement it in
all of the existing "*WithComment" classes, and add the setter there, which
would avoid the possibility of throwing an exception.

(3) We could define TokenWithComment as a simple class and mix it in to all of
the existing "*WithComment" classes.

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_test.dart (right):

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:1553: "f/*<E>*/(dynamic/*=E extends
num*/p);",
When we're inserting the tokens from the comment, should we only insert to the
end of a type name, and just ignore the rest (or generate a warning that the
rest are being ignored)? We're in the unique position here of having an explicit
"end of production" marker that tells us how to recover :-).

https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_test.dart:1561:
expect(method.parameters.toString(), '(E extends, extends)',
I don't think this actually tests that the comments are gone, nor does it check
that we only get rid of the first comment when there are multiple:

f/*<E>*/ /* I'd like a better name for this function! */( ...

[Bob Nystrom, 2016-02-12 18:27:26]

On 2016/02/12 17:56:55, Brian Wilkerson wrote:
> LGTM, and thanks for the ping (I did miss it).
> 
>
https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/parser.dart (right):
> 
>
https://codereview.chromium.org/1686173002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/parser.dart:3958: (_currentToken as
> dynamic).precedingComments = t.next;
> I just turned Token into an interface, and replaced it in the type hierarchy
> with SimpleToken, which is clearly marked as internal implementation, so that
> opens some options.
> 
> (1) We could add a setter to SimpleToken, although it would have to throw an
> exception if the token cannot support comments.

As far as I know, we don't need to *add* comments, so we could just add a
removeComment() method. That would safely do nothing in SimpleToken since it has
no comment anyway.

– bob

[Brian Wilkerson, 2016-02-12 18:37:19]

> removeComment()

Sure, removeFirstComment().

[Jennifer Messerly, 2016-02-12 18:45:17]

On 2016/02/12 18:37:19, Brian Wilkerson wrote:
> > removeComment()
> 
> Sure, removeFirstComment().

Let me see if I understand this ... given:

     void f/*foo*//*<E>*//*bar*//*baz*/(p)

As I understand the token stream looks something like:

    "void" --> "f" --> "(" --> "p" --> ")"
 
where "(" has precedingComments: /*foo*/ --> /*<E>*/ --> /*bar*/ --> /*baz*/

Ideally we'd end up with:

    "void" --> "f" --> "<" --> "E" --> ">" --> "(" --> "p" --> ")"

where "<" has precedingComments: /*foo*/
and "(" now has precedingComments: /*bar*/ --> /*baz*/

Does that seem right?

(that's not what the code currently does, it will leave foo,bar,baz attached to
the open-paren)

[Jennifer Messerly, 2016-02-18 00:23:01]

PTAL. Tweaked to support asking the comment to remove itself.

[Bob Nystrom, 2016-02-18 16:58:58]

LGTM!

https://codereview.chromium.org/1686173002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1686173002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/parser.dart:3954: // Remove the tokens from the
comment stream.
Nit: "tokens" -> "token".

The overall effect of the outer loop is to remove them all, but the line here
just removes the one.

[Jennifer Messerly, 2016-02-18 22:13:42]

Description was changed from

==========
fix #25739, incorrectly expanding a generic comment multiple times
==========

to

==========
fix #25739, incorrectly expanding a generic comment multiple times

R=brianwilkerson@google.com, rnystrom@google.com

Committed:
https://github.com/dart-lang/sdk/commit/9bc31f7bc4ba1c5bd92111b865934b4b6a187543
==========

[Jennifer Messerly, 2016-02-18 22:13:43]

Committed patchset #3 (id:40001) manually as
9bc31f7bc4ba1c5bd92111b865934b4b6a187543 (presubmit successful).