This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY-9281 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit fd5cf2f5fdacb3c31aca80c699152a655eec1403 Author: Eric Milles <[email protected]> AuthorDate: Fri Oct 18 12:56:32 2019 -0500 GROOVY-9281: allow package-private for resolved nested type refactor ResolveVisitor to use single method for testing visibility --- .../codehaus/groovy/control/ResolveVisitor.java | 55 +++------- src/test/groovy/bugs/groovy8531/Groovy8531.groovy | 115 +++++++++++++++++++++ .../groovy/bugs/groovy8531/Groovy8531Bug.groovy | 80 -------------- src/test/groovy/bugs/groovy8531/Reducer.java | 12 ++- 4 files changed, 139 insertions(+), 123 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java b/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java index ac6d110..5943cb4 100644 --- a/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java +++ b/src/main/java/org/codehaus/groovy/control/ResolveVisitor.java @@ -77,14 +77,13 @@ import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.BiConsumer; import static org.codehaus.groovy.ast.CompileUnit.ConstructedOuterNestedClassNode; import static org.codehaus.groovy.ast.GenericsType.GenericsTypeName; import static org.codehaus.groovy.ast.tools.ClosureUtils.getParametersSafe; -import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage; -import static org.codehaus.groovy.ast.tools.GeneralUtils.isDefaultVisibility; /** * Visitor to resolve Types and convert VariableExpression to @@ -150,8 +149,6 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer { } } - - private static String replacePoints(String name) { return name.replace('.','$'); } @@ -309,40 +306,20 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer { private boolean resolveToNestedOfCurrentClassAndSuperClasses(ClassNode type) { // GROOVY-8531: Fail to resolve type defined in super class written in Java - for (ClassNode enclosingClassNode = currentClass; ClassHelper.OBJECT_TYPE != enclosingClassNode && null != enclosingClassNode; enclosingClassNode = enclosingClassNode.getSuperClass()) { - if(resolveToNested(enclosingClassNode, type)) return true; - } - - return false; - } - - private boolean resolveToNested(ClassNode enclosingType, ClassNode type) { - if (type instanceof ConstructedNestedClass) return false; - // GROOVY-3110: It may be an inner enum defined by this class itself, in which case it does not need to be - // explicitly qualified by the currentClass name - String name = type.getName(); - if (enclosingType != type && !name.contains(".") && type.getClass().equals(ClassNode.class)) { - ClassNode tmp = new ConstructedNestedClass(enclosingType,name); - if (resolve(tmp)) { - if (!checkInnerTypeVisibility(enclosingType, tmp)) return false; - - type.setRedirect(tmp); - return true; + for (ClassNode enclosingClass = currentClass; enclosingClass != null && enclosingClass != ClassHelper.OBJECT_TYPE; enclosingClass = enclosingClass.getSuperClass()) { + // GROOVY-3110: It may be an inner enum defined by this class itself + String name = type.getName(); + if (enclosingClass != type && name.indexOf('.') < 0 && type.getClass().equals(ClassNode.class)) { + ClassNode tmp = new ConstructedNestedClass(enclosingClass, name); + if (resolve(tmp) && (enclosingClass == currentClass || isVisibleNestedClass(tmp, currentClass))) { + type.setRedirect(tmp); + return true; + } } } - return false; } - private boolean checkInnerTypeVisibility(ClassNode enclosingType, ClassNode innerClassNode) { - if (currentClass == enclosingType) { - return true; - } - - int modifiers = innerClassNode.getModifiers(); - return Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers); - } - private void resolveOrFail(ClassNode type, String msg, ASTNode node) { if (resolve(type)) return; if (resolveToInner(type)) return; @@ -1072,12 +1049,10 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer { return ret; } - private boolean isVisibleNestedClass(ClassNode type, ClassNode ceType) { - if (!type.isRedirectNode()) return false; - ClassNode redirect = type.redirect(); - if (Modifier.isPublic(redirect.getModifiers()) || Modifier.isProtected(redirect.getModifiers())) return true; - // package local - return isDefaultVisibility(redirect.getModifiers()) && inSamePackage(ceType, redirect); + private static boolean isVisibleNestedClass(ClassNode innerType, ClassNode outerType) { + int modifiers = innerType.getModifiers(); + return Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers) + || (!Modifier.isPrivate(modifiers) && Objects.equals(innerType.getPackageName(), outerType.getPackageName())); } private boolean directlyImplementsTrait(ClassNode trait) { @@ -1151,7 +1126,7 @@ public class ResolveVisitor extends ClassCodeExpressionTransformer { t = new LowerCaseClass(name); } isClass = resolve(t); - if(!isClass) { + if (!isClass) { isClass = resolveToNestedOfCurrentClassAndSuperClasses(t); } } diff --git a/src/test/groovy/bugs/groovy8531/Groovy8531.groovy b/src/test/groovy/bugs/groovy8531/Groovy8531.groovy new file mode 100644 index 0000000..485af3a --- /dev/null +++ b/src/test/groovy/bugs/groovy8531/Groovy8531.groovy @@ -0,0 +1,115 @@ +/* + * 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.groovy8531 + +import groovy.transform.CompileStatic +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript +import static groovy.test.GroovyAssert.shouldFail + +@CompileStatic +final class Groovy8531 { + + @Test + void testVisibleInnerTypes() { + assertScript ''' + package groovy.bugs.groovy8531 + + class Example extends Reducer { + public void reduce1(PublicContext context) {} + public void reduce2(ProtectedContext context) {} + public void reduce3(PackagePrivateContext context) {} + + public void reduce4(PublicStaticContext context) {} + public void reduce5(ProtectedStaticContext context) {} + public void reduce6(PackagePrivateStaticContext context) {} + + public void reduce7(PublicBaseContext context) {} + public void reduce8(ProtectedBaseContext context) {} + public void reduce9(PackagePrivateBaseContext context) {} + + public void reduce10(PublicStaticBaseContext context) {} + public void reduce11(ProtectedStaticBaseContext context) {} + public void reduce12(PackagePrivateStaticBaseContext context) {} + + public void reduce13(InterfaceContext context) {} + + public boolean isDynamic(Type type) { + return Type.DYNAMIC == type + } + } + + new Example().reduce1(null) + new Example().reduce2(null) + new Example().reduce3(null) + new Example().reduce4(null) + new Example().reduce5(null) + new Example().reduce6(null) + new Example().reduce7(null) + new Example().reduce8(null) + new Example().reduce9(null) + new Example().reduce10(null) + new Example().reduce11(null) + new Example().reduce12(null) + new Example().reduce13(null) + + assert new Example().isDynamic(Reducer.Type.DYNAMIC) + ''' + } + + @Test + void testPrivateInnerType1() { + def err = shouldFail ''' + package groovy.bugs.groovy8531 + class Example extends Reducer { + void reduce(PrivateContext context) {} + } + ''' + + assert err.message.contains('unable to resolve class PrivateContext') + } + + @Test + void testPrivateInnerType2() { + def err = shouldFail ''' + package groovy.bugs.groovy8531 + class Example extends Reducer { + void reduce(PrivateBaseContext context) {} + } + ''' + + assert err.message.contains('unable to resolve class PrivateBaseContext') + } + + @Test + void testPackagePrivateInnerType() { + def err = shouldFail ''' + package groovy.bugs.groovy9281 + + import groovy.bugs.groovy8531.Reducer + + class Example extends Reducer { + void reduce(PackagePrivateContext context) {} + } + ''' + + assert err.message.contains('unable to resolve class PackagePrivateContext') + } +} diff --git a/src/test/groovy/bugs/groovy8531/Groovy8531Bug.groovy b/src/test/groovy/bugs/groovy8531/Groovy8531Bug.groovy deleted file mode 100644 index 8b09a30..0000000 --- a/src/test/groovy/bugs/groovy8531/Groovy8531Bug.groovy +++ /dev/null @@ -1,80 +0,0 @@ -/* - * 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.groovy8531 - -import groovy.test.GroovyTestCase - -class Groovy8531Bug extends GroovyTestCase { - void testPublicAndProtectedInnerType() { - assertScript ''' - package groovy.bugs.groovy8531 - class Example extends Reducer { - public void reduce(PublicContext context) {} - public void reduce2(ProtectedContext context) {} - public void reduce3(PublicStaticContext context) {} - public void reduce4(ProtectedStaticContext context) {} - - public void reduce5(PublicBaseContext context) {} - public void reduce6(ProtectedBaseContext context) {} - public void reduce7(PublicStaticBaseContext context) {} - public void reduce8(ProtectedStaticBaseContext context) {} - - public void reduce9(InterfaceContext context) {} - - public boolean isDynamic(Type type) { - return Type.DYNAMIC == type - } - } - - new Example().reduce(null) - new Example().reduce2(null) - new Example().reduce3(null) - new Example().reduce4(null) - - new Example().reduce5(null) - new Example().reduce6(null) - new Example().reduce7(null) - new Example().reduce8(null) - - new Example().reduce9(null) - - assert new Example().isDynamic(Reducer.Type.DYNAMIC) - ''' - } - - void testPrivateInnerType() { - def errMsg = shouldFail ''' - package groovy.bugs.groovy8531 - class Example extends Reducer { - public void reduce3(PrivateContext context) {} - } - ''' - assert errMsg.contains('unable to resolve class PrivateContext') - } - - void testPrivateInnerType2() { - def errMsg = shouldFail ''' - package groovy.bugs.groovy8531 - class Example extends Reducer { - public void reduce3(PrivateBaseContext context) {} - } - ''' - assert errMsg.contains('unable to resolve class PrivateBaseContext') - } -} diff --git a/src/test/groovy/bugs/groovy8531/Reducer.java b/src/test/groovy/bugs/groovy8531/Reducer.java index 8652196..64a12c4 100644 --- a/src/test/groovy/bugs/groovy8531/Reducer.java +++ b/src/test/groovy/bugs/groovy8531/Reducer.java @@ -25,19 +25,25 @@ interface Reducable { class BaseReducer { public abstract class PublicBaseContext {} protected abstract class ProtectedBaseContext {} + /*package*/ abstract class PackagePrivateBaseContext {} + public static abstract class PublicStaticBaseContext {} protected static abstract class ProtectedStaticBaseContext {} + /*package*/ static abstract class PackagePrivateStaticBaseContext {} + private abstract class PrivateBaseContext {} } public class Reducer extends BaseReducer implements Reducable { public abstract class PublicContext {} protected abstract class ProtectedContext {} + /*package*/ abstract class PackagePrivateContext {} + public static abstract class PublicStaticContext {} protected static abstract class ProtectedStaticContext {} + /*package*/ static abstract class PackagePrivateStaticContext {} + private abstract class PrivateContext {} - public enum Type { - DYNAMIC, STATIC - } + public enum Type { DYNAMIC, STATIC } }
