Fix for 11987.

Fix for 11987.

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

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

[jwren, 2013-11-08 18:08:44]

Not to be committed until into next week.

[Brian Wilkerson, 2013-11-08 18:37:14]

LGTM, once comments are addressed

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/InterfaceTypeImpl.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/InterfaceTypeImpl.java:739:
private boolean isMoreSpecificThan(InterfaceType s, HashSet<ClassElement>
visitedClasses,
It would be good to add a TODO somewhere to remind us to investigate whether we
can get rid of visitedClasses.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:52:
if (firstType != null) {
Why are we testing for null here but not in equals? (I don't think the types
should be allowed to be null.)

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:53:
firstHashCode = firstType.getElement() == null ? 0 :
firstType.getElement().hashCode();
Why are we basing the hashCode on the element when we're basing equals on the
type itself? Seems like that could lead to a poor hash distribution.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:55:
int secondHashCode = 0;//= secondType.getElement() == null ? 0 :
secondType.getElement().hashCode();
nit: I don't think we need this comment.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/type/Type.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/type/Type.java:82:
public boolean isAssignableTo(Type type, Set<TypePair> visitedPairs);
Do these methods need to be added to the interface? I'm guessing they are being
invoked at some point where we only know that we have a Type, but I have to
wonder whether we could cast to a TypeImpl and keep internal implementation
details like TypePair out of the public interface. This might be something to
explore later if it looks hard.

[scheglov, 2013-11-10 23:51:59]

lgtm

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:29:
public class TypePair {
static?
May be move to top level, it is internal/implementation package anyway.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:56:
if (secondType != null) {
We check for null here, but don't check in equals().
We could use Guava Objects.hashCode() or JDK's Arrays.hashCode().

For equals() we could use our ObjectUtilities.equals() or again Guava's
Objects.equal().

[jwren, 2013-11-21 18:57:40]

I don't like adding TODOs and landing a CL, but alas.

With the state of my machine and SVN after the OSX upgrade, I'm not even sure
that this will land in a minute., and I don't want to put off getting this fix
committed.

I added some TODOs, and we can revisit some of the comments later, but at least
in the meantime we won't have this infinite loop in the system.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/InterfaceTypeImpl.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/InterfaceTypeImpl.java:739:
private boolean isMoreSpecificThan(InterfaceType s, HashSet<ClassElement>
visitedClasses,
Done. Added a TODO.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:52:
if (firstType != null) {
Agreed.  There was a corner case when Dart code isn't resolvable where this was
actually encountered.  My feeling was to leave it in just for robustness, but we
can always revisit.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:53:
firstHashCode = firstType.getElement() == null ? 0 :
firstType.getElement().hashCode();
Adding a TODO to revisit.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:55:
int secondHashCode = 0;//= secondType.getElement() == null ? 0 :
secondType.getElement().hashCode();
On 2013/11/08 18:37:15, Brian Wilkerson wrote:
> nit: I don't think we need this comment.

Done.

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/type/TypeImpl.java:56:
if (secondType != null) {
Good catch.  Adding a TODO for you comment about ObjectUtilities.equals().

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/type/Type.java
(right):

https://codereview.chromium.org/62523002/diff/120001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/type/Type.java:82:
public boolean isAssignableTo(Type type, Set<TypePair> visitedPairs);
I didn't plan on adding them into the interface at first either, but ended up
doing so only because it wasn't easy not to.  We can revisit later.

[jwren, 2013-11-21 18:59:48]

Committed patchset #7 manually as r30540 (presubmit successful).