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.

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

review: fix: Correctly adapt type parameters inherited from enclosing classes #5228

Merged
merged 7 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 37 additions & 0 deletions doc/type_adaption.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,43 @@ we need to go from any other node in the tree.
#### Finding no hierarchy
If we can not find any hierarchy we currently return the input type unchanged.

#### Adapting generics of enclosing classes
Java allows classes to use generics of their enclosing class, e.g.
```java
public class Outer<A> {
public static class Inner<B> {
public void bar(A a, B b) {}
}
}
I-Al-Istannen marked this conversation as resolved.
Show resolved Hide resolved
```
If you now additionally introduce a dual inheritance relationship
```java
public class OuterSub<A1> extends Outer<A1> {
public static class InnerSub<B1> extends Outer.Inner<B1> {
public void bar(A1 a, B1 b) {}
}
}
```
Adapting the method from `Outer.Inner#bar` to `OuterSub.InnerSub#bar` involves
building the hierarchy from `InnerSub` to `Outer.Inner` and then adapting `A1`
and `B1`.
While this hierarchy can be used to resolve `B` to `B1`, it will not be helpful
for adapting `A` to `A1`: This generic type is passed down through a completely
different inheritance chain.

To solve this, we check whether a type parameter is declared by the class
containing the type reference.
For example, when translating `A1` we notice that even though the usage is in
`InnerSub#bar`, the declaration of the type parameter was in `OuterSub`.
Therefore, we adjust the end of our hierarchy to `Outer` instead of `Inner` and
the start to the innermost enclosing class inheriting from that, which is
`OuterSub` in our example.
Notice that there could be *multiple* matching enclosing classes, but we
have no way to decide which one to use, and just arbitrarily resolve the
ambiguity by picking the first one.

After this adjustment of the start and end types, the rest of the translation
continues normally.

## Adapting a method to a subclass
Closely related to type adaption (but not exactly the same!) is translating
Expand Down
10 changes: 4 additions & 6 deletions src/main/java/spoon/support/adaption/DeclarationNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,10 @@ public Optional<CtTypeReference<?>> resolveTypeParameter(CtTypeParameterReferenc

// We try to find a glue node below us to delegate to. Glue nodes do the mapping so we can just
// pass it on unchanged.
Optional<GlueNode> glueNode = children.stream()
.filter(it -> it.isInducedBy(this.inducedBy))
.findFirst();

if (glueNode.isPresent()) {
return glueNode.get().resolveTypeParameter(reference);
if (!children.isEmpty()) {
// We pick a random child. Well-typed programs will converge to the same solution, no matter
// which path we pick.
return children.iterator().next().resolveTypeParameter(reference);
}

// If we have no glue node, we need to actually resolve the type parameter as we reached the
Expand Down
54 changes: 49 additions & 5 deletions src/main/java/spoon/support/adaption/TypeAdaptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -559,11 +559,17 @@ private Optional<CtExecutable<?>> getDeclaringMethodOrConstructor(CtTypeReferenc
return Optional.of((CtExecutable<?>) parent);
}

@SuppressWarnings("AssignmentToMethodParameter")
private DeclarationNode buildHierarchyFrom(CtTypeReference<?> startReference, CtType<?> startType,
CtTypeReference<?> end) {
CtType<?> endType = findDeclaringType(end);
Map<CtTypeReference<?>, DeclarationNode> declarationNodes = new HashMap<>();

if (needToMoveStartTypeToEnclosingClass(end, endType)) {
I-Al-Istannen marked this conversation as resolved.
Show resolved Hide resolved
startType = moveStartTypeToEnclosingClass(hierarchyStart, endType.getReference());
startReference = startType.getReference();
}

DeclarationNode root = buildDeclarationHierarchyFrom(
startType.getReference(),
endType,
Expand All @@ -583,6 +589,31 @@ private DeclarationNode buildHierarchyFrom(CtTypeReference<?> startReference, Ct
.orElse(null);
}

private boolean needToMoveStartTypeToEnclosingClass(CtTypeReference<?> end, CtType<?> endType) {
if (!(end instanceof CtTypeParameterReference)) {
return false;
}
// Declaring type is not the same as the inner type (i.e. the type parameter was declared on an
// enclosing type)
CtType<?> parentType = end.getParent(CtType.class);
parentType = resolveTypeParameterToDeclarer(parentType);

return !parentType.getQualifiedName().equals(endType.getQualifiedName());
}

private CtType<?> moveStartTypeToEnclosingClass(CtType<?> start, CtTypeReference<?> endRef) {
CtType<?> current = start;
while (current != null) {
if (isSubtype(current, endRef)) {
return current;
}
current = current.getDeclaringType();
}
throw new SpoonException(
"Did not find a suitable enclosing type to start parameter type adaption from"
);
}

/**
* This method attempts to find a suitable end type for building our hierarchy.
* <br>
Expand All @@ -598,20 +629,33 @@ private DeclarationNode buildHierarchyFrom(CtTypeReference<?> startReference, Ct
*/
private CtType<?> findDeclaringType(CtTypeReference<?> reference) {
CtType<?> type = null;
if (reference.isParentInitialized()) {
// Prefer declaration to parent. This will be different if the type parameter is declared on an
// enclosing class.
if (reference instanceof CtTypeParameterReference) {
type = reference.getTypeDeclaration();
}
if (type == null && reference.isParentInitialized()) {
type = reference.getParent(CtType.class);
}
if (type == null) {
type = reference.getTypeDeclaration();
}
if (type instanceof CtTypeParameter) {
CtFormalTypeDeclarer declarer = ((CtTypeParameter) type).getTypeParameterDeclarer();

return resolveTypeParameterToDeclarer(type);
}

private static CtType<?> resolveTypeParameterToDeclarer(CtType<?> parentType) {
if (parentType instanceof CtTypeParameter) {
CtFormalTypeDeclarer declarer = ((CtTypeParameter) parentType).getTypeParameterDeclarer();
if (declarer instanceof CtType) {
return (CtType<?>) declarer;
} else {
return declarer.getDeclaringType();
}
return declarer.getDeclaringType();
}
return type;
// Could not resolve type parameter declarer (no class path mode?).
// Type adaption results will not be accurate, this is just a wild (and probably wrong) guess.
return parentType;
}

private DeclarationNode buildDeclarationHierarchyFrom(
Expand Down
52 changes: 52 additions & 0 deletions src/test/java/spoon/support/TypeAdaptorTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package spoon.support;

import java.util.stream.Collectors;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
Expand All @@ -12,7 +13,9 @@
import spoon.reflect.declaration.CtTypeParameter;
import spoon.reflect.factory.Factory;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.visitor.filter.TypeFilter;
import spoon.support.adaption.TypeAdaptor;
import spoon.test.GitHubIssue;
import spoon.testing.utils.ModelTest;

import java.util.List;
Expand Down Expand Up @@ -440,4 +443,53 @@ public <T extends CharSequence> void overloaded(T t) {}

public <T extends String> void overriden(T t) {}
}

@GitHubIssue(issueNumber = 5226, fixed = true)
void testAdaptingTypeFromEnclosingClass() {
Launcher launcher = new Launcher();
launcher.getEnvironment().setComplianceLevel(11);
launcher.addInputResource("src/test/java/spoon/support/TypeAdaptorTest.java");
CtType<?> type = launcher.getFactory()
.Type()
.get(UseGenericFromEnclosingType.class);
@SuppressWarnings("rawtypes")
List<CtMethod> methods = type.getElements(new TypeFilter<>(CtMethod.class))
.stream()
.filter(it -> it.getSimpleName().equals("someMethod"))
.collect(Collectors.toList());
CtMethod<?> test1Method = methods.stream()
.filter(it -> !it.getDeclaringType().getSimpleName().startsWith("Extends"))
.findAny()
.orElseThrow();
CtMethod<?> test2Method = methods.stream()
.filter(it -> it.getDeclaringType().getSimpleName().startsWith("Extends"))
.findAny()
.orElseThrow();

assertTrue(test2Method.isOverriding(test1Method));
assertFalse(test1Method.isOverriding(test2Method));
}

public static class UseGenericFromEnclosingType {
SirYwell marked this conversation as resolved.
Show resolved Hide resolved

public static class Enclosing<T> {

public class Enclosed<S> {

void someMethod(S s, T t) {
}
}
}

public static class ExtendsEnclosing extends Enclosing<String> {

public class ExtendsEnclosed extends Enclosed<Integer> {

@Override
void someMethod(Integer s, String t) {
throw new UnsupportedOperationException();
}
}
}
}
}