[
https://issues.apache.org/jira/browse/GROOVY-11916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18142684#comment-18142684
]
ASF GitHub Bot commented on GROOVY-11916:
-----------------------------------------
daniellansun commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4532576799
##########
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:
Not that table. `(not) accessible` has to mean the type is present, and `-`
that it is absent, because the result column then says "error if not
accessible". Read that way, every row claims one result whether the type is
public or not, and the chosen type is bound before accessibility is checked.
javac does not do that. An inaccessible type is not imported by a star import,
so it is not a candidate, it does not participate in an ambiguity, and it is
not bound and then rejected.
JLS 7.5.2 imports the accessible types of the named package or type, and
nothing else. The implicit `import java.lang.*` (JLS 7.3) is a
type-import-on-demand of that same kind. Static-import-on-demand (JLS 7.5.4)
imports the accessible static member types. None of these declarations shadows
another type-import-on-demand (JLS 6.4.1); a module import is the thing they do
shadow. A simple name must then denote exactly one type in scope (JLS 6.5.5.1).
Two or more accessible types imported on demand is a compile-time error. Zero
is "cannot find symbol", including when the only types of that name are
inaccessible. The "not public … cannot be accessed from outside package"
diagnostic is the qualified name and the single-type import (JLS 6.6.1,
6.5.5.2, 7.5.1), not a failed star import.
So for `import a.*; import b.*; import c.*`, with the importing class in
none of those packages, the result depends only on how many of `a.X`, `b.X` and
`c.X` are accessible. Package-private counts as absent. Order does not matter.
```java
package user;
import pa.*;
import pb.*;
class Use { X x; }
```
`pa.X` and `pb.X` both public: javac reports `reference to X is ambiguous`.
One public and one package-private: the public type. Both package-private, or
neither present: `cannot find symbol`. The same holds for three packages. I
checked all 27 combinations of missing / public / package-private on JDK 21;
JDK 25 is not what I compiled with, and this JDK 21 javac does not accept
`import module` at all.
That is not row 1 or row 2. Those rows report an ambiguity for `(not)
accessible`, so they would also report one when every type is package-private,
and they would report one when only one of them is public. javac reports an
ambiguity only when two or more are actually accessible.
Row 3 is the one I do not follow even for the public/public case, when `a`
is the implicit `java.lang` import. `c` does not win.
```java
package pb;
public class String {}
```
```java
package user;
import pb.*;
class Use { String s; }
```
javac: `reference to String is ambiguous, both class pb.String in pb and
class java.lang.String in java.lang match`. An explicit `import java.lang.*` is
redundant with the implicit one (JLS 7.5.2) and does not change that. If
`pb.String` is package-private, the star imports nothing and the name is
`java.lang.String`. There is no access error on `pb.String`. Row 3's "resolved
to c.X, error if not accessible" is the opposite on both counts: the accessible
`java.lang` type is not hidden, and the inaccessible one is not bound.
Row 4 has the same split. `b.X` public and `c` absent is `b.X` when `a`
contributes no accessible type. It is an ambiguity when `a` is `java.lang` and
the simple name is a public `java.lang` type that `b` also declares. It is not
an access error when `b.X` is package-private.
Row 5 is right only for an accessible type: the name resolves to it. For an
inaccessible type the name does not resolve. `import java.util.*;` plus
`TimSort` is `cannot find symbol` on javac, and `unable to resolve class
TimSort` here (`testStarImportDoesNotResolvePackagePrivateJdkType`,
`testDefaultImportDoesNotResolvePackagePrivateJdkType`). I am not going to turn
that into "not public … cannot be accessed". That wording is what javac uses
for `import java.util.TimSort` and for `java.util.TimSort`, which this PR still
accepts (`testExplicitReferenceToPackagePrivateTypeStillResolves`). That is JLS
7.5.1 / 6.6.1, and it is still a separate diagnostic. A star import that merely
fails to import the type must not start reporting it.
Two further priorities, neither of which is a row in the table:
A type declared in the importing package, public or package-private, is
already in scope and is not shadowed by an on-demand import. `package pa;
import pb.*; class Use { X x; }` is `pa.X` even when `pa.X` is package-private
and `pb.X` is public. `import pa.*` from inside `pa` is ignored (JLS 7.5.2).
A single-type import shadows on-demand imports (JLS 6.4.1). `import pb.X;
import pc.*;` is `pb.X` even though `pc.X` is also public.
Groovy's other default imports (`java.util`, `java.io`, `java.net`,
`java.time`, `groovy.lang`, `groovy.util`, plus `BigInteger` and `BigDecimal`)
are not the JLS implicit import. Putting them in the same tier as a star the
user wrote would reject programs javac accepts:
```java
import java.sql.*;
class Use { Date d; } // javac: java.sql.Date
import java.awt.*;
class Use { List x; } // javac: java.awt.List
```
Both are one accessible type-import-on-demand. `java.util.Date` and
`java.util.List` are not in scope in Java. They stay below explicit stars here,
so those two programs keep the javac result. They also stay below module
imports: `import module java.desktop` binds `List` to `java.awt.List`, and an
explicit `import java.util.*` shadows that, which is the JLS 6.4.1 order
`ImportTest` already locks. What does become ambiguous is two stars the user
actually wrote:
```java
import java.util.*;
import java.sql.*;
class Use { Date d; } // javac: ambiguous, either order
```
The loop you commented on returned at the first accessible star, so `pa`
then `pb` both public bound `pa.X`, and the reverse import order bound the
other one. That is not an ambiguity, and it is not "last wins" either.
Accessible on-demand types are now collected, including `java.lang` and static
member types from `import static p.Holder.*` / `import p.Holder.*`. One
distinct type is bound. Two is `reference to X is ambiguous, both class … and …
match`. An inaccessible type is left out of that set, which is what the `&&
isStarImportAccessible(...)` is for, and it is why an earlier package-private
type still cannot hide a later public one
(`testInaccessibleStarImportDoesNotHideLaterPublicType`).
The cases above are `testExplicitStarImportMatrixMatchesJavac`,
`testSamePackageTypeShadowsOnDemandImports`,
`testJavaLangOnDemandTiesWithExplicitStars`,
`testStaticOnDemandImportsAreAmbiguous`,
`testExtraDefaultImportsDoNotTieWithExplicitStars`,
`testSingleTypeImportShadowsOnDemand` and `testJavaLangShadowsModuleImports`.
> 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)
