This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY_3_0_X in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 9352e9c3e55bce5988f9b7de034c7305a82eec41 Author: Eric Milles <[email protected]> AuthorDate: Thu May 27 15:14:27 2021 -0500 GROOVY-5239: prefer outer class method over static import method Conflicts: src/main/java/org/codehaus/groovy/control/StaticImportVisitor.java --- .../groovy/ast/expr/MethodCallExpression.java | 4 +- .../groovy/control/StaticImportVisitor.java | 22 ++--- src/test/groovy/bugs/Groovy5239.groovy | 100 +++++++++++++++++++++ 3 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/ast/expr/MethodCallExpression.java b/src/main/java/org/codehaus/groovy/ast/expr/MethodCallExpression.java index 4f8583c..6a6379c 100644 --- a/src/main/java/org/codehaus/groovy/ast/expr/MethodCallExpression.java +++ b/src/main/java/org/codehaus/groovy/ast/expr/MethodCallExpression.java @@ -115,9 +115,7 @@ public class MethodCallExpression extends Expression implements MethodCall { * calculated method name, but a constant. */ public String getMethodAsString() { - if (!(method instanceof ConstantExpression)) return null; - ConstantExpression constant = (ConstantExpression) method; - return constant.getText(); + return (method instanceof ConstantExpression ? method.getText() : null); } public Expression getObjectExpression() { diff --git a/src/main/java/org/codehaus/groovy/control/StaticImportVisitor.java b/src/main/java/org/codehaus/groovy/control/StaticImportVisitor.java index 5f7e1a5..ad24384 100644 --- a/src/main/java/org/codehaus/groovy/control/StaticImportVisitor.java +++ b/src/main/java/org/codehaus/groovy/control/StaticImportVisitor.java @@ -59,6 +59,8 @@ import static org.apache.groovy.ast.tools.ClassNodeUtils.hasPossibleStaticProper import static org.apache.groovy.ast.tools.ClassNodeUtils.hasStaticProperty; import static org.apache.groovy.ast.tools.ClassNodeUtils.isInnerClass; import static org.apache.groovy.ast.tools.ClassNodeUtils.isValidAccessorName; +import static org.apache.groovy.ast.tools.ExpressionUtils.isSuperExpression; +import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper; import static org.apache.groovy.ast.tools.ExpressionUtils.transformInlineConstants; import static org.apache.groovy.util.BeanUtils.capitalize; import static org.codehaus.groovy.ast.tools.ClosureUtils.getParametersSafe; @@ -248,16 +250,16 @@ public class StaticImportVisitor extends ClassCodeExpressionTransformer { Expression args = transform(mce.getArguments()); if (mce.isImplicitThis()) { - if (currentClass.tryFindPossibleMethod(mce.getMethodAsString(), args) == null) { + String name = mce.getMethodAsString(); + if (currentClass.tryFindPossibleMethod(name, args) == null + && currentClass.getOuterClasses().stream().noneMatch(oc -> oc.tryFindPossibleMethod(name, args) != null)) { Expression result = findStaticMethodImportFromModule(method, args); if (result != null) { setSourcePosition(result, mce); return result; } - if (method instanceof ConstantExpression && !inLeftExpression) { - // could be a closure field - String methodName = (String) ((ConstantExpression) method).getValue(); - result = findStaticFieldOrPropAccessorImportFromModule(methodName); + if (name != null && !inLeftExpression) { // maybe a closure field + result = findStaticFieldOrPropAccessorImportFromModule(name); if (result != null) { result = new MethodCallExpression(result, "call", args); result.setSourcePosition(mce); @@ -265,14 +267,13 @@ public class StaticImportVisitor extends ClassCodeExpressionTransformer { } } } - } else if (currentMethod != null && currentMethod.isStatic() && (object instanceof VariableExpression && ((VariableExpression) object).isSuperExpression())) { + } else if (currentMethod != null && currentMethod.isStatic() && isSuperExpression(object)) { Expression result = new MethodCallExpression(new ClassExpression(currentClass.getSuperClass()), method, args); result.setSourcePosition(mce); return result; } - if (method instanceof ConstantExpression && ((ConstantExpression) method).getValue() instanceof String && (mce.isImplicitThis() - || (object instanceof VariableExpression && (((VariableExpression) object).isThisExpression() || ((VariableExpression) object).isSuperExpression())))) { + if (method instanceof ConstantExpression && ((ConstantExpression) method).getValue() instanceof String && (mce.isImplicitThis() || isThisOrSuper(object))) { String methodName = (String) ((ConstantExpression) method).getValue(); boolean foundInstanceMethod = (currentMethod != null && !currentMethod.isStatic() && currentClass.hasPossibleMethod(methodName, args)); @@ -358,9 +359,8 @@ public class StaticImportVisitor extends ClassCodeExpressionTransformer { } protected Expression transformPropertyExpression(PropertyExpression pe) { - if (currentMethod!=null && currentMethod.isStatic() - && pe.getObjectExpression() instanceof VariableExpression - && ((VariableExpression) pe.getObjectExpression()).isSuperExpression()) { + if (currentMethod != null && currentMethod.isStatic() + && isSuperExpression(pe.getObjectExpression())) { PropertyExpression pexp = new PropertyExpression( new ClassExpression(currentClass.getSuperClass()), transform(pe.getProperty()) diff --git a/src/test/groovy/bugs/Groovy5239.groovy b/src/test/groovy/bugs/Groovy5239.groovy new file mode 100644 index 0000000..e76281c --- /dev/null +++ b/src/test/groovy/bugs/Groovy5239.groovy @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript + +final class Groovy5239 { + + static class PublicStatic { + static who() { 'PublicStatic' } + } + + @Test @groovy.test.NotYetImplemented + void testStaticImportVersusDelegateMethod() { + assertScript ''' + import static groovy.bugs.Groovy5239.PublicStatic.who + + class C { + def who() { + 'C' + } + } + + new C().with { + assert 'C' == who() + } + ''' + } + + @Test + void testStaticImportVersusOuterClassMethod1() { + assertScript ''' + import static groovy.bugs.Groovy5239.PublicStatic.who + + class C { + def who() { + 'C' + } + void test() { + assert 'C' == who() + new D().test() + } + + class D { + void test() { + assert 'C' == who() // resolves to static import + } + } + } + + new C().test() + ''' + } + + @Test + void testStaticImportVersusOuterClassMethod2() { + assertScript ''' + import static groovy.bugs.Groovy5239.PublicStatic.who + + class C { + def who() { + 'C' + } + } + + class D extends C { + void test() { + assert 'C' == who() + new E().test() + } + + class E { + void test() { + assert 'C' == who() // resolves to static import + } + } + } + + new D().test() + ''' + } +}
