[
https://issues.apache.org/jira/browse/GROOVY-11916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141993#comment-18141993
]
ASF GitHub Bot commented on GROOVY-11916:
-----------------------------------------
daniellansun commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4525453204
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -750,7 +750,7 @@ protected boolean resolveFromDefaultImports(final ClassNode
type, final String[]
// WARNING: This code does not expect a class that has a static
// inner class in DEFAULT_IMPORTS
ConstructedClassWithPackage tmp = new
ConstructedClassWithPackage(packagePrefix, typeName);
- if (resolve(tmp, false, false, false)) {
+ if (resolve(tmp, false, false, false) &&
isStarImportAccessible(tmp, null)) {
Review Comment:
`resolve(tmp, false, false, false)` and `isStarImportAccessible` answer
different questions, and both have to stay.
`tmp` is a `ConstructedClassWithPackage` for one default-import prefix plus
the simple name (`java.util.` + `TimSort`, and the same for `java.lang.`,
`java.io.`, and the rest). `resolve` asks whether that binary name exists. For
`java.util.TimSort` it does: `resolveToOuter` loads the class,
`ClassNodeResolver` caches it under the qualified name, and `tmp.setRedirect`
points at that `ClassNode`. `getModifiers()` then follows the redirect, so the
accessibility test sees the real flags, not the `ACC_PUBLIC` on the temporary
node.
A default import is a type-import-on-demand. JLS §7.5.2 imports the
accessible types of the package, and nothing else. `TimSort` is
package-private, so the import must not bind the simple name. The `&&` is that
filter. On failure we do not call `type.setRedirect`, we do not add the prefix
to `DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE`, and the loop tries the next
default package. That is also why a resolved inaccessible type must not count
as success: an earlier default package that only contains an inaccessible type
of that simple name would otherwise hide a public type in a later one.
`Groovy11916.testInaccessibleStarImportDoesNotHideLaterPublicType` is the same
rule for explicit star imports (`p.Collision` exists, is not accessible, and
must not hide `q.Collision`).
`testDefaultImportDoesNotResolvePackagePrivateJdkType` is the default-import
case: the simple name stays unresolved (`unable to resolve class TimSort`),
which is the javac result for a star import that did not import the type.
`resolve` should still return true. Accessibility is not known until the
class is resolved, and this `resolve` is the shared lookup. The single-type
import and the qualified name use it too.
`testExplicitReferenceToPackagePrivateTypeStillResolves` requires `import
java.util.TimSort` and `java.util.TimSort` to find the class. Folding the
access check into `resolve` would make those names fail lookup and report
"unable to resolve class", and it would stop this loop from distinguishing "no
such class" from "present, but not importable on demand". The side effect on a
failed check is `tmp`, which is discarded, and the qualified-name cache entry,
which a later qualified lookup should hit. The one control-flow effect already
in `resolveToOuter` is an `Interrupt` when the candidate is another
`SourceUnit`; that restarts resolution before the access check. It only comes
up if a source file's qualified name is a default-import prefix plus this
simple name, and the restarted lookup then goes through the same `&&`.
So I would keep `resolve(tmp, false, false, false) &&
isStarImportAccessible(tmp, null)`.
One related gap, not this condition: JLS §7.5.1 and §6.6.1 make a
single-type import, or a use of the qualified name, of an inaccessible type a
compile-time error ("not public … cannot be accessed from outside package").
This PR still accepts those names. That wants its own diagnostic at the import
and at the qualified use. It is not a reason for `resolve` to return false
here, and a star or default import that merely fails to import the type should
keep the "unable to resolve" result the tests assert.
> 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)
