Skip to content

Commit

Permalink
GROOVY-5106, GROOVY-10439, GROOVY-11508: re-implement with new type args
Browse files Browse the repository at this point in the history 
3_0_X backport
  • Loading branch information
@eric-milles
eric-milles committed Oct 26, 2024
1 parent 2827e14 commit 09247dd
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 77 deletions.
39 changes: 38 additions & 1 deletion src/main/java/org/codehaus/groovy/classgen/Verifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.declS;
import static org.codehaus.groovy.ast.tools.GeneralUtils.fieldX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.getInterfacesAndSuperInterfaces;
import static org.codehaus.groovy.ast.tools.GeneralUtils.localVarX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.param;
import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
Expand Down Expand Up @@ -272,6 +273,10 @@ public void variableNotAlwaysInitialized(final VariableExpression var) {
};
}

private static Set<ClassNode> getAllInterfaces(final ClassNode cn) {
return getInterfacesAndSuperInterfaces(cn);
}

private static void checkForDuplicateInterfaces(final ClassNode cn) {
ClassNode[] interfaces = cn.getInterfaces();
int nInterfaces = interfaces.length;
Expand All @@ -284,6 +289,38 @@ private static void checkForDuplicateInterfaces(final ClassNode cn) {
throw new RuntimeParserException("Duplicate interfaces in implements list: " + interfaceNames, cn);
}
}

// GROOVY-5106: check for same interface with different type argument(s)
List< Set<ClassNode> > allInterfaces = new ArrayList<>(nInterfaces + 1);
for (ClassNode in : interfaces) allInterfaces.add(getAllInterfaces(in));
allInterfaces.add(getAllInterfaces(cn.getUnresolvedSuperClass()));
if (nInterfaces == 1 && allInterfaces.get(1).isEmpty())
return; // no peer interface(s) to verify

for (int i = 0; i < nInterfaces; i += 1) {
for (ClassNode in : allInterfaces.get(i)) {
if (in.redirect().getGenericsTypes() != null) {
for (int j = i + 1; j < nInterfaces + 1; j += 1) {
Set<ClassNode> set = allInterfaces.get(j);
if (set.contains(in)) {
for (ClassNode t : set) { // find match and check generics
if (t.equals(in)) {
String one = in.toString(false), two = t.toString(false);
if (!one.equals(two)) {
String warning = String.format(
"The %s %s is implemented more than once with different arguments: %s and %s",
(Traits.isTrait(in) ? "trait" : "interface"), in.getNameWithoutPackage(), one, two);
Token token = new Token(0, "", cn.getLineNumber(), cn.getColumnNumber()); // ASTNode to CSTNode
cn.getModule().getContext().getErrorCollector().addWarning(1, warning, token, cn.getModule().getContext());
}
break;
}
}
}
}
}
}
}
}

private static void checkForDuplicateMethods(final ClassNode cn) {
Expand Down Expand Up @@ -622,7 +659,7 @@ private GroovyRuntimeException newVariableError(final String name, final ASTNode

@Override
public void visitMethod(final MethodNode node) {
// GROOVY-3712: if it's an MOP method, it's an error as they aren't supposed to exist before ACG is invoked
// GROOVY-3712: if it's a MOP method, it's an error as they aren't supposed to exist before ACG is invoked
if (MopWriter.isMopMethod(node.getName())) {
throw new RuntimeParserException("Found unexpected MOP methods in the class node for " + classNode.getName() + "(" + node.getName() + ")", classNode);
}
Expand Down
125 changes: 69 additions & 56 deletions src/main/java/org/codehaus/groovy/control/ResolveVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,60 +267,74 @@ protected SourceUnit getSourceUnit() {
return source;
}

@Override
public void visitField(final FieldNode node) {
Map<GenericsTypeName, GenericsType> oldNames = genericParameterNames;
if (!canSeeTypeVars(node.getModifiers(), node.getDeclaringClass())) {
genericParameterNames = Collections.emptyMap();
}

if (!fieldTypesChecked.contains(node)) {
resolveOrFail(node.getType(), node);
}
super.visitField(node);

genericParameterNames = oldNames;
}

@Override
public void visitProperty(final PropertyNode node) {
Map<GenericsTypeName, GenericsType> oldNames = genericParameterNames;
if (!canSeeTypeVars(node.getModifiers(), node.getDeclaringClass())) {
genericParameterNames = Collections.emptyMap();
}

resolveOrFail(node.getType(), node);
fieldTypesChecked.add(node.getField());

super.visitProperty(node);

genericParameterNames = oldNames;
}

private static boolean canSeeTypeVars(final int mods, final ClassNode node) {
return !Modifier.isStatic(mods) || Traits.isTrait(node); // GROOVY-8864, GROOVY-11508
}

@Override
protected void visitConstructorOrMethod(final MethodNode node, final boolean isConstructor) {
VariableScope oldScope = currentScope;
currentScope = node.getVariableScope();
Map<GenericsTypeName, GenericsType> oldNames = genericParameterNames;
genericParameterNames = node.isStatic() && !Traits.isTrait(node.getDeclaringClass())
? new HashMap<>() : new HashMap<>(genericParameterNames);
genericParameterNames =
canSeeTypeVars(node.getModifiers(), node.getDeclaringClass())
? new HashMap<>(genericParameterNames) : new HashMap<>();

resolveGenericsHeader(node.getGenericsTypes());

resolveOrFail(node.getReturnType(), node);
for (Parameter p : node.getParameters()) {
p.setInitialExpression(transform(p.getInitialExpression()));
resolveOrFail(p.getType(), p.getType());
ClassNode t = p.getType();
resolveOrFail(t, t);
visitAnnotations(p);
}
ClassNode[] exceptions = node.getExceptions();
for (ClassNode t : exceptions) {
resolveOrFail(t, node);
if (node.getExceptions() != null) {
for (ClassNode t : node.getExceptions()) {
resolveOrFail(t, t);
}
}
resolveOrFail(node.getReturnType(), node);

MethodNode oldCurrentMethod = currentMethod;
currentMethod = node;

super.visitConstructorOrMethod(node, isConstructor);

currentMethod = oldCurrentMethod;
genericParameterNames = oldNames;
currentScope = oldScope;
}

@Override
public void visitField(final FieldNode node) {
ClassNode t = node.getType();
if (!fieldTypesChecked.contains(node)) {
resolveOrFail(t, node);
}
super.visitField(node);
}

@Override
public void visitProperty(final PropertyNode node) {
Map<GenericsTypeName, GenericsType> oldPNames = genericParameterNames;
if (node.isStatic() && !Traits.isTrait(node.getDeclaringClass())) {
genericParameterNames = new HashMap<>();
}

ClassNode t = node.getType();
resolveOrFail(t, node);
super.visitProperty(node);
fieldTypesChecked.add(node.getField());

genericParameterNames = oldPNames;
}

private void resolveOrFail(final ClassNode type, final ASTNode node) {
resolveOrFail(type, "", node);
}
Expand Down Expand Up @@ -731,7 +745,7 @@ private boolean resolveAliasFromModule(final ClassNode type) {
if (importNode != null && importNode != currImportNode) {
// static alias only for inner classes and must be at end of chain
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), importNode.getFieldName());
if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
if (resolve(tmp, false, false, true) && Modifier.isStatic(tmp.getModifiers())) {
type.setRedirect(tmp.redirect());
return true;
}
Expand All @@ -752,7 +766,7 @@ private boolean resolveAliasFromModule(final ClassNode type) {
// At this point we know that we have a match for pname. This may
// mean, that name[pname.length()..<-1] is a static inner class.
// For this the rest of the name does not need any dots in its name.
// It is either completely a inner static class or it is not.
// It is either completely an inner static class or it is not.
// Since we do not want to have useless lookups we create the name
// completely and use a ConstructedClassWithPackage to prevent lookups against the package.
String className = aliasedNode.getNameWithoutPackage() + "$" + name.substring(pname.length() + 1).replace('.', '$');
Expand Down Expand Up @@ -813,7 +827,7 @@ protected boolean resolveFromModule(final ClassNode type, final boolean testModu
// check package this class is defined in. The usage of ConstructedClassWithPackage here
// means, that the module package will not be involved when the
// compiler tries to find an inner class.
ClassNode tmp = new ConstructedClassWithPackage(module.getPackageName(), name);
ClassNode tmp = new ConstructedClassWithPackage(module.getPackageName(), name);
if (resolve(tmp, false, false, false)) {
ambiguousClass(type, tmp, name);
return true;
Expand All @@ -823,15 +837,15 @@ protected boolean resolveFromModule(final ClassNode type, final boolean testModu
for (ImportNode importNode : module.getStaticImports().values()) {
if (importNode.getFieldName().equals(name)) {
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
if (resolve(tmp, false, false, true) && Modifier.isStatic(tmp.getModifiers())) {
type.setRedirect(tmp.redirect());
return true;
}
}
}
for (ImportNode importNode : module.getStaticStarImports().values()) {
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
if (resolve(tmp, false, false, true) && Modifier.isStatic(tmp.getModifiers())) {
ambiguousClass(type, tmp, name);
return true;
}
Expand All @@ -840,7 +854,7 @@ protected boolean resolveFromModule(final ClassNode type, final boolean testModu
for (ImportNode importNode : module.getStarImports()) {
if (importNode.getType() != null) {
ClassNode tmp = new ConstructedNestedClass(importNode.getType(), name);
if (resolve(tmp, false, false, true) && (tmp.getModifiers() & Opcodes.ACC_STATIC) != 0) {
if (resolve(tmp, false, false, true) && Modifier.isStatic(tmp.getModifiers())) {
ambiguousClass(type, tmp, name);
return true;
}
Expand All @@ -862,7 +876,7 @@ protected boolean resolveToOuter(final ClassNode type) {

// We do not need to check instances of LowerCaseClass
// to be a Class, because unless there was an import for
// for this we do not lookup these cases. This was a decision
// this we do not look up these cases. This was a decision
// made on the mailing list. To ensure we will not visit this
// method again we set a NO_CLASS for this name
if (type instanceof LowerCaseClass) {
Expand Down Expand Up @@ -951,7 +965,7 @@ private static String lookupClassName(final PropertyExpression pe) {
doInitialClassTest = classNameInfo.getV2();
}

if (null == name || name.length() == 0) return null;
if (name == null || name.length() == 0) return null;

return name.toString();
}
Expand Down Expand Up @@ -1089,7 +1103,7 @@ private void checkThisAndSuperAsPropertyAccess(final PropertyExpression expressi
addError("The class '" + type.getName() + "' needs to be an outer class of '" +
currentClass.getName() + "' when using '.this' or '.super'.", expression);
}
if ((currentClass.getModifiers() & Opcodes.ACC_STATIC) == 0) return;
if (!Modifier.isStatic(currentClass.getModifiers())) return;
if (currentScope != null && !currentScope.isInStaticContext()) return;
addError("The usage of 'Class.this' and 'Class.super' within static nested class '" +
currentClass.getName() + "' is not allowed in a static context.", expression);
Expand All @@ -1100,13 +1114,6 @@ protected Expression transformVariableExpression(final VariableExpression ve) {
visitAnnotations(ve);
Variable v = ve.getAccessedVariable();

if(!(v instanceof DynamicVariable) && !checkingVariableTypeInDeclaration) {
/*
* GROOVY-4009: when a normal variable is simply being used, there is no need to try to
* resolve its type. Variable type resolve should proceed only if the variable is being declared.
*/
return ve;
}
if (v instanceof DynamicVariable) {
String name = ve.getName();
ClassNode t = ClassHelper.make(name);
Expand All @@ -1132,8 +1139,14 @@ protected Expression transformVariableExpression(final VariableExpression ve) {
for (VariableScope scope = currentScope; scope != null && !scope.isRoot(); scope = scope.getParent()) {
if (scope.removeReferencedClassVariable(ve.getName()) == null) break;
}
return new ClassExpression(t);
ClassExpression ce = new ClassExpression(t);
ce.setSourcePosition(ve);
return ce;
}
} else if (!checkingVariableTypeInDeclaration) {
// GROOVY-4009: When a normal variable is simply being used, there is no need to try to
// resolve its type. Variable type resolve should proceed only if the variable is being declared.
return ve;
}
resolveOrFail(ve.getType(), ve);
ClassNode origin = ve.getOriginType();
Expand Down Expand Up @@ -1206,14 +1219,14 @@ protected Expression transformBinaryExpression(final BinaryExpression be) {
protected Expression transformClosureExpression(final ClosureExpression ce) {
boolean oldInClosure = inClosure;
inClosure = true;
for (Parameter para : getParametersSafe(ce)) {
ClassNode t = para.getType();
resolveOrFail(t, ce);
visitAnnotations(para);
if (para.hasInitialExpression()) {
para.setInitialExpression(transform(para.getInitialExpression()));
}
visitAnnotations(para);
for (Parameter p : getParametersSafe(ce)) {
ClassNode t = p.getType();
resolveOrFail(t, t);
visitAnnotations(p);
if (p.hasInitialExpression()) {
p.setInitialExpression(transform(p.getInitialExpression()));
}
visitAnnotations(p);
}

Statement code = ce.getCode();
Expand Down
55 changes: 44 additions & 11 deletions src/test/groovy/InterfaceTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,54 @@
package groovy

import gls.CompilableTestSupport
import org.codehaus.groovy.control.CompilationUnit

class InterfaceTest extends CompilableTestSupport {
final class InterfaceTest extends CompilableTestSupport {

void testGenericsInInterfaceMembers() {
// control
shouldCompile """
interface I1 {
public <T> T copy1(T arg)
public <U extends CharSequence> U copy2(U arg)
public <V, W> V copy3(W arg)
public <N extends Number> void foo()
}
"""
shouldCompile '''
interface I {
def <T> T m1(T x)
def <U extends CharSequence> U m2(U x)
def <V, W> V m3(W x)
def <N extends Number> void m4( )
}
'''
// erroneous
shouldNotCompile "interface I2 { public <?> copy1(arg) }"
shouldNotCompile "interface I3 { public <? extends CharSequence> copy1(arg) }"
shouldNotCompile 'interface I { def <?> m(x) }'
shouldNotCompile 'interface I { def <? extends CharSequence> m(x) }'
}

// GROOVY-5106
void testReImplementsInterface1() {
new CompilationUnit(new GroovyClassLoader(this.class.classLoader)).with {
addSource 'X', '''
interface I<T> {}
interface J<T> extends I<T> {}
class X implements I<String>, J<Number> {}
'''
compile()

assert errorCollector.errorCount == 0
assert errorCollector.warningCount == 1
assert errorCollector.warnings[0].message == 'The interface I is implemented more than once with different arguments: I <String> and I <java.lang.Number>'
}
}

// GROOVY-5106
void testReImplementsInterface2() {
new CompilationUnit(new GroovyClassLoader(this.class.classLoader)).with {
addSource 'X', '''
interface I<T> {}
class X implements I<Number> {}
class Y extends X implements I<String> {}
'''
compile()

assert errorCollector.errorCount == 0
assert errorCollector.warningCount == 1
assert errorCollector.warnings[0].message == 'The interface I is implemented more than once with different arguments: I <String> and I <java.lang.Number>'
}
}
}
Loading

0 comments on commit 09247dd

Please sign in to comment.