[
https://issues.apache.org/jira/browse/GROOVY-11916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18143265#comment-18143265
]
ASF GitHub Bot commented on GROOVY-11916:
-----------------------------------------
daniellansun commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4544238289
##########
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:
Jochen,
You are right about both breaking changes, and the follow-up restores the
older rule.
A declared import shadows the default imports, including `java.lang`.
`import pb.*; class Use { String s }` with a public `pb.String` is `pb.String`
again. It is not ambiguous with `java.lang.String`, and it is not
`java.lang.String`. Making that a tie was a breaking change. The
program-structure page lists `java.lang.*` in the same default-import list as
`java.util.*`, `java.io.*`, `java.net.*`, `java.time.*`, `groovy.lang.*`,
`groovy.util.*`, `BigInteger`, and `BigDecimal`. Those are the imports Groovy
adds so the boilerplate can be left out. A source import shadows that whole
list. `java.lang` is not a special case that ties with a star the user wrote.
That is also why the extra defaults were kept below a source star in the
previous commit, and why they stay there. `import java.sql.*; Date` is
`java.sql.Date`, not `java.util.Date`. `import java.awt.*; List` is
`java.awt.List`. Under the same list, `java.lang` does not get a different rule
from `java.util`.
Two source star imports are first-match in source order again, not an
ambiguity error. `import java.util.*; import java.sql.*; Date` is
`java.util.Date`. The reverse order is `java.sql.Date`. The star-import
section's answer to a cluttered namespace is aliasing. The error the page
actually states is two simple imports of the same name (`import java.awt.List;
import java.util.List`), or a simple import and a top-level type of that name.
This is not the Generics section, so I am not applying the JLS on-demand tie
here.
An `ImportCustomizer` star sits below imports written in the source and
above that language list. A source star or a source single-type import hides
it. With no source import, `star 'java.sql'` still binds `Date` to
`java.sql.Date`, and a customizer star of a public `pb.String` binds
`pb.String`, without an ambiguity error against `java.lang` or `java.util`. It
still does not bind an inaccessible type. A simple import added by the
customizer stays a simple import: it still conflicts with another simple import
of the same name, which is the case `ImportTest` already covers for `Proxy`.
GROOVY-11916 itself stands. A star import, a module import, a default
import, and a customizer star bind only a type that is accessible from the
importing class. `import java.util.*` does not bind `java.util.TimSort`. An
inaccessible type is not a candidate, so it does not hide a later accessible
type and it is not reported as "not public" after being bound. A
package-private type in the same package, a protected member type used from a
subclass, and a private member type used from the enclosing nest stay visible.
An explicit single-type import, a fully qualified name, and `Class.forName`
still resolve `TimSort`. I am not adding an access diagnostic for those forms.
`import static pb.Holder.*` follows the same idea. It shadows the defaults,
so a public static member type `String` or `Date` is the member type, not
`java.lang.String` or `java.util.Date`. Two source static stars use source
order. A package-private static member type is not imported. The static-import
section talks about overloading and about an imported method taking precedence
when the signature matches. It does not make two stars an ambiguity error.
A module import is the star imports of the packages that module exports,
placed at that statement. `import module java.desktop` then `import
java.util.*` keeps `java.awt.List`, because the module import is earlier. The
reverse order keeps `java.util.List`. `import java.util.List` still wins over
the module, which is the priority the documentation states. If one module
exports two public types with the same simple name, the first accessible export
is used rather than a compile error; a single-type import is how you name the
other one. `import module java.base; String` remains `java.lang.String`,
because `java.lang` is already a language default.
The `NamedVariantTransformTest` and Guice tweaks were only there so those
programs would compile under the ambiguity rule. Both are reverted.
`NamedVariantTransformTest` is back to `star 'groovy.transform',
'org.codehaus.groovy.ast'`. `ASTTest` is `groovy.transform.ASTTest` again
because that star is configured first; the other candidate is the test class
`org.codehaus.groovy.ast.ASTTest`. The Guice example says `as Module` again:
`import com.google.inject.*` is a declared star, so `Module` is
`com.google.inject.Module`, not `java.lang.Module`.
The tests that were not on main are in the second commit on this PR,
`4e761587`: `Groovy11916.groovy`, the qualified Guice `Module`, and the extra
`ASTTest` import. This follow-up rewrites the cases that expected a javac-style
tie (a source star against `java.lang`, two source stars, and two static stars)
and restores those two test tweaks. The accessibility tests from the first
commit stay.
> 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)
