Issue 3643. Associate 'no such method' problem with method name, not with whole invocation

Issue 3643. Associate 'no such method' problem with method name, not with whole invocation
http://code.google.com/p/dart/issues/detail?id=3643


R=brianwilkerson@google.com
BUG=
TEST=

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

[Brian Wilkerson, 2012-06-15 00:08:42]

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/ast/DartNode.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/ast/DartNode.java:23: private
Set<ErrorCode> reportedErrors;
Why do we need to keep track of this information?

Assuming we need it, I really don't like adding this field to every node. Can we
make it a Map<Node, Set<ErrorCode>> in Resolver instead?

[scheglov, 2012-06-15 00:27:33]

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/ast/DartNode.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/ast/DartNode.java:23: private
Set<ErrorCode> reportedErrors;
On 2012/06/15 00:08:42, Brian Wilkerson wrote:
> Why do we need to keep track of this information?
> 
> Assuming we need it, I really don't like adding this field to every node. Can
we
> make it a Map<Node, Set<ErrorCode>> in Resolver instead?

We need it to avoid reporting warning in TypeAnalyzer when Resolver already
reported error.
I can rewrite it to use single static field in DartNode class with weak Map like
this.

[Brian Wilkerson, 2012-06-15 01:07:22]

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/ast/DartNode.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/ast/DartNode.java:23: private
Set<ErrorCode> reportedErrors;
> I can rewrite it to use single static field in DartNode
> class with weak Map like this.

I actually like that less. That makes dartc even less re-entrant, which is bad,
and adds a risk of a memory leak if exceptions prevent us from clearing the
cache after a complete compile.

Could we associate this with something that is passed in to both the resolver
and the type analyzer (do we have a compilation context of some kind)?

[scheglov, 2012-06-15 18:01:25]

Have to commit to unfreeze my "master" branch (I've missed branch creation for
this issue).
Will make changes in new CL if needed.

[scheglov, 2012-06-15 18:01:48]

Have to commit to unfreeze my "master" branch (I've missed branch creation for
this issue).
Will make changes in new CL if needed.

[Brian Wilkerson, 2012-06-15 18:18:45]

LGTM

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/ast/DartIdentifier.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/ast/DartIdentifier.java:1: // Copyright
(c) 2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/ast/DartIdentifier.java:17: private
boolean hasResolutionError;
Definitely better than the other solutions, but I still don't like it much. The
one change I would request at this point is that we make the name of the field
very specific so that it's clear that this should not be used for any other
kinds of resolution errors. Perhaps something as ugly as
"resolutionAlreadyReportedThatTheMethodCouldNotBeFound", but something that will
make it clear that this is intended for a very specific case.

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/ast/DartNode.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/ast/DartNode.java:112: 
This file should have just been removed from the CL.

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/javatests/...
File
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java
(right):

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:2535:
public void test_canNotResolveMethod_unqualified() throws Exception {
nit: "canNot" --> "cannot"

[scheglov, 2012-06-15 18:46:09]

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/ast/DartIdentifier.java (right):

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/ast/DartIdentifier.java:17: private
boolean hasResolutionError;
On 2012/06/15 18:18:45, Brian Wilkerson wrote:
> Definitely better than the other solutions, but I still don't like it much.
The
> one change I would request at this point is that we make the name of the field
> very specific so that it's clear that this should not be used for any other
> kinds of resolution errors. Perhaps something as ugly as
> "resolutionAlreadyReportedThatTheMethodCouldNotBeFound", but something that
will
> make it clear that this is intended for a very specific case.

I like this name!

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/javatests/...
File
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java
(right):

https://chromiumcodereview.appspot.com/10545189/diff/6001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:2535:
public void test_canNotResolveMethod_unqualified() throws Exception {
On 2012/06/15 18:18:45, Brian Wilkerson wrote:
> nit: "canNot" --> "cannot"

Done.