matrei commented on PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#issuecomment-5644173002
# Review Findings (round 4)
One new commit since the previous round (`d5c7bdf097`, *Widen the reachable
set to what the runtime actually answers*). It addresses all three findings
from that round: the extension-method set is now built from
`DefaultGroovyMethods.DGM_LIKE_CLASSES`, static members of a `@CompileStatic`
host are in reach and the dynamic-host message says how to qualify them, and
`invokeMethod`/`getProperty` count toward `answersAnything` only when declared
in a class this unit compiles from source. I re-measured the two shapes that
were rejected last round and both compile and run now. No blocking findings;
two small items below, one of them a measured runtime gap in an unusual shape.
## New Findings
### [P3] `isStaticallyCompiled` treats
`@CompileStatic(TypeCheckingMode.SKIP)` as statically compiled
**File:**
`grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:1737-1744`
The test is "carries a `@CompileStatic` annotation", without reading its
`value`. `@CompileDynamic` is an `@AnnotationCollector` for
`@CompileStatic(TypeCheckingMode.SKIP)` and is expanded before
canonicalization, so a host written either way passes the test, while its
bytecode is dynamic and a static member of the enclosing class *is* reached
through `this$0`. Measured:
```groovy
@CompileStatic(TypeCheckingMode.SKIP)
@GrailsBeans
@AutoConfiguration
class ScrEGrailsPlugin extends Plugin {
static String helper() { 'hello' }
def beans = {
bean('greeter', ScrEGreeter) { new ScrEGreeter() { String greet() {
helper() } } }
}
}
```
compiles without a diagnostic, and `greeter().greet()` fails with:
```
java.lang.NoSuchFieldError: Class ScrEGrailsPlugin$1 does not have member
field 'ScrEGrailsPlugin this$0'
```
This is exactly the error the check exists to turn into a compile error. The
shape is rare - a whole `@GrailsBeans` host marked dynamic inside an otherwise
static file - but `applyStaticTypesTransformation` (`:563-566`) copies the
annotation's members onto the sibling/group, so the compiler already knows the
body will be dynamic; the reach check just does not consult the same fact.
Suggested fix: in `isStaticallyCompiled`, read the `value` member of the first
`@CompileStatic` found and return `false` when it is `TypeCheckingMode.SKIP` (a
`PropertyExpression` ending in `SKIP`, or a `ConstantExpression` after
folding). A spec row with the shape above expecting the `static member of an
enclosing class` message would pin it.
### [P3] The third correction has no test, and the static-member row covers
only the sibling
**File:**
`grails-beans-dsl/src/test/groovy/org/grails/compiler/beans/GrailsBeansASTTransformationSpec.groovy:2111-2177`
The new features pin the DGM widening (group) and the `@CompileStatic`
static member (sibling), plus the dynamic-host rejection. Not pinned:
- **The `answersAnything` narrowing.** Nothing in the spec exercises
`invokeMethod`/`getProperty` at all - the only catch-all row is `'its own
methodMissing'` at `:1919`. I measured the three cases the new `getModule()`
discriminator is meant to separate, and all three behave as the commit message
says: an anonymous class extending `java.util.Properties` (a precompiled class
declaring `getProperty(String)`) and calling a moved `suffix()` is now reported
with the `"suffix()" does not resolve` message where before this commit it was
exempt; an anonymous class writing its own `getProperty` is still exempt; a
same-unit source superclass writing `invokeMethod` is still exempt. Those three
are the regression surface for this change and are one `where:` table away.
CLAUDE.md rules 10 and 13 ask for every touched behavior to be covered.
- **A `@CompileStatic` host with a `group(...)`.** The sibling and the group
take different static-compilation paths (the group's is deferred to
`INSTRUCTION_SELECTION`), and `staticsInReach` is decided from the host's
annotation for both. I measured a `@CompileStatic` host with `group('extras')`
and `helper() + SUFFIX` inside the anonymous class: it compiles and `greet()`
returns `hello`. A row in `"the reachable set follows what the runtime
answers"` for that shape would keep the two paths from drifting.
## Notes (not blocking)
- **Receiver-aware extension set.** The over-approximation the comment at
`:1689-1695` accepts is avoidable at the same cost: keep a DGM method only when
its first parameter type is a supertype of the anonymous class (compare
`Class.getName()` against the names of the inner's superclass chain, all its
interfaces, and `Object`). For a `Runnable` implementation that yields exactly
the old `Object`-receiver set; for an `ArrayList` subclass it adds the
`Iterable`/`Collection`/`List` entries. It would also stop the `relatedNames`
expansion from letting a moved property named `text`, `first`, `last`, `count`
or `lines` slip through on an unrelated receiver, since those match DGM
`getText`/`first`/`last`/`count`/`getLines` today. Optional; the trade as
written is documented and defensible.
- `DGM_LIKE_CLASSES` does not include extension modules found on the
classpath (`groovy-nio`'s `NioExtensions`, `groovy-datetime`, and so on). An
anonymous class whose receiver type is one of those is unusual enough that I
would only mention it in the comment, not handle it.
- **Javadoc wrap** at
`grails-beans-dsl/src/main/java/grails/compiler/beans/GrailsBeans.java:228`:
the new sentence runs to ~170 columns on one line while the paragraph around it
wraps at ~100. Checkstyle has no line-length rule so it passes; reflow for the
next reader.
## Verified Correct
- `DGM_LIKE_CLASSES` is `public static final Class[]` on Groovy 5.1.2
(checked with `javap`), so the constant is a stable public surface to build
from.
- **Both shapes rejected last round now compile and run** - the
`ArrayList`-typed anonymous class calling `join('')` in a group, and the
`@CompileStatic` descriptor's `helper() + SUFFIX` - each as a spec row and each
returning `hello`.
- **The dynamic-host static member is still reported**, and the appended
sentence names the fix (qualify with the declaring class).
`Collections.disjoint(spellings, enclosingStatics)` keys the sentence on the
same expanded spellings the rejection uses, so `getFoo()` against a static
property `foo` gets it too.
- **`enclosingStaticNames`** walks the outer chain and each outer's
superclass chain, so an inherited static of `Plugin` on a `@CompileStatic`
descriptor is in reach, matching how the static compiler resolves an
implicit-this call from an inner class.
- **`answersAnything`** keeps `methodMissing`/`propertyMissing`
unconditional and stops at `Object`, so the earlier exemption behavior for
those two is unchanged; the `getModule()` discriminator is measured above.
- **Docs.** The `GrailsBeans` Javadoc and the guide NOTE both now say
"Groovy's extension methods" and state the static-member rule with the same
condition the code applies.
## Verification
- Read the full diff of `d5c7bdf097` (4 files, +146/-25) and re-read
`rejectAnonymousClassReachingOutward` through `relatedNames` in the current
file.
- Ran `./gradlew :grails-beans-dsl:check --continue` on the branch as
pushed: 387 tests, 0 failures, 0 errors, 0 skipped; module Checkstyle reports 0
violations.
- Wrote a throwaway spec with five features (the `@CompileStatic` group
shape, the `java.util.Properties` superclass, an anonymous class writing
`getProperty`, a source superclass writing `invokeMethod`, and the
`TypeCheckingMode.SKIP` host) and ran it against the branch; results are as
reported above. The spec was deleted afterwards and nothing from the experiment
remains in the tree.
## 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]
