matrei commented on PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#issuecomment-5655308577
# Review Findings (round 5)
Head `74f7951df6` (*Read the receiver, and read what @CompileStatic was
given*), one commit since the previous round, base `8.0.x`. It resolves both
findings from that round and the receiver-aware extension set I listed as
optional: `isStaticallyCompiled` now reads the annotation's `value`, the DGM
names are keyed by receiver type and matched against the anonymous class's
superclass chain and every interface along it, and the three `answersAnything`
cases plus the `@CompileStatic` group shape are pinned. I re-measured the
`TypeCheckingMode.SKIP` host and it is reported; a moved `getText` read as
`text` is reported; `join` on an `ArrayList` subclass and on an `Iterable`-only
supertype both compile and run. No blocking findings. One residual gap in the
same family as last round's, with both directions measured.
## New Findings
### [P3] The static-compilation mode is read from the class, and a
method-level annotation inside the anonymous class overrides it in both
directions
**File:**
`grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:1571,1588,1764`
`staticsInReach` is decided once per anonymous class from the outer chain's
class-level annotation, and `own` is then shared by every body the walk visits.
Groovy's static type checker honours `@CompileStatic` / `@CompileDynamic` on a
*method* of an inner class regardless of what the enclosing classes say
(`StaticTypeCheckingVisitor.visitMethod` checks `isSkipMode` on the method
itself), so the class-level answer is wrong for a method annotated the other
way. Measured on the pushed head, then again with the sibling reach check
bypassed:
| Host | Anonymous method | Check says | With check bypassed |
|---|---|---|---|
| `@CompileStatic` | `@CompileDynamic String greet() { helper() }` |
compiles clean | `NoSuchFieldError: Class FGrailsPlugin$1 does not have member
field 'FGrailsPlugin this$0'` |
| dynamic | `@CompileStatic String greet() { helper() }` | `"helper()" does
not resolve ... static member of an enclosing class` | compiles, `greet()`
returns `hello` |
The first row is the runtime failure the check exists to catch; the second
row is working code refused, which the PR's own principle (commit `0c25aefef2`)
treats as the worse of the two. Both shapes are rare - a single method marked
dynamic inside a `@CompileStatic` descriptor is the more plausible one, and it
is exactly the shape someone reaches for when a body will not type-check.
Suggested fix: decide per body rather than per class. In
`reportOutwardReferences`, when collecting `bodies` from `inner.getMethods()`,
look for `@CompileStatic` on the method first and only fall back to
`isStaticallyCompiled(inner.getOuterClass())` when the method carries none;
give a method whose answer differs from the class's its own copy of `own` with
`enclosingStatics` added or withheld. Field initializers and object-initializer
statements keep the class-level answer, since there is nowhere to annotate
them. `isTypeCheckingSkipped` already does the value reading and can be reused
as is. Two spec rows would pin it: the `@CompileStatic` host /
`@CompileDynamic` method expecting the static-member message, and the dynamic
host / `@CompileStatic` method compiling and returning `hello`.
## Notes (not blocking)
- `isTypeCheckingSkipped` (`:1778`) matches a `ConstantExpression` with
`endsWith("SKIP")`. I produced that path with a Groovy-compiled
`@AnnotationCollector` in a separate loader and the value arrives as the enum
constant, so `String.valueOf` gives exactly `SKIP` and `equals` would do.
`endsWith` is harmless as there is no other constant ending that way;
mentioning it only so the looser match is a choice rather than an accident.
- The Javadoc (`GrailsBeans.java:226-231`) and the guide
(`hookingIntoRuntimeConfiguration.adoc:229`) say "on a statically compiled
host". If the per-method fix above lands, "on a statically compiled host, or in
a method the anonymous class marks `@CompileStatic`" would keep the prose
exact; not worth touching otherwise.
## Verified Correct
- **Every spelling of the skip mode is caught, and each one really fails.**
`@CompileDynamic`, `@CompileStatic(TypeCheckingMode.SKIP)`,
`@CompileStatic(value = TypeCheckingMode.SKIP)`, `@CompileStatic(SKIP)` via
static import (a `PropertyExpression` after `StaticImportVisitor`),
`@GrailsCompileStatic(TypeCheckingMode.SKIP)`, and a precompiled Groovy
collector expanding to the same (the `ConstantExpression` branch) are all
reported with the static-member sentence. With the check bypassed all six
compile and fail with the `NoSuchFieldError` the message promises, so none is a
false rejection. A plain `@GrailsCompileStatic` host still reaches `helper() +
SUFFIX` and returns `hello`.
- **Receiver keying is right in both directions.** A `Runnable`-typed
anonymous class calling `join('')` is now reported, and with the check bypassed
it fails at runtime with the same `NoSuchFieldError`; an `Iterable`-only
supertype (`abstract class IBase implements Iterable<String>, IGreeter`)
calling `join('')` compiles and returns `hello`, so `getAllInterfaces()` on the
class itself covers the interface-only case and the per-superclass walk covers
the `ArrayList` subclass the spec pins. `Object` stays in the receiver set, so
`println`/`with`/`tap`/`each` remain reachable everywhere.
- **`answersAnything` narrowing pinned.** The `java.util.Properties`
superclass row rejects, and the two source-declared catch-all rows (own
`getProperty`, same-unit superclass `invokeMethod`) stay exempt - the three
cases I measured by hand last round, now in the spec.
- **Group path pinned.** `"a @CompileStatic host reaches its statics from
inside a group too"` runs the deferred static-compilation path and returns
`hello`.
- **Javadoc reflow.** The paragraph at `GrailsBeans.java:222-232` now wraps
at 98-101 columns throughout.
## Verification
- Read the full diff of `74f7951df6` (3 files, +226/-19) and re-read
`reportOutwardReferences`, `addExtensionMethodNames`, `enclosingStaticNames`,
`isStaticallyCompiled` and `applyStaticTypesTransformation` in the current file.
- Ran `./gradlew :grails-beans-dsl:check --continue` on the branch as
pushed: 393 tests, 0 failures, 0 errors, 0 skipped; module Checkstyle reports 0
errors across 5 files.
- Wrote a throwaway spec with eleven shapes (the six skip-mode spellings,
the `@GrailsCompileStatic` control, the two method-level annotations, the
`Runnable` and `Iterable`-only receivers, and a skip-mode host whose anonymous
class reaches nothing) and ran it twice: on the pushed head, and with the
sibling `rejectAnonymousClassReachingOutward` call disabled. 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` 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]
