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 goe
s 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
