[
https://issues.apache.org/jira/browse/GROOVY-11916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18143773#comment-18143773
]
ASF GitHub Bot commented on GROOVY-11916:
-----------------------------------------
daniellansun commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4550640697
##########
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:
Each method now says what it checks.
`testStarImportsBindFirstAccessibleType`: `pa.X`, `pb.X`, and `pc.X` are
missing, public, or package-private, and the use class imports the three stars
in that order. The first public type wins. An inaccessible type does not hide a
later public one. With no public type the name stays unresolved. A repeated
star and the reversed list follow the same rule.
`testSamePackageTypeShadowsOnDemandImports`: the use class is `pa.Use`. When
`pa.X` exists it wins whether it is public or package-private. When it is
missing, the first accessible star is used.
`testDeclaredStarShadowsLanguageDefaults`: the use class names `String`. The
first public `pb.String` or `pc.String` shadows `java.lang.String`. A
package-private one is skipped. An explicit `import java.lang.*` is a source
star too, so its position relative to `pb.*` decides the winner.
> 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)
