[
https://issues.apache.org/jira/browse/GROOVY-12484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133432#comment-18133432
]
ASF GitHub Bot commented on GROOVY-12484:
-----------------------------------------
paulk-asert commented on PR #3019:
URL: https://github.com/apache/groovy/pull/3019#issuecomment-6462624536
Thanks Jochen. You're right that STC doesn't support an extension or DGM
method acting as a constructor, so nothing stops the constructor check from
coming earlier. I've checked the code and the runtime:
- `findMethod` adds extension methods only when the name isn't `<init>`.
- Dynamic Groovy never treats a DGM or category method as a constructor.
`MetaClassImpl.invokeConstructor` chooses only among the class's own
constructors.
- The one runtime way to add a constructor is ExpandoMetaClass
(`Foo.metaClass.constructor = { Integer i -> … }`). It works for dynamic calls,
but STC doesn't see it either and reports "Cannot find matching constructor".
So the current reworked version moves the constructor check ahead of lookup.
For an inaccessible type, STC now reports "Cannot access constructor of class:
T" and skips lookup altogether. It still checks the arguments the same way it
does when no constructor matches. A new test covers `new p.Outer.Impl('x')`.
Before the move, that reported "Cannot find matching constructor", which
pointed at the wrong problem.
For method calls, I don't think the check can go inside `findMethod`. What
has to be accessible is the type written at the call site, not the class that
declares the method. `findMethod` only gets `(receiver, name, args)`, so it
can't tell `T.m()` from a call on an expression whose type happens to be `T`.
Filtering by declaring class would reject `p.Pub.s()` (row 4 below), which is
legal in Java and in Groovy. Filtering by receiver type would drop every
candidate at once, so you'd get the same rejection with a "Cannot find matching
method" message, which is worse when the method plainly exists. That's why the
method check stays on the `ClassExpression` branch.
Fixtures (all Java, precompiled): in package `p`, `Base` and `Impl` are
package-private, `Pub` is public, `Pub extends Base`, `Impl extends Pub`, and
`Outer.makeImpl()` returns an `Impl`. Every call is made from another package.
| # | Call from another package | Java (javac) | Groovy dynamic |
`@CompileStatic` before PR | `@CompileStatic` with PR |
|---|---|---|---|---|---|
| 1 | `new p.Impl()` | error: Impl is not public | OK | compiles, then
`IllegalAccessError` | error: Cannot access constructor |
| 2 | `new p.Impl("x")` (no matching constructor) | error: Impl is not
public | `GroovyRuntimeException` (no matching constructor) | error: Cannot
find matching constructor | error: Cannot access constructor |
| 3 | `p.Impl.hello()` | error: Impl is not public | OK | compiles, then
`IllegalAccessError` | error: Cannot access method |
| 4 | `p.Pub.s()` (`s` declared in package-private `Base`) | OK | OK | OK |
OK |
| 5 | `p.Pub.make().i()` (`i` declared in `Base`) | OK | OK | OK | OK |
| 6 | `p.Impl.class.getSimpleName()` / `p.Impl.getSimpleName()` | error:
Impl is not public | OK | OK | OK (`Class` methods exempt) |
| 7 | `p.Outer.makeImpl().own()` (`own` declared in `Impl`) | error: defined
in an inaccessible class | OK | compiles, then `IllegalAccessError` | compiles,
then `IllegalAccessError` (unchanged) |
| 8 | `p.Outer.makeImpl().toString()` | error: defined in an inaccessible
class | OK | OK | OK |
| 9 | constructor added with `metaClass.constructor` | n/a | OK | error:
Cannot find matching constructor | unchanged |
Rows 4 and 5 are why the check has to use the type written at the call site
rather than the declaring class. Rows 1–3 are the cases the ticket is about.
Row 6 is a deliberate difference from Java, because Groovy lets you use an
inaccessible class as a `Class` value.
Rows 7 and 8 are the case Daniel asked about, a call on an expression whose
type is inaccessible. Java rejects both, even `toString()`. Groovy shouldn't
copy that: row 8 works at runtime because the generated bytecode names
`Object`, not `Impl`. Row 7 still compiles and then fails at runtime, because
the bytecode names `Impl`. That's where your "select only from accessible
candidates" idea fits, since there it really is a property of each candidate
method. I'd rather handle it in a follow-up ticket than widen this PR.
> STC: method and constructor calls on an inaccessible type compile and fail at
> runtime with IllegalAccessError
> -------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12484
> URL: https://issues.apache.org/jira/browse/GROOVY-12484
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Reporter: Paul King
> Priority: Major
>
> h3. Summary
> GROOVY-12310 (fixed via GROOVY-12314 in 6.0.0) made the type checker reject
> *field* access on a type that is not accessible from the calling class, e.g.
> a public enum constant of a package-private nested enum. The same check is
> missing for method calls and constructor calls: {{@CompileStatic}} code that
> calls a public static method, or a public constructor, of a package-private
> type compiles and then fails at runtime with {{IllegalAccessError}}.
> Java rejects all of these at compile time ("X is not public in Y; cannot be
> accessed from outside package").
> h3. Reproducer
> Compile with javac:
> {code:java}
> package a;
> public class A {
> enum Keys { X, Y }
> static class Impl {
> public Impl() {}
> public static String hello() { return "hello"; }
> }
> }
> {code}
> {code:java}
> package a;
> class B {
> public static String greet() { return "greet"; }
> }
> {code}
> Then, from another package:
> {code:groovy}
> import a.A
> @groovy.transform.CompileStatic
> class Baz {
> def run1() { A.Keys.values() }
> def run2() { A.Keys.valueOf('X') }
> def run3() { A.Impl.hello() }
> def run4() { new A.Impl() }
> def run5() { a.B.greet() }
> }
> def b = new Baz()
> ['run1', 'run2', 'run3', 'run4', 'run5'].each { m ->
> try { println "$m: ${b."$m"()}" } catch (Throwable t) { println "$m: $t" }
> }
> {code}
> h3. Actual
> All five compile. At runtime each throws:
> {noformat}
> java.lang.IllegalAccessError: failed to access class a.A$Keys from class Baz
> java.lang.IllegalAccessError: failed to access class a.A$Impl from class Baz
> java.lang.IllegalAccessError: failed to access class a.B from class Baz
> {noformat}
> For comparison, {{A.Keys.X}} is rejected at compile time since 6.0.0 ("Cannot
> access f from class: Baz"), and a package-private *constructor* is already
> rejected ("Cannot
> find matching constructor"), because member modifiers are checked. Only the
> declaring t.
> h3. Expected
>
> A static type checking error for each call,
> consistent with the field case, e.g. "Cannf class: a.A$Keys from class: Baz"
> / "Cannot access constructor ...". Without{{@CompileStatic}} the calls should
> keep working as they do today.
> h3. Notes
> * {{StaticTypeCheckingVisitor.isFieldAccessible}} applies
> {{hasAccessToClass}} (JLS 6.ssible type is out of reach regardless of its own
> modifiers). Method selection and
> constructor selection only apply {{hasAccessToMember}} to the member's
> modifiers, so te.
> * The class literal ({{A.Keys}}) and subscript access ({{A.Keys['X']}}) are
> deliberate12310's tests and remain the escape hatch; they go through
> {{Class.forName}} and the
> MOP, which perform no access check.
> * Two things to decide: whether to also cover instance method calls on a vale
> inaccessible class (e.g. a value returned as A.Impl from a public API — javac
> rejects naming the type at all, so this follows from the same rule), and the
> exact message wording to match the existing "Cannot access field" one.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
