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.
--
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]
