blackdrag commented on code in PR #3227:
URL: https://github.com/apache/groovy/pull/3227#discussion_r4542807437
##########
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:
> package pb;
> public class String {}
>
> 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.
Meaning this is a breaking change, and going away from the old guiding
principle: declared imports shadow default imports.
> 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:
>
> import java.sql.*;
> class Use { Date d; } // javac: java.sql.Date
>
> import java.awt.*;
> class Use { List x; } // javac: java.awt.List
why do we special case them? Do we want to comply with the JLS or with how
javac behaves? What about custom default star imports?
> 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:
>
> import java.util.*;
> import java.sql.*;
> class Use { Date d; } // javac: ambiguous, either order
current Groovy uses the first import to resolve Date. Another breaking
change.
> The cases above are `testExplicitStarImportMatrixMatchesJavac`,
`testSamePackageTypeShadowsOnDemandImports`,
`testJavaLangOnDemandTiesWithExplicitStars`,
`testStaticOnDemandImportsAreAmbiguous`,
`testExtraDefaultImportsDoNotTieWithExplicitStars`,
`testSingleTypeImportShadowsOnDemand` and `testJavaLangShadowsModuleImports`.
`NamedVariantTransformTest` and `testSingletonGuice` show case breaking
changes. The tests you name are not part of this PR. And at least
`testExplicitStarImportMatrixMatchesJavac` is not in main as far as I can tell.
I did not check the others
--
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]
