[
https://issues.apache.org/jira/browse/GROOVY-11916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18143311#comment-18143311
]
ASF GitHub Bot commented on GROOVY-11916:
-----------------------------------------
blackdrag commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4544863829
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -791,6 +792,141 @@ private void ambiguousClass(final ClassNode type, final
ClassNode iType, final S
}
}
+ /**
+ * Binds a simple name from on-demand imports.
+ * <p>
+ * A source import shadows every default import, including implicit
+ * {@code java.lang.*} and star imports added by a compilation customizer.
+ * Source static-import-on-demand, source type-import-on-demand, and
+ * packages expanded from a source {@code import module} are one sequence
+ * in source order. The first accessible type in that sequence is bound.
+ * A later import does not create an ambiguity, and an inaccessible type
+ * is skipped so it does not hide a later accessible type.
+ * <p>
+ * Customizer on-demand imports have no source location. They are tried
+ * after every source on-demand import and before the language defaults,
+ * so a configured star can still supply {@code Date} or {@code String}
+ * without tying with {@code java.lang} or {@code java.util}. Within that
+ * tier, static stars precede package stars because the two are stored
+ * separately. An explicit single-type import is resolved earlier and
+ * therefore wins over every on-demand import.
+ *
+ * @return {@code true} if the name was bound
+ */
+ private boolean resolveFromOnDemandImports(final ClassNode type, final
String name, final ModuleNode module) {
+ OnDemandImports order = onDemandImports(module);
+ for (ImportNode importNode : order.declared) {
+ if (bindOnDemand(type, name, importNode)) return true;
+ }
+ for (ImportNode importNode : order.customizer) {
+ if (bindOnDemand(type, name, importNode)) return true;
+ }
+ return false;
+ }
+
+ private boolean bindOnDemand(final ClassNode type, final String name,
final ImportNode importNode) {
+ ClassNode outer = importNode.getType();
+ if (outer != null) {
+ ClassNode tmp = new ConstructedNestedClass(outer, name);
+ if (resolve(tmp, false, false, true) &&
Modifier.isStatic(tmp.getModifiers())
Review Comment:
it is unclear why here only a static class is valid. It may be an issue of
context, but I think it needs some explanation.
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -791,6 +792,141 @@ private void ambiguousClass(final ClassNode type, final
ClassNode iType, final S
}
}
+ /**
+ * Binds a simple name from on-demand imports.
+ * <p>
+ * A source import shadows every default import, including implicit
+ * {@code java.lang.*} and star imports added by a compilation customizer.
+ * Source static-import-on-demand, source type-import-on-demand, and
+ * packages expanded from a source {@code import module} are one sequence
+ * in source order. The first accessible type in that sequence is bound.
+ * A later import does not create an ambiguity, and an inaccessible type
+ * is skipped so it does not hide a later accessible type.
+ * <p>
+ * Customizer on-demand imports have no source location. They are tried
+ * after every source on-demand import and before the language defaults,
+ * so a configured star can still supply {@code Date} or {@code String}
+ * without tying with {@code java.lang} or {@code java.util}. Within that
+ * tier, static stars precede package stars because the two are stored
+ * separately. An explicit single-type import is resolved earlier and
+ * therefore wins over every on-demand import.
+ *
+ * @return {@code true} if the name was bound
+ */
+ private boolean resolveFromOnDemandImports(final ClassNode type, final
String name, final ModuleNode module) {
+ OnDemandImports order = onDemandImports(module);
+ for (ImportNode importNode : order.declared) {
+ if (bindOnDemand(type, name, importNode)) return true;
+ }
+ for (ImportNode importNode : order.customizer) {
+ if (bindOnDemand(type, name, importNode)) return true;
+ }
+ return false;
+ }
+
+ private boolean bindOnDemand(final ClassNode type, final String name,
final ImportNode importNode) {
+ ClassNode outer = importNode.getType();
+ if (outer != null) {
+ ClassNode tmp = new ConstructedNestedClass(outer, name);
+ if (resolve(tmp, false, false, true) &&
Modifier.isStatic(tmp.getModifiers())
+ && isStarImportAccessible(tmp, outer)) {
+ type.setRedirect(tmp.redirect());
+ return true;
+ }
+ return false;
+ }
+ String packageName = importNode.getPackageName();
+ if (packageName == null) return false;
+ ClassNode tmp = new ConstructedClassWithPackage(packageName, name);
+ if (resolve(tmp, false, false, true) && isStarImportAccessible(tmp,
null)) {
+ type.setRedirect(tmp.redirect());
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Source on-demand imports in source order, then customizer on-demand
+ * imports. Computed once per module; import lists are fixed before
resolve.
+ */
+ private static OnDemandImports onDemandImports(final ModuleNode module) {
+ OnDemandImports cached = module.getNodeMetaData(OnDemandImports.class);
+ if (cached != null) return cached;
+
+ List<PositionedImport> positioned = new ArrayList<>();
+ int sequence = 0;
+ for (ImportNode node : module.getStaticStarImports().values()) {
+ if (node.getLineNumber() > 0) {
+ positioned.add(new PositionedImport(node.getLineNumber(),
node.getColumnNumber(), sequence++, node));
+ }
+ }
+ for (ImportNode node : module.getStarImports()) {
+ if (node.getLineNumber() > 0) {
+ positioned.add(new PositionedImport(node.getLineNumber(),
node.getColumnNumber(), sequence++, node));
+ }
+ }
+ List<ImportNode> pendingModule = new ArrayList<>();
+ for (ImportNode node : module.getModuleStarImports()) {
+ pendingModule.add(node);
+ if (node.getLineNumber() > 0) {
+ int line = node.getLineNumber();
+ int column = node.getColumnNumber();
+ for (ImportNode part : pendingModule) {
+ positioned.add(new PositionedImport(line, column,
sequence++, part));
+ }
+ pendingModule.clear();
+ }
+ }
+ positioned.sort((a, b) -> {
+ int byLine = Integer.compare(a.line, b.line);
+ if (byLine != 0) return byLine;
+ int byColumn = Integer.compare(a.column, b.column);
+ if (byColumn != 0) return byColumn;
+ return Integer.compare(a.sequence, b.sequence);
+ });
+
+ List<ImportNode> declared = new ArrayList<>(positioned.size());
+ for (PositionedImport positionedImport : positioned) {
+ declared.add(positionedImport.node);
+ }
+ List<ImportNode> customizer = new ArrayList<>();
+ for (ImportNode node : module.getStaticStarImports().values()) {
+ if (node.getLineNumber() <= 0) customizer.add(node);
Review Comment:
I am just taking this one here to put the comment somewhere... first of all,
isn't the sort on position stable? If yes, then sequence looks surplus. If you
would put them all in one list (including the ones with negative index) and
then sort, then with a stable sort you would get the negative ones first in
addition order and then the positives ones, the ones with the same line/col
information would also be in addition order. If you would then put the the
negative ones last, you could probably work with a single list. I may have
missed some usages though that speak against that.
And then the question is of course... is this really so much work that it is
worth caching? We are not talking about huge lists here. And if we did, we
should not forget that you do not remove the node meta data, which means the
list is carried through the whole compilation process as potential ballast
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -1195,6 +1289,72 @@ protected Expression transformPropertyExpression(final
PropertyExpression pe) {
return isTopLevelProperty ? correctClassClassChain(xe) : xe;
}
+ /**
+ * Whether a type found through a star import, a module import, or a
default
+ * import is accessible from {@link #currentClass}.
+ * <p>
+ * Public types are visible. A package-private type stays visible in its
own
+ * package, a protected member type stays visible to subclasses of its
enclosing
+ * type, and a private member type stays visible inside the enclosing
nest. A
+ * public member of an inaccessible enclosing type is not visible, so a
star
+ * import cannot bind it.
+ * <p>
+ * Single-type imports are not filtered. An explicit {@code import
p.Hidden}
+ * or a fully-qualified name still resolves, matching long-standing Groovy
+ * behaviour, and {@code Class.forName} is unaffected. Static field imports
+ * already apply an access check ({@link StaticImportVisitor}, since
Groovy 5).
+ *
+ * @param type a resolved candidate
+ * @param knownOuter the type named by {@code import some.Type.*} when
+ * {@code type} is a binary member type that does not
expose
+ * {@link ClassNode#getOuterClass()}
+ * @return {@code true} if the star import may bind {@code type}
+ */
+ private boolean isStarImportAccessible(final ClassNode type, final
ClassNode knownOuter) {
+ ClassNode outer = type.getOuterClass();
+ if (outer == null) {
+ outer = knownOuter;
+ }
+ if (outer == null) {
+ outer = binaryEnclosing(type);
Review Comment:
The question here is... why do we have a type, that is supposed to be an
InnerClassNode (or its redirect at least) not as InnerClassNode. If it would be
an InnerClassNode, then getOuterClass would not be null. And if we had the
gurantee it is an InnerClassNode when it factually is an inner class, then we
would not need binaryEnclosing(type)
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -1195,6 +1289,72 @@ protected Expression transformPropertyExpression(final
PropertyExpression pe) {
return isTopLevelProperty ? correctClassClassChain(xe) : xe;
}
+ /**
+ * Whether a type found through a star import, a module import, or a
default
+ * import is accessible from {@link #currentClass}.
+ * <p>
+ * Public types are visible. A package-private type stays visible in its
own
+ * package, a protected member type stays visible to subclasses of its
enclosing
+ * type, and a private member type stays visible inside the enclosing
nest. A
+ * public member of an inaccessible enclosing type is not visible, so a
star
+ * import cannot bind it.
+ * <p>
+ * Single-type imports are not filtered. An explicit {@code import
p.Hidden}
+ * or a fully-qualified name still resolves, matching long-standing Groovy
+ * behaviour, and {@code Class.forName} is unaffected. Static field imports
+ * already apply an access check ({@link StaticImportVisitor}, since
Groovy 5).
+ *
+ * @param type a resolved candidate
+ * @param knownOuter the type named by {@code import some.Type.*} when
+ * {@code type} is a binary member type that does not
expose
+ * {@link ClassNode#getOuterClass()}
+ * @return {@code true} if the star import may bind {@code type}
+ */
+ private boolean isStarImportAccessible(final ClassNode type, final
ClassNode knownOuter) {
+ ClassNode outer = type.getOuterClass();
+ if (outer == null) {
+ outer = knownOuter;
Review Comment:
it was not the case before, but is knownOuter now not always null?
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -1195,6 +1289,72 @@ protected Expression transformPropertyExpression(final
PropertyExpression pe) {
return isTopLevelProperty ? correctClassClassChain(xe) : xe;
}
+ /**
+ * Whether a type found through a star import, a module import, or a
default
+ * import is accessible from {@link #currentClass}.
+ * <p>
+ * Public types are visible. A package-private type stays visible in its
own
+ * package, a protected member type stays visible to subclasses of its
enclosing
+ * type, and a private member type stays visible inside the enclosing
nest. A
+ * public member of an inaccessible enclosing type is not visible, so a
star
+ * import cannot bind it.
+ * <p>
+ * Single-type imports are not filtered. An explicit {@code import
p.Hidden}
+ * or a fully-qualified name still resolves, matching long-standing Groovy
+ * behaviour, and {@code Class.forName} is unaffected. Static field imports
+ * already apply an access check ({@link StaticImportVisitor}, since
Groovy 5).
+ *
+ * @param type a resolved candidate
+ * @param knownOuter the type named by {@code import some.Type.*} when
+ * {@code type} is a binary member type that does not
expose
+ * {@link ClassNode#getOuterClass()}
+ * @return {@code true} if the star import may bind {@code type}
+ */
+ private boolean isStarImportAccessible(final ClassNode type, final
ClassNode knownOuter) {
+ ClassNode outer = type.getOuterClass();
+ if (outer == null) {
+ outer = knownOuter;
+ }
+ if (outer == null) {
+ outer = binaryEnclosing(type);
+ }
+ if (outer != null && outer != type && !isStarImportAccessible(outer,
null)) {
+ return false;
Review Comment:
assuming type.getOuterClass() sets outer to !=null. Why is it guaranteed
that outer is resolved? If it is not resolved it will have a public modifier
regardless of its accessibility
##########
src/test/groovy/bugs/Groovy11916.groovy:
##########
@@ -0,0 +1,727 @@
+/*
+ * 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 bugs
+
+import groovy.lang.GroovyClassLoader
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.Phases
+import org.codehaus.groovy.control.customizers.ImportCustomizer
+import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Modifier
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Star imports, module imports, and default imports resolve only accessible
+ * types. A source import shadows those defaults, including {@code java.lang}.
+ * Two source star imports bind the first accessible type in source order.
+ * An explicit single-type import, a fully qualified name, and
+ * {@code Class.forName} still name a type that is not public.
+ */
+final class Groovy11916 {
+
+ private static GroovyClassLoader types
+
+ @BeforeAll
+ static void compilePackagePrivateTypes() {
+ types = compile(Groovy11916.classLoader, [
+ 'p/Types': '''
+ package p
+
+ @groovy.transform.PackageScope
+ class Hidden {}
+
+ class Visible {}
+
+ @groovy.transform.PackageScope
+ class Outer {
+ public static class Inner {}
+ }
+
+ class Protector {
+ protected static class Secret {}
+ }
+
+ class Priv {
+ private static class Secret {}
+ public static class Shown {}
+ }
+
+ @groovy.transform.PackageScope
+ class Collision {}
+ '''
+ ])
+ assert !Modifier.isPublic(types.loadClass('p.Hidden').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Collision').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Visible').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Outer').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Outer$Inner').modifiers)
+ assert
Modifier.isProtected(types.loadClass('p.Protector$Secret').modifiers)
+ assert Modifier.isPrivate(types.loadClass('p.Priv$Secret').modifiers)
Review Comment:
why do we test the modifiers here?
##########
src/test/groovy/bugs/Groovy11916.groovy:
##########
@@ -0,0 +1,727 @@
+/*
+ * 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 bugs
+
+import groovy.lang.GroovyClassLoader
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.Phases
+import org.codehaus.groovy.control.customizers.ImportCustomizer
+import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Modifier
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Star imports, module imports, and default imports resolve only accessible
+ * types. A source import shadows those defaults, including {@code java.lang}.
+ * Two source star imports bind the first accessible type in source order.
+ * An explicit single-type import, a fully qualified name, and
+ * {@code Class.forName} still name a type that is not public.
+ */
+final class Groovy11916 {
+
+ private static GroovyClassLoader types
+
+ @BeforeAll
+ static void compilePackagePrivateTypes() {
+ types = compile(Groovy11916.classLoader, [
+ 'p/Types': '''
+ package p
+
+ @groovy.transform.PackageScope
+ class Hidden {}
+
+ class Visible {}
+
+ @groovy.transform.PackageScope
+ class Outer {
+ public static class Inner {}
+ }
+
+ class Protector {
+ protected static class Secret {}
+ }
+
+ class Priv {
+ private static class Secret {}
+ public static class Shown {}
+ }
+
+ @groovy.transform.PackageScope
+ class Collision {}
+ '''
+ ])
+ assert !Modifier.isPublic(types.loadClass('p.Hidden').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Collision').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Visible').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Outer').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Outer$Inner').modifiers)
+ assert
Modifier.isProtected(types.loadClass('p.Protector$Secret').modifiers)
+ assert Modifier.isPrivate(types.loadClass('p.Priv$Secret').modifiers)
+ }
+
+ private static GroovyClassLoader compile(ClassLoader parent, Map<String,
String> sources, CompilerConfiguration config = null) {
+ def loader = new GroovyClassLoader(parent, config)
+ def unit = new CompilationUnit(config, null, loader)
+ sources.each { name, text -> unit.addSource(name + '.groovy', text) }
+ unit.compile(Phases.CLASS_GENERATION)
+ unit.classes.sort { it.name }.each { groovyClass ->
+ loader.defineClass(groovyClass.name, groovyClass.bytes)
+ }
+ loader
+ }
+
+ private static void putType(Map<String, String> sources, String pkg,
String simpleName, String state) {
+ sources[pkg + '/Marker'] = "package ${pkg}\nclass Marker {}"
+ if (state == 'p') {
+ sources[pkg + '/' + simpleName] = "package ${pkg}\nclass
${simpleName} {}"
+ } else if (state == 'k') {
+ sources[pkg + '/' + simpleName] = "package
${pkg}\[email protected]\nclass ${simpleName} {}"
+ }
+ }
+
+ private static String useSource(String pkg, String imports, String
simpleName) {
+ """\
+package ${pkg}
+${imports}
+class Use {
+ ${simpleName} field
+ static Class which() { ${simpleName} }
+}
+"""
+ }
+
+ /** First public type in source order. {@code p} public, {@code k}
package-private, {@code m} missing. */
+ private static String firstStar(String a, String b, String c) {
+ if (a == 'p') return 'pa.X'
+ if (b == 'p') return 'pb.X'
+ if (c == 'p') return 'pc.X'
+ return 'unresolved'
+ }
+
+ private static String classifyUse(Map<String, String> sources, String
className, CompilerConfiguration config = null) {
+ // Dependency types are compiled first so package-private modifiers are
+ // already on the bytecode when the importing compilation unit is
resolved.
+ def deps = sources.findAll { name, text -> !name.endsWith('/Use') }
+ def use = sources.findAll { name, text -> name.endsWith('/Use') }
+ try {
+ def parent = deps ? compile(Groovy11916.classLoader, deps) :
Groovy11916.classLoader
+ def loader = compile(parent, use, config)
+ return
loader.loadClass(className).getDeclaredMethod('which').invoke(null).name
+ } catch (Exception e) {
+ String text = ''
+ for (Throwable t = e; t != null; t = t.cause) {
+ if (t.message != null) text += t.message + '\n'
+ }
+ if (text.contains('ambiguous')) return 'ambiguous'
+ if (text.contains('unable to resolve')) return 'unresolved'
+ if (text.contains('not public')) return 'not-public'
+ def snippet = text.readLines().collect { it.trim() }.findAll { it
}.take(4).join(' | ')
+ return 'other:' + (snippet ?: e.getClass().simpleName)
+ }
+ }
+
+ @Test
+ void testStarImportDoesNotResolvePackagePrivateJdkType() {
+ assert !Modifier.isPublic(Class.forName('java.util.TimSort').modifiers)
+
+ def err = shouldFail '''
+ import java.util.*
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+
+ // A bare name in a script is a dynamic property when it is not a type.
+ // It must not bind the inaccessible class (that binding is what throws
+ // IllegalAccessError for a later instanceof / checkcast).
+ err = shouldFail '''
+ import java.util.*
+ assert TimSort.name == 'java.util.TimSort'
+ '''
+ assert err instanceof MissingPropertyException
+ }
+
+ @Test
+ void testDefaultImportDoesNotResolvePackagePrivateJdkType() {
+ def err = shouldFail '''
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+ }
+
+ @Test
+ void testModuleImportDoesNotResolvePackagePrivateJdkType() {
+ def err = shouldFail '''
+ import module java.base
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+ }
+
+ @Test
+ void testStarImportStillResolvesPublicTypes() {
+ assertScript '''
+ import java.util.*
+ assert ArrayList.name == 'java.util.ArrayList'
+ assert List.name == 'java.util.List'
+ assert new ArrayList() instanceof List
+ '''
+ assertScript '''
+ import java.util.Map.*
+ Entry e = [a:1].entrySet().first()
+ assert e.key == 'a'
+ '''
+ assertScript '''
+ import static java.util.Map.*
+ Entry e = [a:1].entrySet().first()
+ assert e instanceof Entry
+ '''
+ assertScript '''
+ import module java.base
+ assert ArrayList.name == 'java.util.ArrayList'
+ assert LocalDate.name == 'java.time.LocalDate'
+ '''
+ }
+
+ @Test
+ void testExplicitReferenceToPackagePrivateTypeStillResolves() {
+ assertScript '''
+ import java.util.TimSort
+ assert TimSort.name == 'java.util.TimSort'
+ '''
+ assertScript '''
+ assert java.util.TimSort.name == 'java.util.TimSort'
+ '''
+ assertScript '''
+ assert Class.forName('java.util.TimSort').name ==
'java.util.TimSort'
+ '''
+ compile(types, [
+ 'q/Direct': '''
+ package q
+ import p.Hidden
+ class Direct {
+ Hidden h
+ }
+ '''
+ ])
+ }
+
+ @Test
+ void testSamePackageStarImportSeesPackagePrivateType() {
+ compile(types, [
+ 'p/Peer': '''
+ package p
+ import p.*
+ class Peer {
+ Hidden h
+ Visible v
+ }
+ '''
+ ])
+ }
+
+ @Test
+ void testOtherPackageStarImportDoesNotSeePackagePrivateType() {
+ def err = shouldFail {
+ compile(types, [
+ 'q/User': '''
+ package q
+ import p.*
+ class User {
+ Hidden h
+ }
+ '''
+ ])
+ }
+ assert err.message.contains('unable to resolve class Hidden')
+
+ compile(types, [
+ 'q/PublicUser': '''
+ package q
+ import p.*
+ class PublicUser {
+ Visible v
+ }
+ '''
+ ])
+ }
+
+ @Test
+ void testStarImportOfPublicMemberOfInaccessibleType() {
+ def err = shouldFail {
+ compile(types, [
+ 'q/InnerUser': '''
+ package q
+ import p.Outer.*
+ class InnerUser {
+ Inner i
+ }
+ '''
+ ])
+ }
+ assert err.message.contains('unable to resolve class Inner')
+
+ compile(types, [
+ 'p/InnerPeer': '''
+ package p
+ import p.Outer.*
+ class InnerPeer {
+ Inner i
+ }
+ '''
+ ])
+ }
+
+ @Test
+ void testProtectedMemberTypeFromSubclass() {
+ compile(types, [
+ 'q/Child': '''
+ package q
+ import p.Protector
+ import p.Protector.*
+ class Child extends Protector {
+ Secret s
+ }
+ '''
+ ])
+ def err = shouldFail {
+ compile(types, [
+ 'q/Stranger': '''
+ package q
+ import p.Protector.*
+ class Stranger {
+ Secret s
+ }
+ '''
+ ])
+ }
+ assert err.message.contains('unable to resolve class Secret')
+ }
+
+ @Test
+ void testPrivateMemberTypeStarImport() {
+ compile(types, [
+ 'q/ShownUser': '''
+ package q
+ import p.Priv.*
+ class ShownUser {
+ Shown s
+ }
+ '''
+ ])
+ compile(types, [
+ 'q/ShownStatic': '''
+ package q
+ import static p.Priv.*
+ class ShownStatic {
+ Shown s
+ }
+ '''
+ ])
+ def err = shouldFail {
+ compile(types, [
+ 'q/SecretUser': '''
+ package q
+ import p.Priv.*
+ class SecretUser {
+ Secret s
+ }
+ '''
+ ])
+ }
+ assert err.message.contains('unable to resolve class Secret')
+ }
+
+ @Test
+ void testInaccessibleStarImportDoesNotHideLaterPublicType() {
+ def loader = compile(types, [
+ 'q/Collision': '''
+ package q
+ class Collision {}
+ ''',
+ 'r/User': '''
+ package r
+ import p.*
+ import q.*
+ class User {
+ static Class which() { Collision }
+ }
+ '''
+ ])
+ assert
loader.loadClass('r.User').getDeclaredMethod('which').invoke(null).name ==
'q.Collision'
+ }
+
+ // GROOVY-11916
+ @Test
+ void testStarImportsBindFirstAccessibleType() {
Review Comment:
this test needs an explanation what it actually tests. As it is, it is hard
to read. Same for testSamePackageTypeShadowsOnDemandImports and
testDeclaredStarShadowsLanguageDefaults
##########
src/test/groovy/bugs/Groovy11916.groovy:
##########
@@ -0,0 +1,727 @@
+/*
+ * 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 bugs
+
+import groovy.lang.GroovyClassLoader
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.Phases
+import org.codehaus.groovy.control.customizers.ImportCustomizer
+import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Modifier
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Star imports, module imports, and default imports resolve only accessible
+ * types. A source import shadows those defaults, including {@code java.lang}.
+ * Two source star imports bind the first accessible type in source order.
+ * An explicit single-type import, a fully qualified name, and
+ * {@code Class.forName} still name a type that is not public.
+ */
+final class Groovy11916 {
+
+ private static GroovyClassLoader types
+
+ @BeforeAll
+ static void compilePackagePrivateTypes() {
+ types = compile(Groovy11916.classLoader, [
+ 'p/Types': '''
+ package p
+
+ @groovy.transform.PackageScope
+ class Hidden {}
+
+ class Visible {}
+
+ @groovy.transform.PackageScope
+ class Outer {
+ public static class Inner {}
+ }
+
+ class Protector {
+ protected static class Secret {}
+ }
+
+ class Priv {
+ private static class Secret {}
+ public static class Shown {}
+ }
+
+ @groovy.transform.PackageScope
+ class Collision {}
+ '''
+ ])
+ assert !Modifier.isPublic(types.loadClass('p.Hidden').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Collision').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Visible').modifiers)
+ assert !Modifier.isPublic(types.loadClass('p.Outer').modifiers)
+ assert Modifier.isPublic(types.loadClass('p.Outer$Inner').modifiers)
+ assert
Modifier.isProtected(types.loadClass('p.Protector$Secret').modifiers)
+ assert Modifier.isPrivate(types.loadClass('p.Priv$Secret').modifiers)
+ }
+
+ private static GroovyClassLoader compile(ClassLoader parent, Map<String,
String> sources, CompilerConfiguration config = null) {
+ def loader = new GroovyClassLoader(parent, config)
+ def unit = new CompilationUnit(config, null, loader)
+ sources.each { name, text -> unit.addSource(name + '.groovy', text) }
+ unit.compile(Phases.CLASS_GENERATION)
+ unit.classes.sort { it.name }.each { groovyClass ->
+ loader.defineClass(groovyClass.name, groovyClass.bytes)
+ }
+ loader
+ }
+
+ private static void putType(Map<String, String> sources, String pkg,
String simpleName, String state) {
+ sources[pkg + '/Marker'] = "package ${pkg}\nclass Marker {}"
+ if (state == 'p') {
+ sources[pkg + '/' + simpleName] = "package ${pkg}\nclass
${simpleName} {}"
+ } else if (state == 'k') {
+ sources[pkg + '/' + simpleName] = "package
${pkg}\[email protected]\nclass ${simpleName} {}"
+ }
+ }
+
+ private static String useSource(String pkg, String imports, String
simpleName) {
+ """\
+package ${pkg}
+${imports}
+class Use {
+ ${simpleName} field
+ static Class which() { ${simpleName} }
+}
+"""
+ }
+
+ /** First public type in source order. {@code p} public, {@code k}
package-private, {@code m} missing. */
+ private static String firstStar(String a, String b, String c) {
+ if (a == 'p') return 'pa.X'
+ if (b == 'p') return 'pb.X'
+ if (c == 'p') return 'pc.X'
+ return 'unresolved'
+ }
+
+ private static String classifyUse(Map<String, String> sources, String
className, CompilerConfiguration config = null) {
+ // Dependency types are compiled first so package-private modifiers are
+ // already on the bytecode when the importing compilation unit is
resolved.
+ def deps = sources.findAll { name, text -> !name.endsWith('/Use') }
+ def use = sources.findAll { name, text -> name.endsWith('/Use') }
+ try {
+ def parent = deps ? compile(Groovy11916.classLoader, deps) :
Groovy11916.classLoader
+ def loader = compile(parent, use, config)
+ return
loader.loadClass(className).getDeclaredMethod('which').invoke(null).name
+ } catch (Exception e) {
+ String text = ''
+ for (Throwable t = e; t != null; t = t.cause) {
+ if (t.message != null) text += t.message + '\n'
+ }
+ if (text.contains('ambiguous')) return 'ambiguous'
+ if (text.contains('unable to resolve')) return 'unresolved'
+ if (text.contains('not public')) return 'not-public'
+ def snippet = text.readLines().collect { it.trim() }.findAll { it
}.take(4).join(' | ')
+ return 'other:' + (snippet ?: e.getClass().simpleName)
+ }
+ }
+
+ @Test
+ void testStarImportDoesNotResolvePackagePrivateJdkType() {
+ assert !Modifier.isPublic(Class.forName('java.util.TimSort').modifiers)
+
+ def err = shouldFail '''
+ import java.util.*
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+
+ // A bare name in a script is a dynamic property when it is not a type.
+ // It must not bind the inaccessible class (that binding is what throws
+ // IllegalAccessError for a later instanceof / checkcast).
+ err = shouldFail '''
+ import java.util.*
+ assert TimSort.name == 'java.util.TimSort'
+ '''
+ assert err instanceof MissingPropertyException
+ }
+
+ @Test
+ void testDefaultImportDoesNotResolvePackagePrivateJdkType() {
+ def err = shouldFail '''
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+ }
+
+ @Test
+ void testModuleImportDoesNotResolvePackagePrivateJdkType() {
+ def err = shouldFail '''
+ import module java.base
+ def seen = null instanceof TimSort
+ '''
+ assert err.message.contains('unable to resolve class TimSort')
+ }
+
+ @Test
+ void testStarImportStillResolvesPublicTypes() {
+ assertScript '''
+ import java.util.*
+ assert ArrayList.name == 'java.util.ArrayList'
+ assert List.name == 'java.util.List'
+ assert new ArrayList() instanceof List
+ '''
+ assertScript '''
+ import java.util.Map.*
+ Entry e = [a:1].entrySet().first()
+ assert e.key == 'a'
+ '''
+ assertScript '''
+ import static java.util.Map.*
+ Entry e = [a:1].entrySet().first()
+ assert e instanceof Entry
+ '''
+ assertScript '''
+ import module java.base
+ assert ArrayList.name == 'java.util.ArrayList'
+ assert LocalDate.name == 'java.time.LocalDate'
+ '''
+ }
+
+ @Test
+ void testExplicitReferenceToPackagePrivateTypeStillResolves() {
+ assertScript '''
+ import java.util.TimSort
+ assert TimSort.name == 'java.util.TimSort'
Review Comment:
if TimSort is not accessible, then why does the script succeed. Note: I am
not asking why it compiles, I am asking why it is executable. Isn't the purpose
of the accessibility check to avoid the later runtime failure? And isn't
TimSort inaccessible?
> Star imports should not resolve package-private types
> -----------------------------------------------------
>
> Key: GROOVY-11916
> URL: https://issues.apache.org/jira/browse/GROOVY-11916
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> Groovy's star import resolution (e.g., import java.util.*) currently includes
> package-private classes in name resolution. On modern JDKs (9+) with the
> module system, these classes cannot actually be used — attempts result in
> IllegalAccessError at runtime. This can lead to confusing errors and
> inconsistency with Java's behaviour where star imports only resolve public
> types.
> For example, import java.util.* allows def cls = TimSort to compile
> (resolving the package-private java.util.TimSort), but any actual usage
> (instanceof, construction, method calls) fails at runtime with
> IllegalAccessError.
> {code:groovy}
> import java.util.*
> assert String.modifiers == 17
> def cls = TimSort // fine
> assert cls.modifiers == 0
> println cls // => class java.util.TimSort
> println Class.forName('java.util.TimSort') // => class java.util.TimSort
> println new Object() instanceof TimSort // IllegalAccessError
> {code}
> Having said that, as a counter example, is it really any different than being
> able to get the class via Class.forName?
> If the Java behavior is deemed correct, the fix would be to filter non-public
> classes during star import resolution in ResolveVisitor, producing a clean
> compile-time error instead of a runtime error. This is technically a breaking
> change but only affects code that was already failing at runtime on modular
> JDKs.
> This also benefits the import module feature (GROOVY-11896), which expands to
> star imports — filtering at the resolution level means module imports
> correctly include only public types, matching Java's import module semantics.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
