This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY-8965 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 206ce5b8554362ffd4c963b4f8e228f53f105959 Author: Eric Milles <[email protected]> AuthorDate: Thu Jul 7 15:39:41 2022 -0500 GROOVY-8965, GROOVY-10180, GROOVY-10668: STC: multiple `instanceof` --- .../transform/stc/StaticTypeCheckingSupport.java | 19 +++--- .../transform/stc/StaticTypeCheckingVisitor.java | 68 +++++++++++----------- src/test/groovy/transform/stc/BugsSTCTest.groovy | 43 ++++++-------- .../transform/stc/TypeInferenceSTCTest.groovy | 2 +- .../groovy/parser/antlr4/util/AstDumper.groovy | 20 +++---- .../asm/sc/TypeInferenceStaticCompileTest.groovy | 30 +--------- .../asm/sc/bugs/ReproducibleBytecodeBugs.groovy | 65 ++++++++++----------- 7 files changed, 105 insertions(+), 142 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java index f68572e192..18093b95f5 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java @@ -903,8 +903,7 @@ public abstract class StaticTypeCheckingSupport { } if (result) return true; } else if (superOrInterface instanceof UnionTypeClassNode) { - UnionTypeClassNode union = (UnionTypeClassNode) superOrInterface; - for (ClassNode delegate : union.getDelegates()) { + for (ClassNode delegate : ((UnionTypeClassNode) superOrInterface).getDelegates()) { if (implementsInterfaceOrIsSubclassOf(type, delegate)) return true; } } @@ -1040,8 +1039,9 @@ public abstract class StaticTypeCheckingSupport { int bestDist = Integer.MAX_VALUE; List<MethodNode> bestChoices = new LinkedList<>(); + boolean duckType = receiver instanceof UnionTypeClassNode; // GROOVY-8965: type disjunction boolean noCulling = methods.size() <= 1 || "<init>".equals(methods.iterator().next().getName()); - Iterable<MethodNode> candidates = noCulling ? methods : removeCovariantsAndInterfaceEquivalents(methods); + Iterable<MethodNode> candidates = noCulling ? methods : removeCovariantsAndInterfaceEquivalents(methods, duckType); for (MethodNode candidate : candidates) { MethodNode safeNode = candidate; @@ -1096,7 +1096,7 @@ public abstract class StaticTypeCheckingSupport { } } } - if (bestChoices.size() > 1) { + if (bestChoices.size() > 1 && !duckType) { // GROOVY-6849: prefer extension method in case of ambiguity List<MethodNode> onlyExtensionMethods = new LinkedList<>(); for (MethodNode choice : bestChoices) { @@ -1195,9 +1195,8 @@ public abstract class StaticTypeCheckingSupport { return raw; } - private static List<MethodNode> removeCovariantsAndInterfaceEquivalents(final Collection<MethodNode> collection) { - List<MethodNode> toBeRemoved = new ArrayList<>(); - List<MethodNode> list = new ArrayList<>(new LinkedHashSet<>(collection)); + private static List<MethodNode> removeCovariantsAndInterfaceEquivalents(final Collection<MethodNode> collection, final boolean disjoint) { + List<MethodNode> list = new ArrayList<>(new LinkedHashSet<>(collection)), toBeRemoved = new ArrayList<>(); for (int i = 0, n = list.size(); i < n - 1; i += 1) { MethodNode one = list.get(i); if (toBeRemoved.contains(one)) continue; @@ -1228,11 +1227,9 @@ public abstract class StaticTypeCheckingSupport { } else if (!oneDC.equals(twoDC)) { if (ParameterUtils.parametersEqual(one.getParameters(), two.getParameters())) { // GROOVY-6882, GROOVY-6970: drop overridden or interface equivalent method - if (twoDC.isInterface() ? oneDC.implementsInterface(twoDC) - : oneDC.isDerivedFrom(twoDC)) { + if (twoDC.isInterface() ? oneDC.implementsInterface(twoDC) : oneDC.isDerivedFrom(twoDC)) { toBeRemoved.add(two); - } else if (oneDC.isInterface() ? twoDC.isInterface() - : twoDC.isDerivedFrom(oneDC)) { + } else if (oneDC.isInterface() ? (disjoint ? twoDC.implementsInterface(oneDC) : twoDC.isInterface()) : twoDC.isDerivedFrom(oneDC)) { toBeRemoved.add(one); } } diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index ae0acf3d27..a0bc295c95 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -3503,7 +3503,20 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { if (areCategoryMethodCalls(mn, name, args)) { addCategoryMethodCallError(call); } - mn = disambiguateMethods(mn, chosenReceiver != null ? chosenReceiver.getType() : null, args, call); + + { + ClassNode obj = chosenReceiver != null ? chosenReceiver.getType() : null; + if (mn.size() > 1 && obj instanceof UnionTypeClassNode) { // GROOVY-8965: support duck-typing using dynamic resolution + ClassNode returnType = mn.stream().map(MethodNode::getReturnType).reduce(WideningCategories::lowestUpperBound).get(); + call.putNodeMetaData(DYNAMIC_RESOLUTION, returnType); + + MethodNode tmp = new MethodNode(name, 0, returnType, Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, null); + tmp.setDeclaringClass(obj); // for storeTargetMethod + mn = Collections.singletonList(tmp); + } else { + mn = disambiguateMethods(mn, obj, args, call); + } + } if (mn.size() == 1) { MethodNode targetMethodCandidate = mn.get(0); @@ -3752,7 +3765,8 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } /** - * Given an object expression (a receiver expression), generate the list of potential receiver types. + * Given an object expression (a message receiver expression), generate list + * of possible types. * * @param objectExpression the receiver expression * @return the list of types the receiver may be @@ -3762,23 +3776,20 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { List<Receiver<String>> owners = new ArrayList<>(); if (typeCheckingContext.delegationMetadata != null && objectExpression instanceof VariableExpression - && ((VariableExpression) objectExpression).getName().equals("owner") + && ((Variable) objectExpression).getName().equals("owner") && /*isNested:*/typeCheckingContext.delegationMetadata.getParent() != null) { List<Receiver<String>> enclosingClass = Collections.singletonList( Receiver.make(typeCheckingContext.getEnclosingClassNode())); addReceivers(owners, enclosingClass, typeCheckingContext.delegationMetadata.getParent(), "owner."); } else { - if (!typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { - List<ClassNode> potentialReceiverType = getTemporaryTypesForExpression(objectExpression); - if (potentialReceiverType != null && !potentialReceiverType.isEmpty()) { - for (ClassNode node : potentialReceiverType) { - owners.add(Receiver.make(node)); - } - } + List<ClassNode> temporaryTypes = getTemporaryTypesForExpression(objectExpression); + int temporaryTypesCount = (temporaryTypes != null ? temporaryTypes.size() : 0); + if (temporaryTypesCount > 0) { // GROOVY-8965, GROOVY-10180, GROOVY-10668 + owners.add(Receiver.make(lowestUpperBound(temporaryTypes))); } if (typeCheckingContext.lastImplicitItType != null && objectExpression instanceof VariableExpression - && ((VariableExpression) objectExpression).getName().equals("it")) { + && ((Variable) objectExpression).getName().equals("it")) { owners.add(Receiver.make(typeCheckingContext.lastImplicitItType)); } if (isClassClassNodeWrappingConcreteType(receiver)) { @@ -3798,6 +3809,9 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } } + if (temporaryTypesCount > 1 && !(objectExpression instanceof VariableExpression)) { + owners.add(Receiver.make(new UnionTypeClassNode(temporaryTypes.toArray(ClassNode.EMPTY_ARRAY)))); + } } return owners; } @@ -4702,37 +4716,25 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { return null; } - private List<MethodNode> disambiguateMethods(List<MethodNode> methods, final ClassNode receiver, final ClassNode[] argTypes, final Expression call) { - if (methods.size() > 1 && receiver != null && argTypes != null) { + private List<MethodNode> disambiguateMethods(List<MethodNode> methods, final ClassNode receiver, final ClassNode[] arguments, final Expression call) { + if (methods.size() > 1 && receiver != null && arguments != null) { List<MethodNode> filteredWithGenerics = new LinkedList<>(); - for (MethodNode methodNode : methods) { - if (typeCheckMethodsWithGenerics(receiver, argTypes, methodNode)) { - if ((methodNode.getModifiers() & Opcodes.ACC_BRIDGE) == 0) { - filteredWithGenerics.add(methodNode); - } + for (MethodNode method : methods) { + if (typeCheckMethodsWithGenerics(receiver, arguments, method) + && (method.getModifiers() & Opcodes.ACC_BRIDGE) == 0) { + filteredWithGenerics.add(method); } } if (filteredWithGenerics.size() == 1) { return filteredWithGenerics; } + methods = extension.handleAmbiguousMethods(methods, call); } - if (methods.size() > 1) { - if (call instanceof MethodCall) { - List<MethodNode> methodNodeList = new LinkedList<>(); - - String methodName = ((MethodCall) call).getMethodAsString(); - - for (MethodNode methodNode : methods) { - if (!methodNode.getName().equals(methodName)) { - continue; - } - methodNodeList.add(methodNode); - } - - methods = methodNodeList; - } + if (methods.size() > 1 && call instanceof MethodCall) { + String methodName = ((MethodCall) call).getMethodAsString(); + methods = methods.stream().filter(m -> m.getName().equals(methodName)).collect(Collectors.toList()); } return methods; diff --git a/src/test/groovy/transform/stc/BugsSTCTest.groovy b/src/test/groovy/transform/stc/BugsSTCTest.groovy index 45f0cd4a91..591a31d724 100644 --- a/src/test/groovy/transform/stc/BugsSTCTest.groovy +++ b/src/test/groovy/transform/stc/BugsSTCTest.groovy @@ -685,18 +685,16 @@ Printer assertScript ''' interface A { void m() } interface B { void m() } - interface C extends A, B {} - - class D { - D(C c) { - c.m() - } + interface C extends A,B { } - class CImpl implements C { - void m() { } + class Impl implements C { + void m() {} } - new D(new CImpl()) + void test(C c) { + c.m() + } + test(new Impl()) ''' } @@ -704,16 +702,16 @@ Printer assertScript ''' interface A { void m() } interface B { void m() } - class C implements A,B { - void m() {} + interface C extends A,B { } - class D { - D(C c) { - c.m() - } + class Impl implements C,A,B { + void m() {} } - new D(new C()) + void test(C c) { + c.m() + } + test(new Impl()) ''' } @@ -721,17 +719,14 @@ Printer assertScript ''' interface A { void m() } interface B { void m() } - interface C extends A,B {} - class CImpl implements C, A,B { + class C implements A,B { void m() {} } - class D { - D(C c) { - c.m() - } - } - new D(new CImpl()) + void test(C c) { + c.m() + } + test(new C()) ''' } diff --git a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy index 93d67cd829..6f1efc8dc7 100644 --- a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy +++ b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy @@ -365,7 +365,7 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { thing.addElement(2) // 'addElement' only in Stack } if (thing instanceof Deque || thing instanceof Stack) { - assert thing.peek() in 1..2 // 'peek' in ArrayDeque and Stack but not LUB + assert thing.peek() in 1..2 // 'peek' in Deque and Stack but not LUB } } test(new Stack()) diff --git a/src/test/org/apache/groovy/parser/antlr4/util/AstDumper.groovy b/src/test/org/apache/groovy/parser/antlr4/util/AstDumper.groovy index 165f2f26eb..f67e01e6c6 100644 --- a/src/test/org/apache/groovy/parser/antlr4/util/AstDumper.groovy +++ b/src/test/org/apache/groovy/parser/antlr4/util/AstDumper.groovy @@ -641,8 +641,7 @@ class AstNodeToScriptVisitor implements CompilationUnit.IPrimaryClassNodeOperati @Override void visitMethodCallExpression(MethodCallExpression expression) { - - Expression objectExp = expression.getObjectExpression() + Expression objectExp = expression.objectExpression if (objectExp instanceof VariableExpression) { visitVariableExpression(objectExp, false) } else { @@ -655,25 +654,23 @@ class AstNodeToScriptVisitor implements CompilationUnit.IPrimaryClassNodeOperati print '?' } print '.' - Expression method = expression.getMethod() + Expression method = expression.method if (method instanceof ConstantExpression) { visitConstantExpression(method, true) } else { method.visit(this) } - expression.getArguments().visit(this) + expression.arguments.visit(this) } @Override void visitStaticMethodCallExpression(StaticMethodCallExpression expression) { print expression?.ownerType?.name + '.' + expression?.method - if (expression?.arguments instanceof VariableExpression || expression?.arguments instanceof MethodCallExpression) { - print '(' - expression?.arguments?.visit this - print ')' - } else { - expression?.arguments?.visit this - } + boolean parens = expression?.arguments instanceof VariableExpression + || expression?.arguments instanceof MethodCallExpression + if (parens) print '(' + expression?.arguments?.visit this + if (parens) print ')' } @Override @@ -1132,5 +1129,4 @@ class AstNodeToScriptVisitor implements CompilationUnit.IPrimaryClassNodeOperati } return true } - } diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/TypeInferenceStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/TypeInferenceStaticCompileTest.groovy index b1ff1521c5..b7ae38f82b 100644 --- a/src/test/org/codehaus/groovy/classgen/asm/sc/TypeInferenceStaticCompileTest.groovy +++ b/src/test/org/codehaus/groovy/classgen/asm/sc/TypeInferenceStaticCompileTest.groovy @@ -26,33 +26,9 @@ import groovy.transform.stc.TypeInferenceSTCTest */ class TypeInferenceStaticCompileTest extends TypeInferenceSTCTest implements StaticCompilationTestSupport { - @Override @NotYetImplemented + @Override + @NotYetImplemented void testInstanceOf9() { - super.testInstanceOf9() - } - - @Override @NotYetImplemented - void testMultipleInstanceOf1() { - super.testMultipleInstanceOf1() - } - - @Override @NotYetImplemented - void testMultipleInstanceOf2() { - super.testMultipleInstanceOf2() - } - - @Override @NotYetImplemented - void testMultipleInstanceOf3() { - super.testMultipleInstanceOf3() - } - - @Override @NotYetImplemented - void testMultipleInstanceOf4() { - super.testMultipleInstanceOf4() - } - - @Override @NotYetImplemented - void testMultipleInstanceOf5() { - super.testMultipleInstanceOf5() + super.testInstanceOf9() // GROOVY-7971 } } diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/ReproducibleBytecodeBugs.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/ReproducibleBytecodeBugs.groovy index 7ca44be0f8..c4586791c0 100644 --- a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/ReproducibleBytecodeBugs.groovy +++ b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/ReproducibleBytecodeBugs.groovy @@ -16,35 +16,34 @@ * specific language governing permissions and limitations * under the License. */ - - package org.codehaus.groovy.classgen.asm.sc.bugs import groovy.transform.stc.StaticTypeCheckingTestCase import org.codehaus.groovy.classgen.asm.sc.StaticCompilationTestSupport class ReproducibleBytecodeBugs extends StaticTypeCheckingTestCase implements StaticCompilationTestSupport { + // GROOVY-8142 - void testShouldNotProduceDeterministicBytecode() { + void testShouldProduceDeterministicBytecode() { 100.times { assertScript ''' - interface Project { - File file() - } - interface FileOperations { - File file() - } - interface ProjectInternal extends Project, FileOperations { - } + interface Project { + File file() + } + interface FileOperations { + File file() + } + interface ProjectInternal extends Project, FileOperations { + } - class Check { - void test(ProjectInternal p) { - def f = p.file() + class Check { + void test(ProjectInternal p) { + def f = p.file() + } } - } - def c = new Check() - ''' + def c = new Check() + ''' assert astTrees['Check'][1].contains('INVOKEINTERFACE FileOperations.file ()Ljava/io/File;') : "Incorrect bytecode found in iteration $it" } @@ -54,20 +53,20 @@ class ReproducibleBytecodeBugs extends StaticTypeCheckingTestCase implements Sta void testShouldAlwaysAddClosureSharedVariablesInSameOrder() { 100.times { assertScript ''' - class Check { - void test() { - def xx = true - def moot = "bar" - def kr = [:] - def zorg = [] - def cl = { - def (x,y,z,t) = [xx, moot, kr , zorg] + class Check { + void test() { + def xx = true + def moot = "bar" + def kr = [:] + def zorg = [] + def cl = { + def (x,y,z,t) = [xx, moot, kr , zorg] + } } } - } - def c = new Check() - ''' + def c = new Check() + ''' def bytecode = astTrees['Check$_test_closure1'][1] assertOrdered it, bytecode, @@ -75,17 +74,15 @@ class ReproducibleBytecodeBugs extends StaticTypeCheckingTestCase implements Sta 'PUTFIELD Check$_test_closure1.moot', 'PUTFIELD Check$_test_closure1.kr', 'PUTFIELD Check$_test_closure1.zorg' - } } - - private static void assertOrdered(int iteration, String bytecode, String... elements) { + private static void assertOrdered(int iteration, String bytecode, String... strings) { int start = 0 - elements.eachWithIndex { it, i -> - start = bytecode.indexOf(it, start) + strings.eachWithIndex { string, index -> + start = bytecode.indexOf(string, start) if (start == -1) { - throw new AssertionError("Iteration $iteration - Element [$it] not found in order (expected to find it at index $i)") + throw new AssertionError("Iteration $iteration - Element [$string] not found in order (expected to find it at index $index)") } } }
