Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't call isSuptype on types whose Java types are not subtypes #772

Merged
merged 5 commits into from
Jun 22, 2016
Merged

Can't call isSuptype on types whose Java types are not subtypes #772

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d44b6ba
Can't call isSuptype on types whose Java types are not subtypes
smillst May 19, 2016
feb576e
Fix error
smillst Jun 8, 2016
12cf7be
Fix
smillst Jun 8, 2016
54a1731
Merge branch 'master' into subtype
smillst Jun 8, 2016
8fb2333
Code review feedback
smillst Jun 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 58 additions & 49 deletions framework/src/org/checkerframework/framework/type/DefaultTypeHierarchy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,18 +314,6 @@ protected boolean isSubtypeOfAll(final AnnotatedTypeMirror subtype,
return true;
}

protected boolean isSubtypeOfAny(final AnnotatedTypeMirror subtype,
final Iterable<? extends AnnotatedTypeMirror> supertypes,
final VisitHistory visited ) {
for (final AnnotatedTypeMirror supertype : supertypes) {
if (isSubtype(subtype, supertype, visited)) {
return true;
}
}

return false;
}

protected boolean areAllSubtypes(final Iterable<? extends AnnotatedTypeMirror> subtypes,
final AnnotatedTypeMirror supertype,
final VisitHistory visited) {
Expand All @@ -338,18 +326,6 @@ protected boolean areAllSubtypes(final Iterable<? extends AnnotatedTypeMirror> s
return true;
}

protected boolean areAnySubtypes(final Iterable<? extends AnnotatedTypeMirror> subtypes,
final AnnotatedTypeMirror supertype,
final VisitHistory visited) {
for (final AnnotatedTypeMirror subtype : subtypes) {
if (isSubtype(subtype, supertype, visited)) {
return true;
}
}

return false;
}

protected boolean areEqual(final AnnotatedTypeMirror type1, final AnnotatedTypeMirror type2 ) {
return equalityComparer.areEqual(type1, type2);
}
Expand Down Expand Up @@ -541,7 +517,14 @@ public Boolean visitDeclared_Typevar(AnnotatedDeclaredType subtype, AnnotatedTyp

@Override
public Boolean visitDeclared_Union(AnnotatedDeclaredType subtype, AnnotatedUnionType supertype, VisitHistory visited) {
return visitUnionSupertype(subtype, supertype, visited);
Types types = checker.getTypeUtils();
for (AnnotatedDeclaredType supertypeAltern : supertype.getAlternatives()) {
if (TypesUtils.isErasedSubtype(types, subtype.getUnderlyingType(), supertypeAltern.getUnderlyingType())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more concise to add the check for subtyping of the underlying type to isSubtype?
Is there ever a reason for not including this check? It looks like it is always occurring together. The only exception I saw was in visitIntersection_Intersection which could keep the additional check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a reason for not including this check?

Yes, for example isSubtype of Integer and int are not erased Java subtypes, so the check can't be added to isSubtype.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem much simpler to add that special casing into isSubtype.
An int is never a subtype of Integer, because boxing/unboxing would apply first.
Making many invocations of isSubtype simpler and documenting the boxing special case in isSubtype seems beneficial and clearer to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As documented, the Java types of the arguments to isSubtype are not always in a subtype relationship.

It would seem much simpler to add that special casing into isSubtype.

It would not be correct to do so. For example, isSubtype(@C Cloneable & @A List<@B String, @A List<@B String>) if the isErasedSubtype is moved to isSubtype, then isSubtype called recursively on @C Cloneable and @A List<@B String> would return false which would cause isSubtype(@C Cloneable & @A List<@B String, @A List<@B String>) to return false as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your example. At the moment the code is

for (alternatives) {
  if (isErasedSubtype(A, B) && isSubtype(A, B) {
    return true;
  }
}
return false;

How is it not semantically equivalent to write:

for (alternatives) {
  if (isSubytpe(A, B)) {
    return true;
  }
}
return false;

and strengthen isSubtype:

boolean isSubtype(A, B) {
   if (!isErasedSubtype(A, B) {
     return false;
   }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be good to do this, but it is not as simple as adding isErasedSubtype to isSubtype. Aside from primitives, isErasedSubtype fails for the following combinations to isSubtype (and there are probably more):

int[], Object[]
U extends Object & Iterable<Object>, Iterable<Object>
Throwable&Cloneable&Unions.MyInterface<String>,  Object&Cloneable&Unions.MyInterface<String>

So, if we would like to add this functionality, it should be done in a separate pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if you think that is easier.
Did you have a test case for this pull request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a refactoring, so there are already some test cases. More complex ones require asSuper to work correctly. See tests in all-systems with Union or Intersection in name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I'm not sure why splitting this up into two refactorings makes it easier, but if you prefer, let's do that. Please file an issue for the remaining work.
(The PR title made it sound like something doesn't work without this change.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have more test cases in asSuper that crash on master because of asSuper bugs that also crash on asSuper because of this issue.

&& isSubtype(subtype, supertypeAltern)) {
Copy link
Member

@wmdietl wmdietl Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the few places that uses an && to combine the two subtype tests. Below most of them are separate ifs. Is there a reason for this? (Putting the two tests into isSubtype would fix this.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to code so all the checks use &&.

return true;
}
}
return false;
}

@Override
Expand All @@ -553,24 +536,63 @@ public Boolean visitDeclared_Wildcard(AnnotatedDeclaredType subtype, AnnotatedWi
// Intersection as subtype
@Override
public Boolean visitIntersection_Declared(AnnotatedIntersectionType subtype, AnnotatedDeclaredType supertype, VisitHistory visited) {
return visitIntersectionSubtype(subtype, supertype, visited);
for (AnnotatedDeclaredType subtypeI : subtype.directSuperTypes()) {
Types types = checker.getTypeUtils();
if (TypesUtils.isErasedSubtype(types, subtypeI.getUnderlyingType(), supertype.getUnderlyingType())
&& isSubtype(subtypeI, supertype)) {
return true;
}
}
return false;
}

@Override
public Boolean visitIntersection_Primitive(AnnotatedIntersectionType subtype, AnnotatedPrimitiveType supertype, VisitHistory visited) {
return visitIntersectionSubtype(subtype, supertype, visited);
for (AnnotatedDeclaredType subtypeI : subtype.directSuperTypes()) {
if (TypesUtils.isBoxedPrimitive(subtypeI.getUnderlyingType()) && isSubtype(subtypeI, supertype)) {
return true;
}
}
return false;
}

@Override
public Boolean visitIntersection_Intersection(AnnotatedIntersectionType subtype, AnnotatedIntersectionType supertype, VisitHistory visited) {
return visitIntersectionSubtype(subtype, supertype, visited);
Types types = checker.getTypeUtils();
for (AnnotatedDeclaredType subtypeI : subtype.directSuperTypes()) {
for (AnnotatedDeclaredType supertypeI : supertype.directSuperTypes()) {
if (TypesUtils.isErasedSubtype(types, subtypeI.getUnderlyingType(), supertypeI.getUnderlyingType())
&& !isSubtype(subtypeI, supertypeI)) {
return false;
}
}
}
return true;
}


@Override
public Boolean visitIntersection_Null(AnnotatedIntersectionType subtype, AnnotatedNullType supertype, VisitHistory visited) {
// this can occur through capture conversion/comparing bounds
return visitIntersectionSubtype(subtype, supertype, visited);
for (AnnotatedDeclaredType subtypeI : subtype.directSuperTypes()) {
if (isPrimarySubtype(subtypeI, supertype)) {
return true;
}
}
return false;
}

@Override
public Boolean visitIntersection_Typevar(AnnotatedIntersectionType subtype, AnnotatedTypeVariable supertype, VisitHistory visited) {
// this can occur through capture conversion/comparing bounds
for (AnnotatedDeclaredType subtypeI : subtype.directSuperTypes()) {
Types types = checker.getTypeUtils();
if (TypesUtils.isErasedSubtype(types, subtypeI.getUnderlyingType(), supertype.getUnderlyingType())
&& isSubtype(subtypeI, supertype)) {
return true;
}
}
return false;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -604,7 +626,12 @@ public Boolean visitNull_Null(AnnotatedNullType subtype, AnnotatedNullType super

@Override
public Boolean visitNull_Union(AnnotatedNullType subtype, AnnotatedUnionType supertype, VisitHistory visited) {
return visitUnionSupertype(subtype, supertype, visited);
for (AnnotatedDeclaredType supertypeAltern : supertype.getAlternatives()) {
if (isSubtype(subtype, supertypeAltern)) {
return true;
}
}
return false;
}

@Override
Expand Down Expand Up @@ -719,11 +746,7 @@ public Boolean visitTypevar_Typevar(AnnotatedTypeVariable subtype, AnnotatedType
// @X T xt = ...; T t = ..;
// xt = t;
//
// we do not want to implement visitIntersection_Typevar because that would make it ok
// to call is subtype on an intersection and typevar which shouldn't happen
// instead we perform the subtyping here
return visitIntersectionSubtype((AnnotatedIntersectionType) subtype.getUpperBound(),
supertype.getLowerBound(), visited);
return visit(subtype.getUpperBound(), supertype.getLowerBound(), visited);
}

}
Expand Down Expand Up @@ -798,13 +821,6 @@ protected boolean visitIntersectionSupertype(AnnotatedTypeMirror subtype, Annota
return isSubtypeOfAll(subtype, supertype.directSuperTypes(), visited);
}

/**
* An intersection is a subtype if any of its bounds is a subtype of supertype
*/
protected boolean visitIntersectionSubtype(AnnotatedIntersectionType subtype, AnnotatedTypeMirror supertype, VisitHistory visited) {
return areAnySubtypes(subtype.directSuperTypes(), supertype, visited);
}

/**
* A type variable is a supertype if its lower bound is above subtype.
*/
Expand All @@ -826,13 +842,6 @@ protected boolean visitTypevarSubtype(AnnotatedTypeVariable subtype, AnnotatedTy
return checkAndSubtype(upperBound, supertype, visited);
}

/**
* A union is a supertype if ONE of its alternatives is a supertype of subtype
*/
protected boolean visitUnionSupertype(AnnotatedTypeMirror subtype, AnnotatedUnionType supertype, VisitHistory visited) {
return isSubtypeOfAny(subtype, supertype.getAlternatives(), visited);
}

/**
* A union type is a subtype if ALL of its alternatives are subtypes of supertype
*/
Expand Down