[
https://issues.apache.org/jira/browse/GROOVY-12569?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135274#comment-18135274
]
ASF GitHub Bot commented on GROOVY-12569:
-----------------------------------------
codeconsole commented on PR #3098:
URL: https://github.com/apache/groovy/pull/3098#issuecomment-6527469623
@daniellansun
1. CI: the push of these commits starts a new run.
2. Coverage. Codecov's per-line data for b0aa14e21d: 34 of the 53 lines are
this PR's; the other 19 are #3097's, which Codecov counts here because the PR
is compared with master. 62754d72c9 adds tests for the branches that can be
reached, and 45f47d8a27 removes the ones that cannot. Line numbers are from
b0aa14e21d.
New tests in `StaticCompileRuntimePackageTest` use a fixture with the
public `Open`, the package-private `Shut`, `Shut2 extends Shut`, `Plain` and
the interface `Secret`, and the public `Pub extends Shut`.
`testPackagePrivateClassesOfAnotherClassLoaderAreUsedThroughPublicClassesAsInDynamicCode`
checks that dynamic and `@CompileStatic` code give the same results, and
`testPackagePrivateClassesOfAnotherClassLoaderAreNotNamedInBytecodeOfUsesThroughPublicClasses`
that no instruction names `p/Shut`, `p/Plain` or `p/Secret`:
- `AsmClassGenerator.getDirectlyAccessibleSuperClass` 1387/1388/1390:
`new Shut2()`, whose superclass `Shut` is hidden too, steps to `Open`;
`Open.secret()` returns the interface `Secret`, which has no superclass, so
`Object`.
- `StaticInvocationWriter.findDirectlyAccessibleDeclaration` 273/275:
through `Shut2` the lookup passes the hidden `Shut`; `s.prot()` finds the
protected `Open.prot()`, so the call is made at run time. 442/445/446:
`s.take(s)` finds the public `Open.take(Shut)`, whose parameter type is hidden,
so at run time; `s.take('x')` stays `INVOKEVIRTUAL p/Open.take`. 773:
`list*.@pf`.
- `StaticTypesCallSiteWriter` 555/920: `Shut.SF` and `Shut.@SF = 'w'`
give `GETSTATIC`/`PUTSTATIC p/Open.SF`; 911: `s.@pf = 'q'` gives `PUTFIELD
p/Open.pf`; 899/900: `s.@peer = s` sets a field typed `Shut`, so at run time.
- `StaticTypesMethodReferenceExpressionWriter` 159/162/166/167/165/168:
`Open::toString` as `Function<Shut, String>`, `Open::make` as `Supplier<Shut>`
and `Supplier<Object>`, `Open::take` with the `take(Shut)` overload, and
`Pub::own`, which the hidden `Shut` declares.
- The `!method.isStatic()` side of 275 needs a Java class, since Groovy
rejects an instance method named like a static interface method: in
`testPackagePrivateJavaClassOfAnotherClassLoaderWithInstanceMethodNamedAsStaticMethodOfInterface`,
`Q implements Function` declares an instance `identity()`; without the check,
`q.identity()` compiles to the static `Function.identity()`.
Removed in 45f47d8a27:
- `StaticInvocationWriter.writeInvokeConstructor` 141: the
`DYNAMIC_RESOLUTION` branch and the marker `ConstructorCallTransformer` set for
it. The map-style call already reaches the
`!isClassDirectlyAccessible(call.getType())` branch below, since its
`DIRECT_METHOD_CALL_TARGET` is the generated map constructor;
`testMapStyleConstructionOfPackagePrivateClassOfAnotherClassLoader` still
passes.
- 241: `BytecodeHelper.doCast` already skips `Object`. 254: no caller
passes `null`.
- 490: `written` is always true there, and `getStackLength() > 0` tested
the wrong thing, since the stack can hold other operands; it now compares the
depth before and after the call, which also covers an elided `void` result such
as `h.run()`.
- `StaticTypesCallSiteWriter.makeGetField` 601: the boxed type of a safe
primitive read goes into `valueType`, so the ternary is gone; a new `s?.n`
reads a public `int` field.
- `MethodPointerExpressionWriter` 87 and
`StaticTypesMethodReferenceExpressionWriter.inferredType` 177–182: the
fallbacks to the accessed variable's inferred or origin type never found a type
(178 and 182 never ran in CI); a receiver of a hidden class always carries
`INFERRED_TYPE` itself.
- `StaticTypesTypeChooser` 58: `getTarget` of a `VariableExpression` is
always a `Variable`.
Partial on purpose:
- `StaticTypesTypeChooser.resolveType` 43: the `type != null && current
!= null` checks stay (a44e7610b2). No caller in the compiler passes `null`, but
the method is public and a type checking extension can call it.
- `AsmClassGenerator` 1317: the missing branch is the existing `field ==
null` guard of the public `isFieldDirectlyAccessible`; all three callers pass a
field. The new `isClassDirectlyAccessible` disjunct is covered both ways:
without it, 4 tests fail.
- `StaticTypesCallSiteWriter` 258: `isFieldOfAnotherRuntimePackage` runs
only after `makeGetField` refused a direct read, which never happens for a
public field of a class the bytecode can name or for a field of the same
run-time package.
With JaCoCo over the `sc`, `stc` and `classgen.asm` test packages (4,808
tests), every line this PR adds is covered apart from those. The three new
tests fail without the change (`IllegalAccessError` for `p.Shut2` and for the
Java `p.Q`, and a `CHECKCAST p/Shut`). The Checkstyle and CodeNarc gates pass.
> SC: a package-private class of another GroovyClassLoader compilation fails
> with IllegalAccessError where dynamic code works
> ---------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12569
> URL: https://issues.apache.org/jira/browse/GROOVY-12569
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> A class that is not public is accessible only within its run-time package
> (JVMS 5.4.4), and the JVM checks that wherever an instruction names the
> class. Dynamic code uses a package-private class of an earlier
> {{GroovyShell}} parse, or of a library used by a script, without naming it.
> Statically compiled code names it and fails:
> {code:groovy}
> def shell = new GroovyShell()
> shell.evaluate '''package p
> @groovy.transform.PackageScope class Hidden implements Runnable {
> static String sm() { 'sm' }
> String name
> String own() { 'own' }
> void run() { }
> }
> true
> '''
> shell.evaluate '''package p
> @groovy.transform.CompileStatic
> def test() {
> def h = new Hidden(name: 'x')
> h.run()
> Runnable r = h
> [Hidden.sm(), h.own(), h.name]
> }
> test()
> ''' // each line fails with IllegalAccessError; without @CompileStatic it
> works
> {code}
> Expected: as in dynamic code, {{new}} and static members go through the
> meta-class, a method is called through a public superclass or interface that
> declares it (else the meta-class), and a value of the class is held as its
> nearest public superclass. A variable declared with the class, a cast to it
> and {{instanceof}} still fail, as in dynamic code. Classes compiled to class
> files are used directly.
> PR: https://github.com/apache/groovy/pull/3098
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
