-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix isJvmAccessible to handle nested protected Java classes #24625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes scala#24507 The previous implementation of `isJvmAccessible` failed to recognize protected Java nested classes as accessible. And it causes compilation errors when extending them from subclasses. Previous implementation: ```scala def isJvmAccessible(cls: Symbol): Boolean = !cls.is(Flags.JavaDefined) || { val boundary = cls.accessBoundary(cls.owner)(using preErasureCtx) (boundary eq defn.RootClass) || (ctx.owner.enclosingPackageClass eq boundary) } ``` For protected nested classes like B below, the access boundary is the enclosing class (`A` in this case), not a `package a` or root. However, `B` should be accessible through a public class `A`. ```java package a; public class A { protected class B {} } ``` This commit replace the manual boundary checks with `cls.isAccessibleFrom`, which properly handles all Java access modifiers. This aligns with the Scala 2 implementation which uses the equivalent `context.isAccessible(cls, cls.owner.thisType)`. https://github.com/scala/scala/blob/efb71845113639bd1da231c917e18af33519fc07/src/compiler/scala/tools/nsc/transform/Erasure.scala#L1451-L1455 For example, - `cls` will be `protected class B` - `cls.owner.thisType` will be `public class A` - And, `cls.isAccessibleFrom(cls.owner.thisType)` is true. On the other hand, `isJvmAccessible` returns false for Java-defined package protected class case like `java-package-protected` case. https://github.com/tanishiking/scala3/tree/b2850be4c79996d8ebd9afaf36ed12f46febb0d1/tests/run/java-package-protected In this case, - `cls` = `class B` (package protected under `a`) - `cls.owner.thisType` = `package a` - And, `cls.isAccessibleFrom(cls.owner.thisType)` is false. because `B` is package protected and, we cannot access B through `a.B` from another package. In this case, cast to `A` happens. --- Note that, this regression starts from e7d479f but the true commit is scala@d3ce551 but the reason why we stopped using `isAccessibleFrom` seems to be an optimization and the `tests/pos/trait-access` still compiles with this change.
|
@lrytz Do you mind taking a look? 🙏 |
|
LGTM, thank you! But could you add the test from scala/scala#6023? |
|
Oh, also I think it would be better to make |
|
Thanks @lrytz! Made i24507 |
lrytz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fixes #24507
The previous implementation of
isJvmAccessiblefailed torecognize protected Java nested classes as accessible.
And it causes compilation errors when extending them from subclasses.
Previous implementation:
For protected nested classes like B below, the access boundary is the enclosing class (
Ain this case), not apackage aor root.However,
Bshould be accessible through a public classA.This commit replace the manual boundary checks with
cls.isAccessibleFrom, which properly handles all Java access modifiers.This aligns with the Scala 2 implementation which uses the
equivalent
context.isAccessible(cls, cls.owner.thisType).https://github.com/scala/scala/blob/efb71845113639bd1da231c917e18af33519fc07/src/compiler/scala/tools/nsc/transform/Erasure.scala#L1451-L1455
For example,
clswill beprotected class Bcls.owner.thisTypewill bepublic class Acls.isAccessibleFrom(cls.owner.thisType)is true.On the other hand,
isJvmAccessiblereturns false for Java-defined package protected class case likejava-package-protectedcase.https://github.com/tanishiking/scala3/tree/b2850be4c79996d8ebd9afaf36ed12f46febb0d1/tests/run/java-package-protected
In this case,
cls=class B(package protected undera)cls.owner.thisType=package acls.isAccessibleFrom(cls.owner.thisType)is false.because
Bis package protected and, we cannot access B througha.Bfrom another package. In this case, cast toAhappens.Note that, this regression starts from e7d479f but the true commit is d3ce551
but the reason why we stopped using
isAccessibleFromseems to be an optimization and thetests/pos/trait-accessstill compiles with this change.