sbglasius commented on code in PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#discussion_r3915706485
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -878,6 +890,118 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
classNode.addMethod(beanMethod);
}
+ // The methods this block just generated, in declaration order: everything
on the host that was
+ // not there before the two processing loops ran. MethodNode does not
override equals, so the
+ // removal is by identity and cannot drop a same-signature method the user
wrote.
+ private List<MethodNode> generatedMembers(ClassNode host, List<MethodNode>
preExisting) {
+ List<MethodNode> generated = new ArrayList<>(host.getMethods());
+ generated.removeAll(preExisting);
+ return generated;
+ }
+
+ /**
+ * Rejects a call from one generated method to another generated {@code
@Bean} method, on a host
+ * whose bean methods Spring does not proxy.
+ *
+ * <p>Calling a sibling {@code @Bean} method and getting the singleton
back is a CGLIB trick, and
+ * Spring only plays it for a full {@code @Configuration} class. On a
<i>lite</i> configuration
+ * source the same call is a plain Java call that constructs a second
instance - and lite is the
+ * common case for this DSL: {@code @AutoConfiguration} is
+ * {@code @Configuration(proxyBeanMethods = false)}, the sibling generated
for a plugin descriptor
+ * carries exactly that, and a Grails {@code Application} class is a
configuration source without
+ * being annotated {@code @Configuration} at all.</p>
+ *
+ * <p>Nothing about that failure is visible at runtime. 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. It is also the
exact mistake a
+ * migration invites, since moving bean methods off a real {@code
@Configuration} class into this
+ * DSL silently changes what those calls mean.</p>
+ */
+ private void rejectUnproxiedSiblingBeanCalls(ClassNode host,
List<MethodNode> generated, SourceUnit source) {
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ for (MethodNode method : generated) {
+ if
(!method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty() || beanMethodsAreProxied(host)) {
Review Comment:
**`static` `@Bean` methods escape this check on a proxied host.**
The early return treats any host that reaches `@Configuration` with
`proxyBeanMethods` left on as safe, but Spring's CGLIB subclass cannot
intercept a `static` `@Bean` method — proxying is method overriding, and static
methods are not overridable. So on a plain `@Configuration` host, a
`.staticMethod()` bean called from a sibling still constructs a second
instance, silently, which is precisely the failure this diagnostic exists to
catch.
```groovy
@Configuration
@GrailsBeans
class C {
def beans = {
bean('a', A).staticMethod() { new A() }
bean('b', B) { new B(a()) } // second A, no diagnostic
}
}
```
`beanMethodsAreProxied(host)` returns `true` here, so
`rejectUnproxiedSiblingBeanCalls` returns without inspecting anything.
Suggestion: keep the early return for the non-static case, but always flag
calls whose target carries `Modifier.STATIC` — e.g. build `beanMethodsByName`
first, and when the host is proxied narrow it to the static bean methods rather
than returning.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -878,6 +890,118 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
classNode.addMethod(beanMethod);
}
+ // The methods this block just generated, in declaration order: everything
on the host that was
+ // not there before the two processing loops ran. MethodNode does not
override equals, so the
+ // removal is by identity and cannot drop a same-signature method the user
wrote.
+ private List<MethodNode> generatedMembers(ClassNode host, List<MethodNode>
preExisting) {
+ List<MethodNode> generated = new ArrayList<>(host.getMethods());
+ generated.removeAll(preExisting);
+ return generated;
+ }
+
+ /**
+ * Rejects a call from one generated method to another generated {@code
@Bean} method, on a host
+ * whose bean methods Spring does not proxy.
+ *
+ * <p>Calling a sibling {@code @Bean} method and getting the singleton
back is a CGLIB trick, and
+ * Spring only plays it for a full {@code @Configuration} class. On a
<i>lite</i> configuration
+ * source the same call is a plain Java call that constructs a second
instance - and lite is the
+ * common case for this DSL: {@code @AutoConfiguration} is
+ * {@code @Configuration(proxyBeanMethods = false)}, the sibling generated
for a plugin descriptor
+ * carries exactly that, and a Grails {@code Application} class is a
configuration source without
+ * being annotated {@code @Configuration} at all.</p>
+ *
+ * <p>Nothing about that failure is visible at runtime. 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. It is also the
exact mistake a
+ * migration invites, since moving bean methods off a real {@code
@Configuration} class into this
+ * DSL silently changes what those calls mean.</p>
+ */
+ private void rejectUnproxiedSiblingBeanCalls(ClassNode host,
List<MethodNode> generated, SourceUnit source) {
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ for (MethodNode method : generated) {
+ if
(!method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty() || beanMethodsAreProxied(host)) {
+ return;
+ }
+ for (MethodNode method : generated) {
+ if (method.getCode() == null) {
+ continue;
+ }
+ MethodNode caller = method;
+ method.getCode().visit(new CodeVisitorSupport() {
+ // Deliberately not descending. Inside a closure an
unqualified call is resolved
+ // against the delegate first, so `new Registry().tap {
initialize() }` calls the
+ // registry - not this class - even though the AST records
implicit-this either way.
+ // Reading that as a sibling bean call would reject working
code, which is a far
+ // worse trade than missing the rare bean call written inside
a nested closure.
+ @Override
+ public void visitClosureExpression(ClosureExpression
expression) {
+ }
+
+ @Override
+ public void visitMethodCallExpression(MethodCallExpression
call) {
+ super.visitMethodCallExpression(call);
+ if (!isSelfCall(call)) {
+ return;
+ }
+ MethodNode target =
beanMethodsByName.get(call.getMethodAsString());
+ if (target == null || target == caller) {
+ return;
+ }
+ addError(call, source, "\"" + call.getMethodAsString() +
"(...)\" is another bean declared " +
+ "in this block, and " +
host.getNameWithoutPackage() + " is not a proxied " +
+ "@Configuration class, so this call does not
return the bean Spring registered - it " +
+ "constructs a second instance. Inject it instead,
by declaring it as a parameter of " +
+ "this closure; if what you want is shared logic
rather than the bean, move it into a " +
+ "method(...) declaration.");
+ }
+ });
+ }
+ }
+
+ // An unqualified call, or one written against this. Anything with a real
receiver is somebody
+ // else's method that happens to share the name.
+ private boolean isSelfCall(MethodCallExpression call) {
+ return call.isImplicitThis() ||
+ (call.getObjectExpression() instanceof VariableExpression &&
+ ((VariableExpression)
call.getObjectExpression()).isThisExpression());
+ }
+
+ // Whether Spring will CGLIB-proxy this host's @Bean methods: true only
when @Configuration is
+ // reachable from the class's own annotations without passing through one
that sets
+ // proxyBeanMethods = false. @AutoConfiguration answers false through that
second clause - its
+ // meta-annotation is @Configuration(proxyBeanMethods = false) - and a
Grails Application class
+ // answers false by carrying no @Configuration at all.
+ private boolean beanMethodsAreProxied(ClassNode host) {
+ return proxiesBeanMethods(host.getAnnotations(), new HashSet<>());
+ }
+
+ private boolean proxiesBeanMethods(List<AnnotationNode> annotations,
Set<String> visited) {
+ for (AnnotationNode annotation : annotations) {
+ ClassNode type = annotation.getClassNode();
+ if (type.getName().startsWith("java.lang.annotation.") ||
!visited.add(type.getName())) {
Review Comment:
**The `visited` set memoizes branches that were pruned, not explored.**
`visited.add(type.getName())` happens here, *before* the `proxyBeanMethods =
false` skip on the next line. So a `@Configuration` first reached through a
non-proxying branch is recorded as visited and then `continue`d past — and a
genuinely proxying `@Configuration` encountered later is skipped by this same
`!visited.add(...)` guard, making the walk answer `false` for a class Spring
does proxy.
Concretely, a host annotated `@AutoConfiguration @Configuration` (or any
pair where one composed annotation meta-annotates
`@Configuration(proxyBeanMethods = false)` and the other is a real proxying
`@Configuration`), with the non-proxying one listed first:
1. `@AutoConfiguration` → recurse → `@Configuration(proxyBeanMethods =
false)`: `visited.add("org.springframework.context.annotation.Configuration")`
succeeds, then `continue` on the false constant. Returns `false`.
2. Back at the top level, the author's own `@Configuration` hits
`!visited.add(...)` → `continue`.
3. Result: `false`, and every sibling bean call on that class is rejected as
a compile error even though Spring returns the singleton.
Admittedly an uncommon annotation pairing, but the memoization is wrong in
principle: only record a type in `visited` on the path where it is actually
descended into.
--
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]
