beans DSL: silent-failure diagnostics, new declaration forms, the rest of the @Conditional* family - #16292
beans DSL: silent-failure diagnostics, new declaration forms, the rest of the @Conditional* family#16292codeconsole wants to merge 56 commits into
Conversation
The implicit `beans` convention claims a block only when every top-level statement is a bean/field/method call, so that an unrelated `beans` property on a pre-8.0 plugin descriptor is left alone rather than failed with a DSL error it never asked for. That is the right call for a block with no declarations in it. It is the wrong call for a block that has some. One stray statement among real declarations is not an unrelated property by any reading - it is the DSL with a mistake in it, typically a typo in a call name or an `if` wrapped around beans that belong under a @conditional* qualifier. Today the entire block is dropped in silence: an author who writes 28 bean() calls and fatfingers one gets zero beans registered, no diagnostic, and a failure that surfaces far away as beans that are simply absent at runtime. So report it, splitting by what there is to protect. An application class fails outright - `beans` has never meant anything but this DSL there, so there is no source compatibility at stake. A plugin descriptor only warns and is still left alone, which is the same conservatism the all-or-nothing claim already applies to it. A block with no declarations at all stays silent in both, unchanged. The message names the fix rather than just the symptom, since the two common causes have different ones: put the condition on the bean (.annotate(ConditionalOnProperty, ...) / .conditionalOnMissingBean()) rather than wrapping it in an if, and use method(...) for logic shared between beans.
Calling one @bean method from another and getting the registered singleton back is a CGLIB trick, and Spring only plays it for a full @configuration class. On a lite configuration source the identical call is a plain Java call that constructs a second instance. Lite is the common case for this DSL, not the exotic one. @autoConfiguration is @configuration(proxyBeanMethods = false); the sibling generated for a plugin descriptor carries exactly that; and a Grails Application class is a configuration source Spring reads @bean methods off without it being annotated @configuration at all. So the DSL's three main hosts are all unproxied, while the shape the mistake takes - `otherBean()` in a bean body - is the shape that was correct in the @configuration class the beans were most likely moved out of. Nothing about the failure is visible. The context starts, every bean exists, and two objects live where the author meant one, so a listener registers on the wrong instance or configuration applied to one is missing from the other. Detected after both processing passes, over the methods this block generated, so a synthesized method name is matched as reliably as a derived one. Only an unqualified call (or one written against `this`) counts - a call with a real receiver is somebody else's method that happens to share the name. Helper bodies from method(...) are scanned too, since a helper reaching a bean has the same problem, but a call TO a helper is left alone: there is no singleton there to miss. Proxying is resolved by walking the host's own annotations for a reachable @configuration, skipping any branch that sets proxyBeanMethods = false. That resolves @autoConfiguration through its own meta-annotation rather than by name, so a composed annotation nobody has heard of gets the same answer.
The javadoc says closure parameters become the generated method's parameters and stops there, so nothing promises their annotations survive - it reads as unsupported. It works, and the @qualifier tests already rely on it, but a reader deciding whether the DSL can express an optional dependency has no way to find that out short of trying it and disassembling the result. @Autowired(required = false) is the case worth naming, because it is the only way the DSL can say "inject this if some other module supplied it" - the shape every optional integration takes - and because unlike @qualifier it changes whether the context starts at all, rather than which candidate is chosen. It had no test. Three now: the annotated parameter starts a context with no candidate and receives null; the identical fixture without the annotation fails to refresh, which is what makes the annotation load-bearing rather than decorative; and the generated method really does carry the annotation, with required=false on it, rather than merely being present.
The type in bean(...)/field(...)/method(...) is a class literal, and Groovy has no syntax for writing type arguments on one, so a bean could only ever be declared raw. That is not cosmetic: Spring resolves an injection point by its full generic type, so a raw Handler bean cannot be told apart from another by whoever asks for Handler<Order>, and neither ObjectProvider<Handler<Order>> nor an injected List<Handler<Order>> can select it at all. The workaround was to declare the implementation class instead and let Spring read the arguments off its hierarchy, which only works when the implementation happens to be the type the author wants the bean known by - and is invisible as a workaround, so the raw declaration usually just stays and quietly narrows what can resolve it. .typeArguments(String) sits in the qualifier chain like the others but is consumed before the member is built, since it shapes the declared type rather than attaching anything. It applies to all three declaration forms: a field with @value("${names}") wants List<String> for the conversion, and a helper method's return type is read by the compiler the same way a bean's is. The arity is checked against the type's own parameters, so a mismatch is a located error naming both counts rather than an unchecked signature that misleads the next reader. Tested through to behaviour: two Handler beans differing only in their type argument, and Spring injecting the right one.
The javadoc called .annotate(...) an escape hatch for "any other single-valued annotation". It has never been limited that way - addMembersFromMap takes as many attributes as the annotation declares, and the mutually-exclusive-variants test has been passing ConditionalOnProperty three at a time. (The adjective itself went when that sentence was rewritten in the previous commit; this is the rest of what it was getting wrong.) Array-valued attributes are the part worth a test of their own rather than a sentence. They work today, but only incidentally: ConditionalOnProperty.name() is a String[] and that test passes it a bare string, so the widening is covered by a test that is not about it and would not say so if it broke. It is also the one behaviour here a reader has grounds to doubt - the transform builds these annotations at canonicalization, after the point where Groovy's own verifier does that widening, so it is reasonable to assume a single value would reach the bytecode writer unwidened and be written as a non-array. It does not; the writer widens it. Both spellings pinned, single value and list, against @dependsOn.
Found by reading the DSL's own in-tree users for exactly this: DataBindingGrailsPlugin
builds its registry with `new DefaultDataBindingSourceRegistry().tap { ... initialize() }`.
Inside a closure an unqualified call is resolved against the delegate - tap and
with are delegate-first - so that call reaches the registry, not the
configuration class. The AST records implicit-this either way, so the check as
written could not tell the two apart, and would have rejected working code the
moment a bean in the same block shared a name with a method on some delegate.
Rejecting valid code is a much worse failure than missing an invalid case, and
that plugin is one grep away from being the first casualty.
So the walk stops at a nested closure. The call that motivated the check -
`otherBean()` used directly to build this bean - is a statement in the body and
is still caught; what is given up is the rare sibling call written inside a
nested closure, which is worth it.
The regression test uses that exact shape, with a bean deliberately named
`initialize` so the check has something to match on.
The previous commit split the severity: a compile error on an application class, a warning on a plugin descriptor, on the grounds that a descriptor's `beans` property might predate the DSL. Three things are wrong with that. The warning is not a warning. GrailsASTUtils.warning is a System.err.println - it never reaches the ErrorCollector, so there is no IDE marker, no Gradle problem, and no record at all on the next build, which is UP-TO-DATE and prints nothing while the descriptor still registers zero beans. Against a failure whose whole problem is that it surfaces far from its cause, that is still silence. The compatibility being protected has no population. No Grails version has ever read a `beans` property off a descriptor - plugin loading reads doWithSpring, watchedResources, onChange and friends; the only "beans" in DefaultGrailsPlugin, on 8.0.x and 7.0.x alike, is BeanBuilder's own beans() method being handed the doWithSpring closure. So the descriptor this spared has to be dead code that also happens to contain a top-level bean(...) call. And a descriptor is the case where loudness matters more, not less: it is compiled by the plugin's author, but the missing beans are felt by every downstream application, whose developers never see the plugin's build output. Worse, isGrailsPluginDescriptorClass is a name-suffix test, so the split made severity depend on the class name - moving an identical block from Application.groovy into FooGrailsPlugin.groovy silently downgraded a build failure to a line of stderr. One rule now, host-independent: a `beans` closure containing any top-level bean/field/method call is the DSL and must be entirely the DSL; one containing none is not the DSL and is left alone. The message gains the way out for a property that genuinely is not the DSL - rename it - which is the escape the warning was standing in for.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16292 +/- ##
==================================================
+ Coverage 55.0529% 55.2638% +0.2110%
- Complexity 20773 21056 +283
==================================================
Files 2110 2111 +1
Lines 101368 102121 +753
Branches 18005 18233 +228
==================================================
+ Hits 55806 56436 +630
- Misses 37517 37556 +39
- Partials 8045 8129 +84
🚀 New features to boost your workflow:
|
Fixes Code Style / Core Projects CI failure on PR apache#16292: GenericsType import was out of lexicographical order in GrailsBeansASTTransformation, and the reportStrayBeansStatement message used a plain double-quoted string with no interpolation.
codeconsole
changed the title
sbglasius
left a comment
sbglasius
left a comment
There was a problem hiding this comment.
Note: this is an AI-assisted review (Claude Code). Two findings on the new
rejectUnproxiedSiblingBeanCallsdiagnostic; everything else in the diff held up under review. Treat the comments as starting points, not verdicts.
Things I checked and found correct, for the record:
MethodNodedoes not overrideequals, sogeneratedMembers'removeAllis identity-based exactly as its comment claims.ClassHelper.makeCachedeagerly callssetAdditionalClassInformation, andDecompiledClassNode.getGenericsTypes()forces lazy init, soraw.redirect().getGenericsTypes()is populated for both classpath and cached JDK types — the.typeArguments(...)arity check is sound forMap,AuditorAwareand friends alike.isSelfCallcannot misfire on a closure- orSupplier-typed parameter invoked asname(): Groovy rewrites that toname.call()duringSEMANTIC_ANALYSIS, before thisCANONICALIZATIONtransform runs. (Verified by compiling the shape and dumping the AST per phase.)- The closure-trimming in
declaredType(...)is consistent with the pre-existingouterCallhandling, andbean(...) { }.qualifier(...)is already rejected upstream.
The exemption for a full @configuration class was too broad. Spring's interception is CGLIB subclassing, and a static method cannot be overridden, so a call to a .staticMethod() bean is never intercepted - not even there. The early return skipped the whole class, so exactly the failure this diagnostic exists to catch went unreported on the one host where the author had most reason to believe the call was safe. The bean-method map is now built first and narrowed rather than abandoned: on a proxied host it keeps only the static bean methods, on any other host all of them. A non-static sibling call on a full @configuration class is still allowed, which is the exemption that host genuinely earns. Pinned by a hand-written @configuration with a static @bean and a sibling call - no DSL involved - asserting the two beans are different instances. That is the premise the rule rests on, so it is worth a test that fails if Spring ever changes it, rather than a comment asserting it. Reported by sbglasius in review.
The walk recorded every type it looked at, including branches it then pruned. So @configuration, reached through @autoConfiguration and immediately skipped on proxyBeanMethods = false, was memoized as seen - and a real proxying @configuration written alongside it was then skipped by that same guard, making the walk answer false for a class Spring proxies. Every sibling bean call on it would have been rejected as a compile error while returning the singleton perfectly well. visited exists to stop a cycle in the meta-annotation graph, which only needs it on the path that actually recurses. So the @configuration test and the proxyBeanMethods = false prune now both happen before anything is recorded, and only the descent records. The regression test uses @autoConfiguration and @configuration on one class - verified to fail with the previous walk, rather than assumed to. Reported by sbglasius in review.
…' into feat/beans-dsl-diagnostics-8.0.x
bean(["name", ] Type) constructs the declared type, so declaring a bean as
the interface its consumers inject forced a factory closure whose whole
content was the construction the bodyless form would have generated:
bean('communityHostUserProvider', CommunityHostUserProvider) {
new PixotoCommunityUserProvider()
}
Twenty-one such beans across two host applications say the same thing
twice. Naming the implementation after the type says it once:
bean('communityHostUserProvider', CommunityHostUserProvider, PixotoCommunityUserProvider)
The generated method returns the declared type and constructs the named
one, taking constructor arguments from the closure parameters exactly as
the bodyless form does. Recognised by two adjacent type literals, which
cannot collide with (name, Type) since a name is a String literal, so
field(...) and method(...) - which declare members, not beans - keep the
two-argument shape and their existing errors.
Naming an implementation is itself the construction, so that form takes
no body; a body alongside it is rejected rather than silently ignored.
The implementation is checked to be a concrete subtype at the bean(...)
statement, so the failure names both types instead of surfacing as an
assignment error inside a body the author never wrote. The bodyless
error on an interface now points at this form as the fix.
A bean declared as a generic interface and built from an implementation
that binds it had to restate the binding:
bean('auditorAware', AuditorAware).typeArguments(Long) { new SpringSecurityAuditorAware() }
SpringSecurityAuditorAware implements AuditorAware<Long>, so the
construction already proves what Spring will match injection points
against. Restating it is only an opportunity to state it differently
from the truth. It is now inferred, from a named implementation type or
from a body that is exactly a `new ...` expression, leaving the above as
bean('auditorAware', AuditorAware, SpringSecurityAuditorAware)
Inference only ever adds what the compiler could already see, and
declines rather than guesses: an explicit .typeArguments(...) always
wins, a body that is anything but a bare construction proves nothing,
and a binding that resolves to a type variable is no answer.
A raw construction of a generic type is declined for a sharper reason
than uninformativeness. Groovy resolves its parameters to their bounds,
so `new GenericBox()` would infer Holder<Object> - and a bean typed
Holder<Object> stops matching the Holder<String> injection point it
satisfied while raw. Narrowing a bean's type on that evidence would be a
silent behaviour change, so raw evidence leaves the raw type standing.
The DSL compiles a beans block into an @autoConfiguration but could not express the shape auto-configurations actually take. Spring Boot's own JacksonAutoConfiguration carries four nested @ConditionalOnClass configuration classes; this DSL had no way to write one, so the beans that need that shape had to move to a separate top-level class of their own. They are not a stylistic preference. A condition is read from the bytecode before anything is loaded, but a @bean method's parameter and return types resolve when its configuration class is parsed - so a bean whose own signature names a class that may be absent cannot be guarded on the method, which is exactly the bean most likely to want guarding. Moving it into a group moves the guard onto a class that is never parsed when the condition fails. The test for that declares a bean taking a type nobody supplies, guarded on a class that does not exist, and asserts the context still starts. group("imageServing").conditionalOnClass(name: "...") { ... } generates a nested static @configuration(proxyBeanMethods = false) named Host$ImageServingConfiguration - the suffix because a bare name says nothing in a stack trace or an /actuator/beans listing. Spring finds it unaided, since ConfigurationClassParser processes a configuration class's member classes. proxyBeanMethods = false matches what Spring Boot's nested classes carry, and keeps the sibling-call diagnostic meaningful inside the group: its beans are as unproxied as the ones outside it, and are checked the same way. Takes the conditions and .annotate(...); the bean-shaped qualifiers have nothing to attach to. Groups do not nest, and the closure takes no parameters - it declares a class, not a bean. 'group' joins the root-call set in GlobalGrailsClassInjectorTransformation too, or a block containing one would be rejected by the stray-statement diagnostic rather than compiled.
CI failed on the positive half of the conditionalOnBean test: the transport bean existed and the adapter still did not register. The test declared both in one beans block, so whether the condition matched depended on @bean method order within a single class - it passed locally and failed on CI, where the build now compiles with invokedynamic off. That is the annotation working as documented, not a transform bug. @ConditionalOnBean matches only against bean definitions the context has already processed, which is exactly why Spring restricts it to auto-configurations. A bean naming another declared beside it is not a condition that reliably holds. The transport now comes from a separate @configuration registered before the fixture, which is the one ordering the annotation does guarantee, so both halves are deterministic. The javadoc says so too - the qualifier had no note about it, and the caveat is the kind this DSL otherwise makes a point of naming. Also fixes the checkstyle ImportOrder violation that came in with group(...): GenericsType sorts before InnerClassNode.
|
AI review: Review FindingsThis PR bundles four themes on [P1]
|
OnGrailsEnvCondition reads grails.util.Environment reflectively and falls back to the grails.env property when it cannot, so a non-Grails context is skipped quietly instead of failing. The catch did not cover the ways that actually happens. ClassNotFoundException only covers the class being absent from the loader that was asked. A class present but unlinkable - a transitive dependency missing, a version mismatch - raises NoClassDefFoundError, and Class.forName here initializes (reading getCurrent needs it), so a static initializer that blows up raises ExceptionInInitializerError. Both are LinkageError, neither is a ReflectiveOperationException, and either was thrown straight out of matches(), failing the configuration this condition exists to skip. LinkageError joins the catch; VirtualMachineError deliberately does not. The loader fallback now goes through Spring's ClassUtils.getDefaultClassLoader() rather than this class's own loader. ConditionContext.getClassLoader() may be null early in bootstrapping, and the default tries the thread context loader first - which is the one that can see Grails when this class and the application are loaded separately, as under Boot's LaunchedClassLoader. The condition had no spec of its own, being exercised only through generated bytecode. It has one now: absent attributes, the property fallback, case-insensitive matching across several environments, neither source answering, and both LinkageError paths - the last two verified to fail against the unbroadened catch rather than assumed to. Reported by matrei in review.
The beans-DSL section of hookingIntoRuntimeConfiguration.adoc enumerates the declaration forms and qualifiers by hand, so it described the DSL as of apache#16019 and none of what this PR adds. CLAUDE.md rule 7 asks for grails-doc coverage of any user-facing change, and this is a large one. Added: bean(name, Interface, Implementation) and what a construction settles about type arguments, .typeArguments(...), the map-construction one-liner for a property-configured implementation, constant member names, group(...) with the signature-guarding case that is its whole reason to exist, and the five remaining conditions - onBean (with the ordering it can and cannot promise), onProperty, onExpression, onClass (types vs String names, and where to put the condition), onGrailsEnv - plus .aliases(...) and .scope(...)'s other attributes. Corrected: .staticMethod() is now required for a post-processor bean rather than recommended, and .annotate(...) is not limited to single-valued annotations and merges into an annotation a qualifier already attached rather than colliding with it. New sections for the two things a reader cannot discover from the syntax: the three diagnostics, each with what used to happen instead, and the dumpdir property. The diagnostics section is also where the behaviour change lands - a plugin descriptor whose beans property has a stray statement among real declarations now fails to compile where it used to be quietly ignored. Reported by matrei in review.
|
Addressed in f7d22ce (code + spec) and cf68805 (docs). [P1] [P2] class loader — the null case now goes through Spring's [P3] spec — new [P2] behaviour change + [P3] docs — One clarification: the reflective catch and the loader fallback are the runtime half of CI is also green again as of c4cadbb: a checkstyle |
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review Findings
Two new commits since the previous round (f7d22cebd0, cf6880565a) address every item from the earlier review. No new findings — the branch is ready to merge.
Resolved Prior Findings
[P1] OnGrailsEnvCondition reflection catch — fixed
File: grails-beans-dsl/src/main/java/org/grails/compiler/beans/OnGrailsEnvCondition.java:76-107
The catch now covers LinkageError in addition to ReflectiveOperationException and RuntimeException, so both NoClassDefFoundError (transitive class missing at link time) and ExceptionInInitializerError (Class.forName initializes here, so a broken static initializer on Environment is reachable) fall back to the property rather than throwing out of matches(...). VirtualMachineError is deliberately still uncaught. The in-line comment enumerates the cases, so the intent will survive future edits.
[P2] TCCL fallback — fixed
File: grails-beans-dsl/src/main/java/org/grails/compiler/beans/OnGrailsEnvCondition.java:79-82
Loader fallback now goes through Spring's ClassUtils.getDefaultClassLoader() rather than getClass().getClassLoader(). That helper tries the thread context loader first — the correct loader to reach the application's Grails when this class and the application are loaded separately (e.g. under Boot's LaunchedClassLoader). Matches Spring's own class-presence pattern.
[P3] Missing direct spec for OnGrailsEnvCondition — fixed
File: grails-beans-dsl/src/test/groovy/org/grails/compiler/beans/OnGrailsEnvConditionSpec.groovy (new, 117 lines)
Six focused feature methods cover every path I asked for and a couple more:
- Null annotation attributes →
false(no NPE). - Property fallback when
grails.util.Environmentis absent (ClassNotFoundException). - Property fallback when the class is present but unlinkable (
NoClassDefFoundError) — this test would fail against the previous, un-broadened catch, which is the point. - Property fallback when static initialization fails (
ExceptionInInitializerError). - Case-insensitive matching, several environments named at once.
- Neither Grails nor the property answering →
false.
The loaderThrowing(...) helper is a nice touch: a ClassLoader that throws for grails.util.Environment specifically and defers otherwise, so the paths are exercised without polluting the real classpath.
[P3] Documentation coverage — fixed
File: grails-doc/src/en/guide/plugins/hookingIntoRuntimeConfiguration.adoc:167-230
The user guide now covers every new item, added inline in the existing bullet list rather than as a separate appendix:
bean(name, Interface, Implementation)and how a construction settles declared type arguments..typeArguments(Type, ...)for the cases where it does not.- The map-construction one-liner for a property-configured implementation.
- Compile-time String constant names for members.
group("name") { ... }with the signature-guarding case that is its whole reason to exist.- All five new conditions:
.conditionalOnBean(...)(with its ordering caveat),.conditionalOnProperty(...),.conditionalOnExpression(...),.conditionalOnClass(...)(types vs String names, and where to put the condition),.conditionalOnGrailsEnv(...). .aliases("legacyName")and.scope("session", proxyMode: ...)with its other attributes.- New "Diagnostics" section listing the three checks that turn silent runtime failures into compile errors — including an explicit note that the stray-statement diagnostic on a plugin descriptor is a behavior change.
- New "Seeing what a block compiled to" section for
-Dgrails.beans.dsl.dumpdir=<dir>.
Two corrections beyond what the earlier review flagged: .staticMethod() is now stated as required (rather than "recommended") for post-processor beans, matching the compile-time rejection; and .annotate(...) is no longer described as single-valued and correctly explains that naming an annotation a qualifier already attached is a merge rather than a collision — which is the only way to reach attributes no qualifier sets, e.g. .annotate(Bean, destroyMethod: '').
Verification
- Read
git log origin/8.0.x..HEAD(42 commits, two new since the previous review). - Read the two new commits in full:
f7d22cebd0(Fall back rather than throw when Grails cannot answer) andcf6880565a(Document the new DSL surface in the user guide). - Read the new
OnGrailsEnvConditionSpec.groovyand confirmed theNoClassDefFoundErrorcase is asserted, which is the load-bearing regression test for the P1 fix. - Read the doc diff and cross-checked against the missing-items list from the previous review — every item is present, plus the two accuracy corrections above.
- Both new Java changes carry the Apache license header from the existing file; the new spec carries a fresh header.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I compared the head of this branch against current 8.0.x with a set of small probe specs, on top of the module's own tests, a fresh compileGroovy of every in-tree beans = { } user (core, i18n, cache, url-mappings, databinding, domain-class, mail, sitemesh3) and the two beans-dsl example projects. All of that is green.
Three things need attention before this merges:
- A regression in the anonymous-inner-class re-homing: an anonymous class inside a nested closure worked on
8.0.xand now fails at runtime (instance bean) or at compile time (.staticMethod()bean). Details and a one-line fix inline. group(...)does not survive@CompileStaticon either host kind, failing in class generation with aGroovyBugError. Since every in-tree plugin descriptor is@GrailsCompileStatic, the feature is currently unusable where it is aimed.- On a plugin descriptor, an anonymous class that reaches back to a
method(...)/field(...)member fails withNoSuchFieldErrorat runtime.
Two smaller notes follow on shared-name validation for the new declaration forms and on how the dump property has to be set to reach the compiler.
Re-homing an anonymous inner class descended into nested closures, and an anonymous class written inside one has not lost its enclosing instance - that closure survives the lift and is still it. Re-homing it anyway rewrote a correct `this` into the configuration class, so a shape that works on 8.0.x failed at runtime with the GroovyCastException the re-homing exists to prevent, or, under .staticMethod(), was rejected outright though it compiles. The walk now stops at closure boundaries, the same way the sibling-call check does. group(...) did not survive @CompileStatic on either host kind, which is where it is aimed - every in-tree plugin descriptor is statically compiled. Groovy's InnerClassCompletionVisitor adds the MOP dispatch methods every inner class gets as a post-transform CANONICALIZATION operation, after this transform, so marking the nested class for static compilation here left those methods generated without ever being type-checked and class generation died with "StaticTypesCallSiteWriter#makeCallSite should not have been called". The pass is deferred to INSTRUCTION_SELECTION, once the methods exist. The sibling's own pass needed the same treatment, since it reaches any nested class the sibling holds. A test asserts a body that only type-checks dynamically is still rejected, so deferring cannot quietly become skipping. All four existing group tests used dynamic hosts, which is why none of this showed. Shared-name validation read the [name, ] Type head with its own copy of the old rules, so both declaration forms this PR adds fell out of it: a three-argument declaration had too many arguments, and a constant name was not a ConstantExpression. Two declarations of one name compiled in either form and Spring silently kept the first - the exact outcome the check exists to prevent. It now reads the head the way processBeanStatement does, sharing the implementation-splitting and the constant resolution. Documented rather than fixed: on a plugin descriptor an anonymous inner class keeps the descriptor as its outer class, so an unqualified reference from its body to a field(...)/method(...) member fails with NoSuchFieldError at runtime. Recognising that at compile time means deciding which names are the anonymous class's own, and a false positive would reject working code - so it is a note in the guide and the javadoc, not a check. Also corrected the dumpdir instructions: GroovyCompile forks, so -D on the Gradle command line never reaches the process that runs the transform. The guide now shows groovyOptions.forkOptions.jvmArgs and says why an absolute path is the safe form. Reported by jdaugherty in review.
On a plugin descriptor the block's members compile onto the generated sibling, but an anonymous inner class written in a bean body keeps the descriptor as its outer class - Groovy fixes that when it creates the node and offers no way to move it. Its MOP dispatch methods then read a this$0 typed as the descriptor where the field holds the sibling, so an unqualified reference from its body to one of those members failed with NoSuchFieldError inside a running application, or under @CompileStatic as a "cannot find matching method" naming a synthetic class nobody wrote. Now a compile error, pointed at the reference. The test is narrow enough to be safe: a name that is both a member this block generated and not resolvable on the anonymous class itself or anything it inherits. A call to the anonymous class's own method, or to one from the interface it implements, is untouched - only names that actually moved cannot be reached. On a non-plugin host nothing moved, so nothing is checked. Three tests: the failing shape rejected with the reference named, a self-contained anonymous class on a descriptor still compiling, and the same reference on a plain host still working at runtime.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Re-checked the head (6663088) with the same probe set as last round, the module spec (313 green locally), and the in-tree beans = { } users. Items 1, 4 and 5 from the previous round are fixed as described - the nested-closure shapes run again, the three-argument and constant-named duplicates are rejected, and the dump property is documented where it counts. Two fixes are incomplete, and one of them introduced a regression:
- A
@CompileStaticplugin descriptor with an anonymous inner class in a bean body now fails with aGroovyBugError. It compiled and ran at cf68805. Deferring the sibling's static-compilation pass reversed the order of the two passes over the anonymous class, and Groovy then dereferences an enclosing method the lift never set. Everygroup(...)body that constructs an anonymous class crashes on the same line, on both host kinds; closure-only group bodies are fine now. Root cause, a verified fix and the missing tests inline. - An anonymous class in a
group(...)body that reaches anything outside itself fails at runtime withNoSuchFieldError, on every host kind, and the new compile-time check does not cover groups. Inline.
One smaller note on property-style access to a moved accessor.
On CI: the two red jobs are the known 5% flake in UserControllerSpec (#16030) and a runner that lost contact with the server on the Hibernate7 indy=true job, which is green on the last five 8.0.x runs. Neither is this branch.
The parser sets `enclosingMethod` on every anonymous class declared inside a method body and has none to set for one written in a property initializer, so a class lifted out of the `beans` closure arrives in its new method without one. Static compilation reads it through the constructor call whenever the class has already been visited as an inner class of its outer, and that ordering is reached two ways: a `@CompileStatic` plugin descriptor, whose own pass now runs first at INSTRUCTION_SELECTION and marks the class before the deferred sibling pass gets to the call, and a `group(...)` on either host kind, where the host visits its inner classes in registration order and the anonymous class was registered at parse. Both die in class generation with `GroovyBugError: ClassNode.getEnclosingMethod() is null`. Setting it alongside the `this$0` field, the constructor parameter and the call argument the lift already corrects is what the parser would have done. The bytecode gains an EnclosingMethod attribute naming the bean method, which is what `this$0` already says.
An anonymous class in a `group(...)` bean body has the same problem the sibling check exists to catch, and never reached it: the group's beans compile onto a nested class while the anonymous class keeps the host as its outer class, so every MOP dispatch out of it reads a `this$0` typed as the host and fails with `NoSuchFieldError` at runtime - for a group member, for a host member, on either host kind. The reachable set for a group is smaller than for a sibling. The group is a static nested class with no enclosing instance behind it, so nothing outside the anonymous class is reachable at all, and the check rejects any implicit-this call or dynamic variable the class cannot resolve on itself or what it inherits rather than consulting the moved names. A self-contained class, and a captured local, are unaffected - which is what the error suggests.
`method('getSuffix', String) { '!' }` on a descriptor plus `'hello' + suffix` in
an anonymous class is the same unreachable reference by another spelling, and
comparing raw names alone let it through to the `NoSuchFieldError` the check
exists to prevent - `suffix` is a DynamicVariable while the moved set holds
`getSuffix`. A variable now matches on the accessor names it would resolve to as
well as on itself, the way collectMethodNames already reserves them.
67d20dc set it only for a class written directly in a bean body, which is the right scope for re-homing `this$0` - a class inside a nested closure keeps that closure as its enclosing instance and must not be re-homed - but the wrong scope for the enclosing method. The parser sets that per enclosing METHOD, so a closure in between makes no difference to what it would have written, and without it the same `GroovyBugError` fires one closure deeper: on a `@CompileStatic` descriptor, in a `@CompileStatic` group on either host kind. Split into its own walk, which descends where the re-homing walk deliberately stops. Also corrects the reach error, which named only `NoSuchFieldError`: a nested-closure class fails with a `ClassCastException` between the group (or sibling) and the host instead, measured with the check disabled.
This comment has been minimized.
This comment has been minimized.
635f284 mapped a property-style read onto a moved accessor, which was one of four spellings of the same unreachable reference. The other three still compiled and still failed with NoSuchFieldError at runtime, measured on a descriptor and in a group: this.suffix() an explicit-this call - the walk tested isImplicitThis, where the sibling-call check next to it already uses isSelfCall this.suffix a PropertyExpression, which the walk never visited at all getSuffix() a call naming the accessor of a moved property or field, the inverse of the mapping 635f284 added So the name set now relates both ways, and the walk visits self-calls and this-qualified properties as well as dynamic variables. A reference is left alone when ANY of its spellings resolves on the anonymous class or what it inherits, which is what keeps `this.name` reaching its own getName() legal.
Two defects in the reach check, both from its idea of what resolves. The sibling branch reported only the names the block generated, on the premise that "the descriptor is still what this$0 is typed as, so everything else about it resolves the way it reads". That premise is wrong - the lift retypes this$0 to the sibling - so a call to a method the descriptor itself declares, a read of a field it declares, and a member inherited from Plugin all failed at runtime with the NoSuchFieldError this check exists to prevent, and none was reported. Both host kinds now use the same rule: anything the anonymous class cannot answer itself is out of reach. A field or property that VariableScopeVisitor resolved against the ENCLOSING class counts as out of reach too - it reads as resolved while still needing this$0 to be fetched - where a local or a parameter does not, the lift copying those into the class. The group branch, conversely, reported too much. "Resolve on itself" was only the declared and inherited members, so every extension method every object has - println, with, tap, identity, sleep - was rejected inside a group though the runtime answers all of them against the instance without ever reading this$0. Those names come from DefaultGroovyMethods now, and a class declaring its own methodMissing/propertyMissing is skipped entirely, being able to answer anything. Also corrects the javadoc's dumpdir instruction, which still said to build with -Dgrails.beans.dsl.dumpdir=<dir> - the spelling the guide documents as writing nothing under a forking GroovyCompile - and omitted the absolute-path caveat.
Both walks took liftedMethod.getCode() as their only root, but a closure parameter's default value comes across with the parameter, so a class written there was invisible to the enclosing-method fix and to the re-homing. A method(...) helper with such a default on a @CompileStatic descriptor still died with the GroovyBugError two commits back addressed - the same defect, one location deeper than the body and a nested closure inside it.
Two places the lift repairs a class and the check could not see it, both
reaching runtime as the NoSuchFieldError this exists to prevent:
a parameter default the previous commit taught the lift about these and not
the check, so a class there was re-homed silently
a class nested inside the check pulls an anonymous class's own code out of
another anonymous one the InnerClassNode by hand, and never recursed into the
anonymous classes it found in there
The recursion carries the reachable names down the chain, because a nested
anonymous class's own enclosing instance was never retyped - it holds the outer
anonymous class as written - so a name on that outer class does resolve from
inside, and only what lies beyond the outermost one is out of reach.
Also corrects two javadoc claims the recent behaviour changes falsified: that a
.staticMethod() bean "cannot carry" an anonymous class, when only one written
directly in its body is rejected and one inside a nested closure is fine and
tested; and that the dump writes one file per host class, when a group writes
its own file under the nested class's name.
Two false positives, each rejecting code that compiles and runs.
The walk descended into closures written inside the anonymous class body and
applied the implicit-this rule there, but a closure's resolve strategy and
delegate are runtime facts: `sb.tap { append(x) }` and `s.with { toLowerCase() }`
are answered by the delegate and were rejected. That is the same shape and the
same fix already applied to rejectUnproxiedSiblingBeanCalls, so this walk stops
at closures too. The cost is a missed diagnostic for a plain owner-first closure
reaching outward, which still fails at runtime with an error naming the class -
strictly better than refusing to compile working code.
And `this.tag` was reported for a field inherited by the anonymous class while
the bare `tag` was not: ClassNode.getFields() is declared fields only, so the
property path found nothing in the reachable set while the variable path
resolved to a real FieldNode and let it through. The inherited field names are
added, so the two spellings agree.
|
All items from both rounds are addressed. 9 commits, 361 module tests (was 313), each fix confirmed to fail with it backed out. Round 1
Round 2
Six further gaps in the same code paths, found while completing the above and each reproduced first:
Also corrected: the javadoc said a Not changed, and worth your view if you disagree: duplicate names across a CI: the two reds are |
4 participants
Work on the
beansDSL from #16019, in four kinds: diagnostics for mistakes the DSL currently accepts in silence, new declaration forms, the rest of the@Conditional*family, and one tooling addition. 33 commits over 7 files, all ingrails-beans-dslplus the implicit-convention hook ingrails-core.Every in-tree
beans = { }user still compiles — cache, core, databinding, domain-class, i18n, mail, url-mappings, sitemesh3 — and thebeans-dsl/beans-dsl-pluginexample projects pass.Reading order
1. Behaviour changes first — these are the only commits that can turn a build that passed into one that fails, and they deserve the most scrutiny:
76c0cc3c6509f299942cce57a0c90c96c2768dd62867e4d39d685b98.staticMethod()8f8e86a52. Then the new syntax, which is purely additive: declaration forms (
e27cb9bfb1c0c4254ec20d1b3c498ebb41d36653), then qualifiers (5ea9efe4507dd2c869c5de628f305841a2603b0ee0f99fc54959bf49bf31c481).3. Then tooling and fixes:
910dc72b(dump),7df2280e6e32b25e(anonymous inner classes in a lifted body),6fbfd28c(meta-annotation walk).4. Docs, tests and tidy-ups last:
dedfe26c2aabf4b24a572c7dfb07669016902348683432b285b14177.b8a8d00arenames.grailsEnv(...)to.conditionalOnGrailsEnv(...); both spellings were introduced in this PR, so nothing outside it is affected.1. Diagnostics
A partly DSL-shaped block is reported, not dropped
A block is claimed only when every top-level statement is a
bean/field/methodcall, so an unrelatedbeansproperty is left alone. Right for a block with no declarations — wrong for one that has some, where today every declaration is dropped without a word:Both application classes and plugin descriptors now fail, naming the statement and the way out for a
beansproperty that genuinely is not the DSL — rename it. A block with no declarations at all stays silent, unchanged. Descriptors are not treated more leniently: no Grails version has ever read abeansproperty off one (plugin loading readsdoWithSpring,watchedResources,onChange), and a descriptor is compiled by its author while its missing beans are felt by downstream applications that never see that build output.A sibling bean call that cannot return the singleton
Getting the registered singleton from a call to another
@Beanmethod is a CGLIB trick, and Spring only plays it for a full@Configurationclass.@AutoConfigurationis@Configuration(proxyBeanMethods = false), a generated plugin sibling carries exactly that, and a GrailsApplicationclass is a configuration source without being annotated at all — so the DSL's three main hosts are all unproxied:Nothing about that is visible at runtime: the context starts, every bean exists, and two objects live where one was meant. Inject it instead, as a closure parameter.
Scope, arrived at by testing rather than assumption: targets include hand-written
@Beanmethods on the same class (the mixed state a migration passes through); a.staticMethod()bean is still checked on a proxied host, because CGLIB cannot override a static method and Spring documents such calls as never intercepted; and the walk stops at nested closures, sincenew Registry().tap { initialize() }is delegate-first at runtime but implicit-this in the AST —DataBindingGrailsPluginwrites exactly that shape.2. New declaration forms
Type arguments matter because Spring resolves an injection point by its full generic type, and the type in
bean(...)is a class literal, on which Groovy has no syntax for writing them.group(...)— the nested conditionally-guarded configuration classGenerates a nested static
@Configuration(proxyBeanMethods = false)namedHost$ImageServingConfiguration, with the conditions on the class. Spring finds it unaided, sinceConfigurationClassParserprocesses a configuration class's member classes.This is the shape real auto-configurations take — Spring Boot's own
JacksonAutoConfigurationcarries four nested@ConditionalOnClassclasses — and the DSL had no way to write one, so beans needing it had to move to a separate top-level class. It is also the only shape that works for a bean whose own signature names a class that may be absent: a condition is read from the bytecode before anything is loaded, but a@Beanmethod's parameter and return types resolve when its configuration class is parsed, so guarding such a bean on the method is not reliably safe. Moving it into a group moves the guard onto a class that is never parsed when the condition fails.3. The rest of the
@Conditional*family.conditionalOnBeanhas no zero-argument form: with nothing named Spring deduces the type from the bean's own return type, conditioning a bean on a bean of its own type already existing..conditionalOnClasstakes types and String names in the same positional list, because a literal is compiler-checked but can only name a class this module compiles against, while a class that may be absent has to be a String — and its javadoc says where to write the condition, since a@Beanmethod's parameter and return types resolve when the configuration class is parsed, making the class-level guard the right one for a signature that names the optional type.4. Seeing what a block compiled to
Writes one
<qualified name>.beans.txtper host class:Declarations only — bean names, the annotations the qualifiers became, declared types with any type arguments they ended up carrying, and parameter annotations. Bodies are omitted, being the author's own closure bodies lifted verbatim. Nothing is written unless the property is set. Grails already takes this shape for its other compile-time generator, where
grails.views.gsp.keepgenerateddirkeeps the Groovy a GSP compiles to.Also
Closure parameters carry their annotations onto the generated method, which is how an optional dependency is expressed —
bean('smsSender', SmsSender) { @Autowired(required = false) SmsTransport t -> ... }. That worked before; it was undocumented and therequired = falsecase was untested, so both are now covered.