Fix VariableDefinitions.endToken for formal parameters.

Fix VariableDefinitions.endToken for formal parameters.

This makes the end token point to that parameter name instead of the next token (comma or right parenthesis).
I could not find a way to test the unparsing of a parameter outside its context, that is outside the parameter list it resides in. All tests unparser(2)_test test parameters in their context.

Token positions for operator - arity checks updated now that parameter positions are precise.

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

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

[ngeoffray, 2013-01-10 14:20:52]

DBC

https://codereview.chromium.org/11783089/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11783089/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
is -> in

[ahe, 2013-01-10 15:00:46]

How about:

testUnparseVariableDeclaration(String type, String variables) {
  VariableDefinitions node = parseStatement('$type$variables');
  Expect.equals(variables, unparse(node.definitions));
}

[Johnni Winther, 2013-01-11 12:51:02]

PTAL

Test added.

[ahe, 2013-01-21 11:14:08]

Don't understand the isMinus parameter. Concerned about loss of test coverage
from marking unparser test as failing.

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
is -> in

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
I don't understand the purpose of this. Could you give some examples?

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1228:
name.getEndToken()));
This should not be computed eagerly.

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/dar...
File tests/compiler/dart2js/dart2js.status (right):

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/dar...
tests/compiler/dart2js/dart2js.status:8: unparser_test: Fail # Issue 7852
This test cannot fail. The amount of code coverage you loose is too great,
instead comment out the failing part.

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/unp...
File tests/compiler/dart2js/unparser_test.dart (right):

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/unp...
tests/compiler/dart2js/unparser_test.dart:320: ["this.foo()", "void this.bar()",
If this doesn't work, comment it out.

[Johnni Winther, 2013-01-22 15:57:30]

PTAL

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
On 2013/01/21 11:14:08, ahe wrote:
> I don't understand the purpose of this. Could you give some examples?

For operator -, always point the whole parameter list, like

     int operator -(a, b) {}
                   ^^^^^^
 
instead of

     int operator -(a, b) {}
                       ^

since the correction might not be to remove 'b' but instead to remove 'a, b'.

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
On 2013/01/21 11:14:08, ahe wrote:
> is -> in

Done.

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1228:
name.getEndToken()));
On 2013/01/21 11:14:08, ahe wrote:
> This should not be computed eagerly.

Changed to be computed dynamically.

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/dar...
File tests/compiler/dart2js/dart2js.status (right):

https://codereview.chromium.org/11783089/diff/5001/tests/compiler/dart2js/dar...
tests/compiler/dart2js/dart2js.status:8: unparser_test: Fail # Issue 7852
On 2013/01/21 11:14:08, ahe wrote:
> This test cannot fail. The amount of code coverage you loose is too great,
> instead comment out the failing part.

Done.

[ahe, 2013-01-23 11:26:35]

LGTM, but I have a suggestion that I think would simplify endToken handling in
VariableDefinitions.

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11783089/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:639: if
(isMinus || // Point to the parameter list is case of operator -.
On 2013/01/22 15:57:30, Johnni Winther wrote:
> On 2013/01/21 11:14:08, ahe wrote:
> > I don't understand the purpose of this. Could you give some examples?
> 
> For operator -, always point the whole parameter list, like
> 
>      int operator -(a, b) {}
>                    ^^^^^^
>  
> instead of
> 
>      int operator -(a, b) {}
>                        ^
> 
> since the correction might not be to remove 'b' but instead to remove 'a, b'.

Thanks for the explanation and adding a comment.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1243: NodeList
variables = makeNodeList(count, null, null, ",");
See my comment in unparser.dart. If you change this to:

makeNodeList(count, null, endToken, ",")

I think you can remove the field VariableDefinitions.endToken.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1279: new
VariableDefinitions(type, modifiers,
It is beginning to feel like a hack to use VariableDefinitions node here. WDYT?

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1501: NodeList
variables = makeNodeList(count, null, null, ",");
Also store the endToken in this list.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1611: NodeList
variables = makeNodeList(count, null, null, ",");
Also store endToken here.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/tree/unparser.dart (right):

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/tree/unparser.dart:367:
node.endToken.value == const SourceString(';')) {
I think you only need to test for "node.endToken != null" now. In other words,
if it has an endToken, it should be printed.

However, don't we already have the same logic for NodeList?

[Johnni Winther, 2013-01-28 12:23:24]

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1243: NodeList
variables = makeNodeList(count, null, null, ",");
On 2013/01/23 11:26:36, ahe wrote:
> See my comment in unparser.dart. If you change this to:
> 
> makeNodeList(count, null, endToken, ",")
> 
> I think you can remove the field VariableDefinitions.endToken.

Done.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1279: new
VariableDefinitions(type, modifiers,
On 2013/01/23 11:26:36, ahe wrote:
> It is beginning to feel like a hack to use VariableDefinitions node here.
WDYT?

It feels better after removing the 'null' for endToken.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1501: NodeList
variables = makeNodeList(count, null, null, ",");
On 2013/01/23 11:26:36, ahe wrote:
> Also store the endToken in this list.

Done.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:1611: NodeList
variables = makeNodeList(count, null, null, ",");
On 2013/01/23 11:26:36, ahe wrote:
> Also store endToken here.

Done.

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/tree/unparser.dart (right):

https://codereview.chromium.org/11783089/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/tree/unparser.dart:367:
node.endToken.value == const SourceString(';')) {
On 2013/01/23 11:26:36, ahe wrote:
> I think you only need to test for "node.endToken != null" now. In other words,
> if it has an endToken, it should be printed.
> 
> However, don't we already have the same logic for NodeList?

We can remove endToken completely since node.definitions handles it.

[Johnni Winther, 2013-02-04 08:22:17]

PTAL

Updated to handle for-in by only including trailing semicolons in statements.

[ahe, 2013-02-04 13:38:26]

LGTM!

Nice. I created a mess initially. I should have had the unparser from the start,
I hope that would have made the problems more obvious.

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:558:
identical(value, token.stringValue);
Since it doesn't fit on one line, I'd prefer using { return ... }.

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:1905:
expectSemicolon(token);
I have a prototype that recovers on missing semicolons, and this version should
just work:

Token semicolon = token;
token = expectSemicolon(token);
listener.endVariablesDeclaration(count, semiColon);
return token;

In particular, it is important that we let expectSemicolon control what the next
token is.

[Johnni Winther, 2013-02-05 09:03:08]

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:558:
identical(value, token.stringValue);
On 2013/02/04 13:38:26, ahe wrote:
> Since it doesn't fit on one line, I'd prefer using { return ... }.

Done.

https://codereview.chromium.org/11783089/diff/28001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:1905:
expectSemicolon(token);
On 2013/02/04 13:38:26, ahe wrote:
> I have a prototype that recovers on missing semicolons, and this version
should
> just work:
> 
> Token semicolon = token;
> token = expectSemicolon(token);
> listener.endVariablesDeclaration(count, semiColon);
> return token;
> 
> In particular, it is important that we let expectSemicolon control what the
next
> token is.

Done.