Better redirecting constructor and initializers problems reporting.

Better redirecting constructor and initializers problems reporting.

Preparation for work on http://code.google.com/p/dart/issues/detail?id=351

R=zundel@google.com
BUG=
TEST=

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

[zundel, 2011-11-10 20:21:41]

I'll be OOO tomorrow, added Fabio to the review.

[fabiomfv, 2011-11-10 20:48:33]

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1064:
parseInitializer(initializers);
minor comment, not sure what this refactoring (pulling the parsing logic out of
the loop) really gives us. it would be nice if we could refactor the parsing
logic...

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1121: if
(initializer.getValue() instanceof DartSuperConstructorInvocation) {
you could use isInvocation() instead of instanceof here and just check the type
when selecting the error message.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1136: boolean
firstRedirectDuplicationReported = false;
I suspect we can merge these two blocks in one (and avoid two passes (for
loops). check if there are more than one invocation and select the error message
depending on the type (super or redir).

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1151: if
(redirectInitializer != null && !firstRedirectDuplicationReported) {
it might be possible to merge this one two. say we loop first on params and find
out if they have param initializers. then if the initializers loop find an
'invocation' initializer of redir type, it reports the error. there is another
side effect of separating these errors. it seems that we would only report one
kind of error, but it is  possible to have multiple in the same source. say 
class A {
  int x, y;
  A(this.x) : this.foo(), this.y = 123, this.foo();
  A.foo() {}
}
we may want to report both "multiple redir.." and "redir cannot have
initializers"

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1160:
ParserErrorCode.REDIRECTING_CONSTRUCTOR_CAN_NOT_HAVE_INITIALIZERS);
I am not sure I understand why we have 3 situations we report
REDIRECTING_CONSTRUCTOR_CAN_NOT_HAVE_INITIALIZERS

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (left):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:1342: if
(constructorElement != null) {
I am fine moving to the parser, but the idea to have in the resolver was to
defer the error and not stop parsing. also, note that strictly speaking, single
super() call in init list is not a grammatical rule, and in fact allowed in the
grammar. the spec says it must be a compile time error (that includes semantic
errors), not necessarily a syntax error.

initializers:
      ':' superCallOrFieldInitializer (',' superCallOrFieldInitializer)*
    ;

superCallOrFieldInitializer:
      super arguments
    | super '.' identifier arguments
    | fieldInitializer
    ;

[zundel, 2011-11-10 20:56:35]

lgtm

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java (right):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java:49:
REDIRECTING_CONSTRUCTOR_CAN_NOT_HAVE_INITIALIZERS("Redirecting constructor can
not have initializers"),
> 100 chars

[zundel, 2011-11-10 21:06:27]

I think I gave my LGTM too soon - please work out with Fabio if the parser error
terminating too soon is going to be a deal breaker.

[scheglov, 2011-11-10 21:55:38]

Yes, I will continue work with Fabio tomorrow.

I probably will just switch to reporting all initializers and redirectings as
errors (with separate ErrorCode-s). I've discussed with IDE team and they
suggested, that this would be same as Eclipse underlines BOTH of duplicate
method.

[scheglov, 2011-11-10 22:07:54]

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1064:
parseInitializer(initializers);
On 2011/11/10 20:48:34, fabiomfv wrote:
> minor comment, not sure what this refactoring (pulling the parsing logic out
of
> the loop) really gives us. it would be nice if we could refactor the parsing
> logic...

  It was supposed to allows us do return parseRedirectedConstructorInvocation();
and then continue parsing next initializer. But yes, this is not needed now.
There are recommendations to make functions which do only one task, but probably
it is not right to try to apply them here. :-)

  Fixed.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1121: if
(initializer.getValue() instanceof DartSuperConstructorInvocation) {
On 2011/11/10 20:48:34, fabiomfv wrote:
> you could use isInvocation() instead of instanceof here and just check the
type
> when selecting the error message.

  Hm...
  I've added first check for isInvocation(), but I'm not sure that this will
give us much performance gain.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1136: boolean
firstRedirectDuplicationReported = false;
On 2011/11/10 20:48:34, fabiomfv wrote:
> I suspect we can merge these two blocks in one (and avoid two passes (for
> loops). check if there are more than one invocation and select the error
message
> depending on the type (super or redir).

Done.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1151: if
(redirectInitializer != null && !firstRedirectDuplicationReported) {
On 2011/11/10 20:48:34, fabiomfv wrote:
> it might be possible to merge this one two. say we loop first on params and
find
> out if they have param initializers. then if the initializers loop find an
> 'invocation' initializer of redir type, it reports the error. there is another
> side effect of separating these errors. it seems that we would only report one
> kind of error, but it is  possible to have multiple in the same source. say 
> class A {
>   int x, y;
>   A(this.x) : this.foo(), this.y = 123, this.foo();
>   A.foo() {}
> }
> we may want to report both "multiple redir.." and "redir cannot have
> initializers" 

Done.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java (right):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java:49:
REDIRECTING_CONSTRUCTOR_CAN_NOT_HAVE_INITIALIZERS("Redirecting constructor can
not have initializers"),
On 2011/11/10 20:56:35, zundel wrote:
> > 100 chars

Done.

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (left):

http://codereview.chromium.org/8527005/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:1342: if
(constructorElement != null) {
On 2011/11/10 20:48:34, fabiomfv wrote:
> I am fine moving to the parser, but the idea to have in the resolver was to
> defer the error and not stop parsing. also, note that strictly speaking,
single
> super() call in init list is not a grammatical rule, and in fact allowed in
the
> grammar. the spec says it must be a compile time error (that includes semantic
> errors), not necessarily a syntax error.

  We don't stop parsing.
  We keep all nodes, but mark them with problems.

[scheglov, 2011-11-14 15:51:54]

ping?

[fabiomfv, 2011-11-14 15:57:10]

On 2011/11/14 15:51:54, scheglov wrote:
> ping?

sgtm, though I still think we should follow the grammar as is.