Use InheritanceManager to lookup 'iterator' getter.

Use InheritanceManager to lookup 'iterator' getter.

R=brianwilkerson@google.com, jwren@google.com
BUG=

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

[Brian Wilkerson, 2013-06-26 19:56:14]

LGTM, modulo comments

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java:195:
while (true) {
I think this code would be easier to understand if we simply swap the original
versions of lines 194 and 195.

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:137:
InheritanceManager inheritanceManager) {
Could we get rid of the fourth argument by getting the inheritance manager from
the library, or are they sometimes allowed to be different?

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:859:
PropertyAccessorElement current = ((InterfaceType) iteratorType).lookUpGetter(
Do we need to use inheritanceManager.lookupMember here as well?

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/resolver/ElementResolverTest.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/resolver/ElementResolverTest.java:648:
InheritanceManager inheritanceManager = new InheritanceManager(definingLibrary);
If this line doesn't go away (because of getting rid of the fourth argument
below), then the fourth argument below probably needs to be
"library.getInheritanceManager()" so that the work will be cached.

[scheglov, 2013-06-26 20:22:29]

PTAL

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java:195:
while (true) {
On 2013/06/26 19:56:14, Brian Wilkerson wrote:
> I think this code would be easier to understand if we simply swap the original
> versions of lines 194 and 195.

Done.

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:137:
InheritanceManager inheritanceManager) {
On 2013/06/26 19:56:14, Brian Wilkerson wrote:
> Could we get rid of the fourth argument by getting the inheritance manager
from
> the library, or are they sometimes allowed to be different?

Done.

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:859:
PropertyAccessorElement current = ((InterfaceType) iteratorType).lookUpGetter(
On 2013/06/26 19:56:14, Brian Wilkerson wrote:
> Do we need to use inheritanceManager.lookupMember here as well?

Done.

Actually we probably should make utility method to combine lookup and type
parameters substitution.

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/resolver/ElementResolverTest.java
(right):

https://codereview.chromium.org/17826013/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/resolver/ElementResolverTest.java:648:
InheritanceManager inheritanceManager = new InheritanceManager(definingLibrary);
On 2013/06/26 19:56:14, Brian Wilkerson wrote:
> If this line doesn't go away (because of getting rid of the fourth argument
> below), then the fourth argument below probably needs to be
> "library.getInheritanceManager()" so that the work will be cached.

Done.

[Brian Wilkerson, 2013-06-26 20:43:07]

LGTM, pending Jaime's comments

https://codereview.chromium.org/17826013/diff/10/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java
(right):

https://codereview.chromium.org/17826013/diff/10/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java:210:
if (inheritancePath == null || inheritancePath.isEmpty()) {
I'm not sure why it was returning when there was only one entry on the path, but
it was, so this changes the semantics. Perhaps Jaime can verify whether this is
correct.

[jwren, 2013-06-26 22:01:57]

LGTM

Are there co19/language status files that need to be updated too?

https://codereview.chromium.org/17826013/diff/10/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java
(right):

https://codereview.chromium.org/17826013/diff/10/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/InheritanceManager.java:210:
if (inheritancePath == null || inheritancePath.isEmpty()) {
This looks correct.


When computeInheritancePath is called the definingType is added to the path, I
was verifying for all of my test cases that at least one other type was put onto
the path list.  (For all of my test cases the member was at least one class up
in the hierarchy.)  That said, this looks right, it looks like I missed the
general base-case.

[scheglov, 2013-06-26 22:45:37]

Committed patchset #2 manually as r24495 (presubmit successful).