matrei commented on PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#issuecomment-5665462955
# Review Findings (round 6)
Head `c8756534f2` (*Decide the static-compilation mode per body, not per
class*), one commit since the previous round, base `8.0.x`, merge-base clean.
It resolves the round-5 finding and both of its non-blocking notes:
`staticsInReach(MethodNode, boolean)` reads a method's own `@CompileStatic`
through the same `isTypeCheckingSkipped` the class level uses, the walk moved
into `walkBody` so each body carries its own reachable set, the `endsWith`
choice is explained in place, and the Javadoc and guide now say "or in a method
it marks `@CompileStatic` itself". All four new spec rows pass and I
re-measured each against the runtime with the check bypassed: the
`@CompileDynamic` method on a static host really fails, and the
`@CompileStatic` method on a dynamic host really returns `hello`. No blocking
findings. One residual gap in the same family, measured on both host kinds, and
it is one level deeper than the shape this commit fixed.
## New Findings
### [P3] A nested anonymous class takes the class-level answer even when its
enclosing method is `@CompileDynamic`
**File:**
`grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:1582-1584,1608-1610`
The per-body decision stops at the first anonymous class. An anonymous class
created *inside* a method body is queued on `nested` and then re-entered with
`classOwn` as `enclosingReachable`, and its own `classStaticsInReach` is
recomputed from `isStaticallyCompiled(inner.getOuterClass())`, which walks the
outer *class* chain and never sees the method the class was written in.
Groovy's static compiler visits an anonymous class from the constructor-call
site inside the enclosing method's visit, so a `@CompileDynamic` method takes
every anonymous class in its body with it. Measured on the pushed head, then
with the sibling and group reach checks bypassed:
| Host | Anonymous method | Nested anonymous class calls | Check says | With
check bypassed |
|---|---|---|---|---|
| `@CompileStatic` | `@CompileDynamic String greet() { new G() { String
greet() { helper() } }.greet() }` | `helper()` | compiles clean |
`NoSuchFieldError: Class PgGGrailsPlugin$1 does not have member field
'PgGGrailsPlugin this$0'` |
| `@CompileStatic`, in a `group('extras')` | same | `helper()` | compiles
clean | same `NoSuchFieldError` on `PgLGrailsPlugin$1` |
| `@CompileStatic` | plain `String greet() { ... }` | `helper()` | compiles
clean | `greet()` returns `hello` |
| dynamic | `@CompileStatic String greet() { ... }` | `helper()` |
static-member message | `NoSuchFieldError` |
| dynamic | plain | `helper()` | static-member message | `NoSuchFieldError` |
The first two rows are the runtime failure the check exists to catch.
`javap` on the compiled classes shows why: under the `@CompileDynamic` method
the nested class's `greet()` is `invokedynamic ... invoke` with a
`methodMissing` that reads `this$0`, while under the plain method it is
`invokestatic PgHGrailsPlugin.helper`. The other three rows agree with the
runtime, including the fourth, so a method-level `@CompileStatic` on a dynamic
host does *not* extend to an anonymous class created inside it. The rule that
matches every measured row is: a nested anonymous class reaches the enclosing
statics only when the class chain says so **and** every enclosing body says so.
Two things have to change together for the nested class to get that answer,
because `helper` currently reaches it by both routes:
1. Pass the body's own set, not `classOwn`, as `enclosingReachable` when
re-entering for a class nested in that body. With the class-level
`ownWithStatics` passed down, `helper` is already in reach before the nested
class's own answer is consulted.
2. Let the caller override the nested class's `classStaticsInReach`.
`nested` could carry the body's answer next to each call (a small record, or
two parallel lists), and `reportOutwardReferences` could take a `Boolean` that
the two top-level callers pass as `null` (compute from the chain, as now) and
the recursive call passes as `classStaticsInReach && bodyStaticsInReach`.
Field initializers and object-initializer statements of the nested class
keep the class answer, as they do today. The `@CompileDynamic` method of a
`@CompileStatic` host is already the shape someone reaches for when a body will
not type-check, and an anonymous callback inside it is not exotic. Two spec
rows would pin it: the sibling and group shapes in the first two rows above,
expecting the static-member message; the third row could join `"the
static-compilation mode is read per body, so #shape reaches the enclosing
statics"` as a control.
## Notes (not blocking)
-
`grails-beans-dsl/src/main/java/grails/compiler/beans/GrailsBeans.java:228` is
a 58-column line inside a paragraph that wraps at 98-101; a reflow of that
sentence would keep the block even. Cosmetic.
## Verified Correct
- **Every spelling of the per-method rule agrees with the runtime.** Beyond
the four rows the spec pins, I measured `@GrailsCompileStatic` on the method of
a dynamic host (compiles, `hello`), `@CompileStatic(TypeCheckingMode.PASS)` on
the method of a dynamic host (compiles, `hello`),
`@CompileStatic(TypeCheckingMode.SKIP)` on the method of a static host
(static-member message; bypassed, `NoSuchFieldError`), `@TypeChecked` on the
method of a dynamic host (static-member message; bypassed, `NoSuchFieldError`,
since `@TypeChecked` leaves the bytecode dynamic and `staticsInReach` rightly
ignores it), and a `@CompileStatic(TypeCheckingMode.SKIP)` host with a
`@CompileStatic` method (compiles, `hello`, so the method's own annotation wins
over a skipped class in the compiler and in the check alike).
- **The group path follows the per-body rule too.** A dynamic host with a
`@CompileStatic` method inside `group('extras')` compiles and returns `hello`;
a `@CompileStatic` host with a `@CompileDynamic` method inside the group is
rejected with the static-member sentence, and with the check bypassed fails
with `NoSuchFieldError`. The spec pins only the sibling path, so a group row
for each direction would keep the two from drifting, but nothing drifts today.
- **`walkBody` is a faithful extraction.** The visitor body is byte-for-byte
the previous one; `nested` is still collected across every body and re-entered
after the walk, and `visited` still guards re-entry, so the traversal order and
the `answersAnything` short-circuit are unchanged.
- **`staticsInReach(MethodNode, boolean)`** reads `annotations.get(0)` on a
non-repeatable annotation, so there is exactly one; the fallback to
`classAnswer` is the pre-commit behaviour for unannotated methods and
field/initializer bodies, which the `"an unannotated method of a static host"`
and `"an unannotated method of a dynamic host"` rows pin in both directions.
- **Docs.** The Javadoc and the guide NOTE state the same condition the code
applies, and the endsWith comment at `:1806-1808` says what round 5 asked for.
## Verification
- Read the full diff of `c8756534f2` (4 files, +114/-26) and re-read
`reportOutwardReferences`, `staticsInReach`, `walkBody`, `isStaticallyCompiled`
and `isTypeCheckingSkipped` in the current file.
- Ran `./gradlew :grails-beans-dsl:check --continue` on the branch as
pushed: 397 tests, 0 failures, 0 errors, 0 skipped; module Checkstyle reports 0
violations across 5 files.
- Wrote a throwaway spec with twelve shapes (the eleven listed above plus
the group nested row) that prints, for each, whether the check rejects and, if
not, what `greet()` does at runtime, and ran it twice: on the pushed head, and
with an early `return` at the top of `rejectAnonymousClassReachingOutward`.
Compiled the three nested sibling shapes to a scratch directory and read
`greet()` with `javap -p -c`. Results are the tables and bullets above. The
patch was reverted and the spec deleted; `git status` shows no tracked changes.
## What I Did Not Run
- `./gradlew clean aggregateViolations :grails-test-report:check --continue`
from the root (CLAUDE.md rule #12), so CodeNarc, PMD and SpotBugs on the new
code are unverified here.
- `./gradlew :grails-core:test` - nothing outside `grails-beans-dsl` and
`grails-doc` changed in this round.
--
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]
