jdaugherty commented on code in PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#discussion_r3964421822
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -913,35 +1660,221 @@ private Statement synthesizedConstruction(ClassNode
beanType, Parameter[] parame
return returnStatement;
}
- private String syntheticBeanMethodName(ClassNode beanType, Set<String>
usedNames) {
- String base = decapitalize(beanType.getNameWithoutPackage());
- String candidate;
- int index = 0;
- do {
- candidate = base + "$" + index;
- index++;
- }
- while (usedNames.contains(candidate));
- return candidate;
+ /**
+ * Whether {@code candidate} is a {@code target}. Used for the
implementation type, where
+ * checking it here rather than leaving it to the generated {@code return
new Impl()} means the
+ * failure names both types and points at the {@code bean(...)} statement
instead of surfacing as
+ * an assignment error inside a body the author never wrote.
+ */
+ private boolean isSubtypeOf(ClassNode candidate, ClassNode target) {
+ ClassNode resolved = target.redirect();
+ return candidate.redirect().equals(resolved) ||
candidate.isDerivedFrom(resolved) ||
+ candidate.implementsInterface(resolved);
}
- private void processFieldStatement(ClassNode classNode,
MethodCallExpression baseCall,
- List<MethodCallExpression> qualifierCalls, SourceUnit source,
Set<String> usedNames) {
- List<Expression> baseArgs = flatten(baseCall.getArguments());
- TypeAndName typeAndName = parseNameAndType(baseArgs, baseCall, source,
FIELD_CALL, true);
- if (typeAndName == null) {
- return;
+ private boolean hasExplicitTypeArguments(List<MethodCallExpression>
qualifierCalls) {
+ for (MethodCallExpression qualifierCall : qualifierCalls) {
+ if (TYPE_ARGUMENTS_CALL.equals(qualifierCall.getMethodAsString()))
{
+ return true;
+ }
}
- if (!registerName(typeAndName.name, baseCall, source, usedNames,
+ return false;
+ }
+
+ /**
+ * The type a factory closure constructs, when its body is exactly that
and nothing else: a last
+ * statement that is a {@code new ...} expression, which in Groovy is the
closure's return value.
+ * Anything else - a local, a method call, a conditional - is not evidence
of anything, and this
+ * returns null rather than guess.
+ */
+ private ClassNode constructedTypeFromBody(ClosureExpression factory) {
+ if (factory == null || !(factory.getCode() instanceof BlockStatement))
{
+ return null;
+ }
+ List<Statement> statements = ((BlockStatement)
factory.getCode()).getStatements();
+ if (statements.isEmpty()) {
+ return null;
+ }
+ Statement last = statements.get(statements.size() - 1);
+ Expression expression = null;
+ if (last instanceof ReturnStatement) {
+ expression = ((ReturnStatement) last).getExpression();
+ }
+ else if (last instanceof ExpressionStatement) {
+ expression = ((ExpressionStatement) last).getExpression();
+ }
+ return expression instanceof ConstructorCallExpression ?
expression.getType() : null;
+ }
+
+ /**
+ * The declared type parameterized by what {@code evidence} binds it to,
or null when that cannot
+ * be answered concretely - {@code evidence} is unrelated, the declared
type is not generic, or
+ * the binding is itself a type variable ({@code class Box<T> implements
Holder<T>} proves
+ * nothing about a {@code Holder} bean). Inference only ever adds
information the compiler could
+ * already see; where it cannot, the raw type stands exactly as before and
+ * {@code .typeArguments(...)} remains the way to say it.
+ */
+ private ClassNode inferTypeArguments(ClassNode declaredRaw, ClassNode
evidence) {
+ GenericsType[] declared = declaredRaw.redirect().getGenericsTypes();
+ if (evidence == null || declared == null || declared.length == 0) {
+ return null;
+ }
+ if (!isSubtypeOf(evidence, declaredRaw)) {
+ return null;
+ }
+ // A raw construction of a generic type proves nothing: Groovy
resolves its parameters to
+ // their bounds, so new GenericBox() would infer Holder<Object> - not
merely uninformative
+ // but wrong, since a bean typed Holder<Object> no longer matches a
Holder<String> injection
+ // point it previously did as a raw Holder.
+ GenericsType[] evidenceParameters =
evidence.redirect().getGenericsTypes();
+ if (evidenceParameters != null && evidenceParameters.length > 0 &&
+ (evidence.getGenericsTypes() == null ||
evidence.getGenericsTypes().length == 0)) {
+ return null;
+ }
+ ClassNode parameterized;
+ try {
+ parameterized = GenericsUtils.parameterizeType(evidence,
declaredRaw.redirect());
+ }
+ catch (RuntimeException ignored) {
+ // parameterizeType is best-effort on partially resolved
hierarchies; an unusable answer
+ // is the same as no answer.
+ return null;
+ }
+ GenericsType[] resolved = parameterized == null ? null :
parameterized.getGenericsTypes();
+ if (resolved == null || resolved.length != declared.length) {
+ return null;
+ }
+ for (GenericsType candidate : resolved) {
+ if (candidate.isPlaceholder() || candidate.isWildcard() ||
candidate.getType() == null ||
+ candidate.getType().isGenericsPlaceHolder()) {
+ return null;
+ }
+ }
+ return GenericsUtils.makeClassSafeWithGenerics(declaredRaw, resolved);
+ }
+
+ /**
+ * Re-homes anonymous inner classes in a body lifted out of the {@code
beans} closure.
+ *
+ * <p>Groovy's {@code InnerClassVisitor} runs at SEMANTIC_ANALYSIS, before
this transform, and
+ * gives an anonymous class its enclosing instance from wherever it was
written: a class
+ * declared inside a closure gets {@code final Closure this$0} and a
constructor taking a
+ * {@code Closure}, where one declared in a method gets the declaring
class. Lifting the body
+ * into a method moves the code and not that decision, so the generated
call passes {@code this}
+ * - the configuration class - to a constructor still expecting the
closure. It compiles, and
+ * fails at runtime with a {@code GroovyCastException} naming neither the
bean nor the DSL.</p>
+ *
+ * <p>So the three places that decision landed are corrected here: the
{@code this$0} field, the
+ * synthetic constructor's first parameter, and the argument at the call
site.</p>
+ *
+ * <p>What this cannot correct is the anonymous class's <i>outer
class</i>, which
+ * {@code InnerClassNode} fixes at construction. On a plugin descriptor
the members move to a
+ * sibling while the anonymous class stays homed on the descriptor, so the
MOP dispatch methods
+ * Groovy generates for it read a {@code this$0} typed as the descriptor
while the field now
+ * holds the sibling. An anonymous class that touches only its own members
and what it inherits
+ * is fine; one that reaches a member declared in the block is rejected by
+ * {@link #rejectAnonymousClassReachingMovedMembers}, rather than left to
fail with
+ * {@code NoSuchFieldError} inside a running application.</p>
+ */
+ private boolean rehomeAnonymousInnerClasses(Statement body, ClassNode
host, boolean staticMethod,
+ String beanName, SourceUnit source) {
+ List<ConstructorCallExpression> anonymous = new ArrayList<>();
+ body.visit(new CodeVisitorSupport() {
+ // Deliberately not descending. The lift moves the closure's own
body into a method, so
+ // an anonymous class written directly in it loses the closure it
was homed against -
+ // that is what this repairs. One written inside a NESTED closure
does not: that closure
+ // survives the lift and is still its enclosing instance, so
re-homing it would rewrite a
+ // correct `this` into the configuration class and fail at runtime
with the very
+ // GroovyCastException this method exists to prevent - or, under
.staticMethod(), reject
+ // a body that has an enclosing instance and compiles perfectly
well.
+ @Override
+ public void visitClosureExpression(ClosureExpression expression) {
+ }
+
+ @Override
+ public void
visitConstructorCallExpression(ConstructorCallExpression call) {
+ if (call.isUsingAnonymousInnerClass()) {
+ anonymous.add(call);
+ }
+ super.visitConstructorCallExpression(call);
+ }
+ });
+ for (ConstructorCallExpression call : anonymous) {
+ ClassNode inner = call.getType();
Review Comment:
Re-typing `this$0` is now only half of what the parser would have done for a
class written here. `AstBuilder` sets `enclosingMethod` on every anonymous
class declared inside a method body (`anonymousInnerClassList.forEach(e ->
e.setEnclosingMethod(methodNode))`); a `beans` property initializer has no
method, so the classes lifted here arrive in a method with `enclosingMethod ==
null`. Groovy tolerates that as long as the class is type-checked *after* the
constructor call that creates it, which is the order a hand-written class
always gets. The deferral in 902f082 reverses it on a plugin descriptor: the
descriptor's own `@CompileStatic` now runs first at INSTRUCTION_SELECTION,
visits the anonymous class as one of the descriptor's inner classes and marks
it, and when the deferred sibling pass then reaches the lifted call,
`StaticCompilationVisitor.visitConstructorCallExpression` takes its
already-visited branch and does
`anonType.getEnclosingMethod().getNodeMetaData(STATIC_COMPILE_N
ODE)`.
```groovy
@GrailsBeans
@CompileStatic
class ProbeGrailsPlugin extends Plugin {
def beans = {
bean('greeter', Greeter) { new Greeter() { String greet() { 'hello'
} } }
}
}
```
compiles and runs at cf68805 and fails at the head with `GroovyBugError:
BUG! exception in phase 'instruction selection' ...
"ClassNode.getEnclosingMethod()" is null`. The spec's `an anonymous inner class
works under @CompileStatic too` covers this shape on a plain host only, which
is why it stayed green.
A `group(...)` body that constructs an anonymous class hits the same line on
either host kind, with no deferral involved: the host's own pass visits its
inner classes in registration order, so the anonymous class (registered at
parse) is marked before the group (registered here) has its bean method
visited. Closure-only group bodies compile now; it is the anonymous class
specifically.
Setting the enclosing method here fixes all four shapes:
`inner.setEnclosingMethod(liftedMethod)` for each class found directly in the
body, with the bean or helper `MethodNode` threaded in from the two call sites.
I verified that locally - the four probes compile and run and the spec stays at
313 green. The bytecode consequence is the `EnclosingMethod` attribute naming
the sibling's (or group's) bean method and the class becoming a local class in
the `InnerClasses` table, which is what `this$0` already says.
Please add coverage for a `@CompileStatic` plugin descriptor with an
anonymous class in a top-level bean body, and for a `group(...)` bean body
constructing one, on both host kinds.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -875,9 +1208,423 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
}
}
+ if (!rejectNonStaticPostProcessor(beanMethod, beanType, baseCall,
source)) {
+ return;
+ }
+
+ if (!rehomeAnonymousInnerClasses(beanBody, classNode,
Modifier.isStatic(beanMethod.getModifiers()),
+ typeAndName.name, source)) {
+ return;
+ }
+
classNode.addMethod(beanMethod);
}
+ private static final String BEAN_FACTORY_POST_PROCESSOR =
"org.springframework.beans.factory.config.BeanFactoryPostProcessor";
+ private static final String BEAN_POST_PROCESSOR =
"org.springframework.beans.factory.config.BeanPostProcessor";
+
+ /**
+ * A {@code BeanFactoryPostProcessor}/{@code BeanPostProcessor} bean must
be creatable without
+ * instantiating its declaring class, because Spring has to obtain it
before the ordinary bean
+ * lifecycle it participates in has started. Declared as an instance
method it still "works",
+ * which is the problem: the configuration class is instantiated far too
early, taking every bean
+ * its methods depend on with it, out of order and past the
post-processors that would have
+ * configured them - a class of startup bug that shows up as an unrelated
bean being unconfigured
+ * rather than as anything pointing here.
+ *
+ * <p>{@code .staticMethod()} is the fix and is already in the DSL; this
only stops the mistake
+ * being silent. An instance-bound post-processor, if one is genuinely
wanted, is still writable
+ * as an ordinary {@code @Bean} method on the same class - the block does
not claim them.</p>
+ */
+ private boolean rejectNonStaticPostProcessor(MethodNode beanMethod,
ClassNode beanType,
+ ASTNode location, SourceUnit source) {
+ if (Modifier.isStatic(beanMethod.getModifiers())) {
+ return true;
+ }
+ String postProcessorType = null;
+ if (isSubtypeOf(beanType,
ClassHelper.make(BEAN_FACTORY_POST_PROCESSOR))) {
+ postProcessorType = "BeanFactoryPostProcessor";
+ }
+ else if (isSubtypeOf(beanType, ClassHelper.make(BEAN_POST_PROCESSOR)))
{
+ postProcessorType = "BeanPostProcessor";
+ }
+ if (postProcessorType == null) {
+ return true;
+ }
+ addError(location, source, "a " + postProcessorType + " bean must be
declared " +
+ ".staticMethod(), so Spring can obtain it without
instantiating this class - as an " +
+ "instance method it forces that instantiation before the beans
it post-processes " +
+ "are configured");
+ return false;
+ }
+
+ /**
+ * Writes the members this block generated to {@code
-Dgrails.beans.dsl.dumpdir=<dir>}, one file
+ * per host class.
+ *
+ * <p>Everything the DSL decides that the source does not say is a
declaration, not a body: the
+ * bean name Spring will resolve by, the annotations the qualifiers
became, the modifiers, the
+ * declared type and whether it ended up carrying type arguments, and the
parameter annotations
+ * that make a dependency optional or qualified. Bodies are excluded on
purpose - a bean body is
+ * the author's own closure body, lifted verbatim, so it is already
readable where they wrote it.
+ *
+ * <p>Without this, the only way to see any of it is {@code javap} on the
compiled class, which
+ * is a poor place to answer "did that qualifier attach anything" while
writing the block.
+ * Grails already takes this shape for its other compile-time generator,
where
+ * {@code grails.views.gsp.keepgenerateddir} keeps the Groovy a GSP
compiles to.</p>
+ */
+ private void dumpGeneratedMembers(ClassNode host, List<MethodNode>
methods, List<FieldNode> fields,
+ SourceUnit source) {
+ String dir = System.getProperty(DUMP_DIR_PROPERTY);
+ if (dir == null || dir.isBlank()) {
+ return;
+ }
+ StringBuilder text = new StringBuilder();
+ text.append("// Generated from the 'beans' DSL in
").append(host.getName()).append('\n');
+ text.append("// Bodies are omitted: each is the closure body from that
source, lifted verbatim.\n");
+ for (FieldNode field : fields) {
+ text.append('\n');
+ for (AnnotationNode annotation : field.getAnnotations()) {
+ text.append(annotationText(annotation)).append('\n');
+ }
+
text.append(AstToTextHelper.getModifiersText(field.getModifiers())).append(' ')
+ .append(typeText(field.getType())).append('
').append(field.getName()).append('\n');
+ }
+ for (MethodNode method : methods) {
+ text.append('\n');
+ for (AnnotationNode annotation : method.getAnnotations()) {
+ text.append(annotationText(annotation)).append('\n');
+ }
+
text.append(AstToTextHelper.getModifiersText(method.getModifiers())).append(' ')
+ .append(typeText(method.getReturnType())).append('
').append(method.getName())
+
.append('(').append(parametersText(method.getParameters())).append(")\n");
+ }
+ try {
+ Path target = Paths.get(dir);
+ Files.createDirectories(target);
+ Files.writeString(target.resolve(host.getName() + ".beans.txt"),
text.toString(),
+ StandardCharsets.UTF_8);
+ }
+ catch (IOException | RuntimeException e) {
+ // Opt-in by definition, so this can only fire for someone who
asked for the dump and
+ // would otherwise be left looking for a file that was never
written.
+ addError(host, source, "could not write the beans DSL dump for " +
host.getName() + " to \"" +
+ dir + "\" (" + DUMP_DIR_PROPERTY + "): " + e);
+ }
+ }
+
+ private String parametersText(Parameter[] parameters) {
+ StringBuilder text = new StringBuilder();
+ for (Parameter parameter : parameters) {
+ if (text.length() > 0) {
+ text.append(", ");
+ }
+ for (AnnotationNode annotation : parameter.getAnnotations()) {
+ text.append(annotationText(annotation)).append(' ');
+ }
+ text.append(typeText(parameter.getType())).append('
').append(parameter.getName());
+ }
+ return text.toString();
+ }
+
+ // Type arguments are printed only when every one of them is concrete. A
raw declared type
+ // resolved from a class still reports its own type PARAMETERS here, and
printing those would
+ // read as <String> when nothing of the sort was declared.
+ private String typeText(ClassNode type) {
+ GenericsType[] generics = type.getGenericsTypes();
+ if (generics == null || generics.length == 0) {
+ return type.getName();
+ }
+ StringBuilder text = new StringBuilder(type.getName());
+ for (GenericsType generic : generics) {
+ if (generic.isPlaceholder() || generic.isWildcard()) {
+ return type.getName();
+ }
+ }
+ text.append('<');
+ for (int i = 0; i < generics.length; i++) {
+ text.append(i == 0 ? "" : ",
").append(generics[i].getType().getName());
+ }
+ return text.append('>').toString();
+ }
+
+ private String annotationText(AnnotationNode annotation) {
+ StringBuilder text = new
StringBuilder("@").append(annotation.getClassNode().getNameWithoutPackage());
+ Map<String, Expression> members = annotation.getMembers();
+ if (members.isEmpty()) {
+ return text.toString();
+ }
+ text.append('(');
+ boolean first = true;
+ for (Map.Entry<String, Expression> member : members.entrySet()) {
+ text.append(first ? "" : ", ").append(member.getKey()).append(" =
")
+ .append(memberValueText(member.getValue()));
+ first = false;
+ }
+ return text.append(')').toString();
+ }
+
+ // Expression.getText() renders a String constant bare, so
@DependsOn("names") would print as
+ // value = names and read as an identifier. Quote them, and descend into a
list so an
+ // array-valued attribute reads the way it was written.
+ private String memberValueText(Expression value) {
+ if (value instanceof ConstantExpression && ((ConstantExpression)
value).getValue() instanceof String) {
+ return "\"" + ((ConstantExpression) value).getValue() + "\"";
+ }
+ if (value instanceof ListExpression) {
+ StringBuilder text = new StringBuilder("[");
+ List<Expression> entries = ((ListExpression)
value).getExpressions();
+ for (int i = 0; i < entries.size(); i++) {
+ text.append(i == 0 ? "" : ",
").append(memberValueText(entries.get(i)));
+ }
+ return text.append(']').toString();
+ }
+ return value.getText();
+ }
+
+ // 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>A full {@code @Configuration} class is not wholly exempt: the
interception is CGLIB
+ * subclassing, so it cannot override a {@code static} method, and Spring
documents that calls to
+ * a static {@code @Bean} method are never intercepted - not even there. A
{@code .staticMethod()}
+ * bean is therefore checked on every host, and is the only thing checked
on a proxied one.</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) {
+ boolean proxied = beanMethodsAreProxied(host);
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ // Every @Bean method on the host, not only the ones this block
generated. A class that
+ // mixes hand-written @Bean methods with the DSL is what a migration
looks like midway
+ // through, and a call to one of those from a generated body misses
the singleton in exactly
+ // the same way - more easily, in fact, since it was correct in the
@Configuration class the
+ // beans are being moved out of. Only generated bodies are scanned:
what a hand-written
+ // method does is its author's business, not this transform's.
+ for (MethodNode method : host.getMethods()) {
+ if (method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty())
{
+ continue;
+ }
+ // A proxied host still cannot intercept a .staticMethod() bean:
the interception is
+ // CGLIB subclassing, and a static method cannot be overridden. So
on a full
+ // @Configuration class those are the only sibling calls still
worth rejecting.
+ if (!proxied || method.isStatic()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty()) {
+ 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, siblingCallCause(host,
call.getMethodAsString(), proxied) +
+ ", 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.");
+ }
+ });
+ }
+ }
+
+ // Why this particular call misses the singleton. On a proxied host the
map holds only static
+ // bean methods, so reaching here means the target is one.
+ private String siblingCallCause(ClassNode host, String name, boolean
proxied) {
+ if (proxied) {
+ return "\"" + name + "(...)\" is another bean declared in this
block, and is declared " +
+ ".staticMethod(). A static @Bean method is never
intercepted by the container - not even " +
+ "on a proxied @Configuration class like " +
host.getNameWithoutPackage() + " - because " +
+ "that interception is CGLIB subclassing, which cannot
override a static method";
+ }
+ return "\"" + name + "(...)\" is another bean declared in this block,
and " +
+ host.getNameWithoutPackage() + " is not a proxied
@Configuration class";
+ }
+
+ /**
+ * Rejects an unqualified reference from an anonymous inner class in a
bean body to a member that
+ * moved to the generated sibling.
+ *
+ * <p>Only on a plugin descriptor, and only because the anonymous class
cannot follow. Groovy
+ * fixes an inner class's outer class when it creates the node and offers
no way to move it, so
+ * the class stays homed on the descriptor while the members it wants are
on the sibling. Its MOP
+ * dispatch methods then read a {@code this$0} typed as the descriptor
where the field holds the
+ * sibling, and the reference fails with {@code NoSuchFieldError} inside a
running application -
+ * or, under {@code @CompileStatic}, as a "cannot find matching method"
naming a synthetic class
+ * nobody wrote.</p>
+ *
+ * <p>The test is the narrow one: a name that is <i>both</i> 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 left
alone - it is only the
+ * names that actually moved that cannot be reached.</p>
+ */
+ private void rejectAnonymousClassReachingMovedMembers(ClassNode host,
ClassNode declaringClass,
+ List<MethodNode> generatedMethods, List<FieldNode>
generatedFields, SourceUnit source) {
+ if (host == declaringClass) {
Review Comment:
This early return is right for the sibling, but `group(...)` has the same
problem on every host kind and never reaches this method's walk.
`processGroupStatement` hands the group class to `processBeanStatement`, so
`rehomeAnonymousInnerClasses` re-types `this$0` to `Host$ExtrasConfiguration`
while the anonymous class's outer class is still the host - the mismatch this
method exists to catch, now on a plain host too. Every MOP dispatch out of such
a class fails:
```groovy
@GrailsBeans
@AutoConfiguration
class Host {
def beans = {
group('extras') {
method('suffix', String) { '!' }
bean('greeter', Greeter) { new Greeter() { String greet() {
'hello' + suffix() } } }
}
}
}
```
`greet()` throws `NoSuchFieldError: Class Host$1 does not have member field
'Host this$0'` at runtime. Same result for a group-level `field(...)` read, for
a host-level `method(...)` reached from inside a group, and on a plugin
descriptor, where this method does run but walks only the sibling's own
generated methods, not the group class's. A self-contained anonymous class in a
group works, as it does on the sibling. Under `@CompileStatic` the same
reference is an ordinary type-checking error, which is at least visible.
The reachable set for a group is smaller than for the sibling: the group is
a static nested class with no host instance behind it, so nothing outside the
anonymous class is reachable through `this$0` - not a group member, not a host
member. The check for a group body can therefore skip the `moved` set and
reject any implicit-this call or dynamic variable the anonymous class cannot
resolve on itself or what it inherits, with a message saying so. Please run it
for each group as well as the sibling, with tests for the group-level member,
the host-level member, and a self-contained control.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -875,9 +1208,423 @@ private void processBeanStatement(ClassNode classNode,
MethodCallExpression oute
}
}
+ if (!rejectNonStaticPostProcessor(beanMethod, beanType, baseCall,
source)) {
+ return;
+ }
+
+ if (!rehomeAnonymousInnerClasses(beanBody, classNode,
Modifier.isStatic(beanMethod.getModifiers()),
+ typeAndName.name, source)) {
+ return;
+ }
+
classNode.addMethod(beanMethod);
}
+ private static final String BEAN_FACTORY_POST_PROCESSOR =
"org.springframework.beans.factory.config.BeanFactoryPostProcessor";
+ private static final String BEAN_POST_PROCESSOR =
"org.springframework.beans.factory.config.BeanPostProcessor";
+
+ /**
+ * A {@code BeanFactoryPostProcessor}/{@code BeanPostProcessor} bean must
be creatable without
+ * instantiating its declaring class, because Spring has to obtain it
before the ordinary bean
+ * lifecycle it participates in has started. Declared as an instance
method it still "works",
+ * which is the problem: the configuration class is instantiated far too
early, taking every bean
+ * its methods depend on with it, out of order and past the
post-processors that would have
+ * configured them - a class of startup bug that shows up as an unrelated
bean being unconfigured
+ * rather than as anything pointing here.
+ *
+ * <p>{@code .staticMethod()} is the fix and is already in the DSL; this
only stops the mistake
+ * being silent. An instance-bound post-processor, if one is genuinely
wanted, is still writable
+ * as an ordinary {@code @Bean} method on the same class - the block does
not claim them.</p>
+ */
+ private boolean rejectNonStaticPostProcessor(MethodNode beanMethod,
ClassNode beanType,
+ ASTNode location, SourceUnit source) {
+ if (Modifier.isStatic(beanMethod.getModifiers())) {
+ return true;
+ }
+ String postProcessorType = null;
+ if (isSubtypeOf(beanType,
ClassHelper.make(BEAN_FACTORY_POST_PROCESSOR))) {
+ postProcessorType = "BeanFactoryPostProcessor";
+ }
+ else if (isSubtypeOf(beanType, ClassHelper.make(BEAN_POST_PROCESSOR)))
{
+ postProcessorType = "BeanPostProcessor";
+ }
+ if (postProcessorType == null) {
+ return true;
+ }
+ addError(location, source, "a " + postProcessorType + " bean must be
declared " +
+ ".staticMethod(), so Spring can obtain it without
instantiating this class - as an " +
+ "instance method it forces that instantiation before the beans
it post-processes " +
+ "are configured");
+ return false;
+ }
+
+ /**
+ * Writes the members this block generated to {@code
-Dgrails.beans.dsl.dumpdir=<dir>}, one file
+ * per host class.
+ *
+ * <p>Everything the DSL decides that the source does not say is a
declaration, not a body: the
+ * bean name Spring will resolve by, the annotations the qualifiers
became, the modifiers, the
+ * declared type and whether it ended up carrying type arguments, and the
parameter annotations
+ * that make a dependency optional or qualified. Bodies are excluded on
purpose - a bean body is
+ * the author's own closure body, lifted verbatim, so it is already
readable where they wrote it.
+ *
+ * <p>Without this, the only way to see any of it is {@code javap} on the
compiled class, which
+ * is a poor place to answer "did that qualifier attach anything" while
writing the block.
+ * Grails already takes this shape for its other compile-time generator,
where
+ * {@code grails.views.gsp.keepgenerateddir} keeps the Groovy a GSP
compiles to.</p>
+ */
+ private void dumpGeneratedMembers(ClassNode host, List<MethodNode>
methods, List<FieldNode> fields,
+ SourceUnit source) {
+ String dir = System.getProperty(DUMP_DIR_PROPERTY);
+ if (dir == null || dir.isBlank()) {
+ return;
+ }
+ StringBuilder text = new StringBuilder();
+ text.append("// Generated from the 'beans' DSL in
").append(host.getName()).append('\n');
+ text.append("// Bodies are omitted: each is the closure body from that
source, lifted verbatim.\n");
+ for (FieldNode field : fields) {
+ text.append('\n');
+ for (AnnotationNode annotation : field.getAnnotations()) {
+ text.append(annotationText(annotation)).append('\n');
+ }
+
text.append(AstToTextHelper.getModifiersText(field.getModifiers())).append(' ')
+ .append(typeText(field.getType())).append('
').append(field.getName()).append('\n');
+ }
+ for (MethodNode method : methods) {
+ text.append('\n');
+ for (AnnotationNode annotation : method.getAnnotations()) {
+ text.append(annotationText(annotation)).append('\n');
+ }
+
text.append(AstToTextHelper.getModifiersText(method.getModifiers())).append(' ')
+ .append(typeText(method.getReturnType())).append('
').append(method.getName())
+
.append('(').append(parametersText(method.getParameters())).append(")\n");
+ }
+ try {
+ Path target = Paths.get(dir);
+ Files.createDirectories(target);
+ Files.writeString(target.resolve(host.getName() + ".beans.txt"),
text.toString(),
+ StandardCharsets.UTF_8);
+ }
+ catch (IOException | RuntimeException e) {
+ // Opt-in by definition, so this can only fire for someone who
asked for the dump and
+ // would otherwise be left looking for a file that was never
written.
+ addError(host, source, "could not write the beans DSL dump for " +
host.getName() + " to \"" +
+ dir + "\" (" + DUMP_DIR_PROPERTY + "): " + e);
+ }
+ }
+
+ private String parametersText(Parameter[] parameters) {
+ StringBuilder text = new StringBuilder();
+ for (Parameter parameter : parameters) {
+ if (text.length() > 0) {
+ text.append(", ");
+ }
+ for (AnnotationNode annotation : parameter.getAnnotations()) {
+ text.append(annotationText(annotation)).append(' ');
+ }
+ text.append(typeText(parameter.getType())).append('
').append(parameter.getName());
+ }
+ return text.toString();
+ }
+
+ // Type arguments are printed only when every one of them is concrete. A
raw declared type
+ // resolved from a class still reports its own type PARAMETERS here, and
printing those would
+ // read as <String> when nothing of the sort was declared.
+ private String typeText(ClassNode type) {
+ GenericsType[] generics = type.getGenericsTypes();
+ if (generics == null || generics.length == 0) {
+ return type.getName();
+ }
+ StringBuilder text = new StringBuilder(type.getName());
+ for (GenericsType generic : generics) {
+ if (generic.isPlaceholder() || generic.isWildcard()) {
+ return type.getName();
+ }
+ }
+ text.append('<');
+ for (int i = 0; i < generics.length; i++) {
+ text.append(i == 0 ? "" : ",
").append(generics[i].getType().getName());
+ }
+ return text.append('>').toString();
+ }
+
+ private String annotationText(AnnotationNode annotation) {
+ StringBuilder text = new
StringBuilder("@").append(annotation.getClassNode().getNameWithoutPackage());
+ Map<String, Expression> members = annotation.getMembers();
+ if (members.isEmpty()) {
+ return text.toString();
+ }
+ text.append('(');
+ boolean first = true;
+ for (Map.Entry<String, Expression> member : members.entrySet()) {
+ text.append(first ? "" : ", ").append(member.getKey()).append(" =
")
+ .append(memberValueText(member.getValue()));
+ first = false;
+ }
+ return text.append(')').toString();
+ }
+
+ // Expression.getText() renders a String constant bare, so
@DependsOn("names") would print as
+ // value = names and read as an identifier. Quote them, and descend into a
list so an
+ // array-valued attribute reads the way it was written.
+ private String memberValueText(Expression value) {
+ if (value instanceof ConstantExpression && ((ConstantExpression)
value).getValue() instanceof String) {
+ return "\"" + ((ConstantExpression) value).getValue() + "\"";
+ }
+ if (value instanceof ListExpression) {
+ StringBuilder text = new StringBuilder("[");
+ List<Expression> entries = ((ListExpression)
value).getExpressions();
+ for (int i = 0; i < entries.size(); i++) {
+ text.append(i == 0 ? "" : ",
").append(memberValueText(entries.get(i)));
+ }
+ return text.append(']').toString();
+ }
+ return value.getText();
+ }
+
+ // 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>A full {@code @Configuration} class is not wholly exempt: the
interception is CGLIB
+ * subclassing, so it cannot override a {@code static} method, and Spring
documents that calls to
+ * a static {@code @Bean} method are never intercepted - not even there. A
{@code .staticMethod()}
+ * bean is therefore checked on every host, and is the only thing checked
on a proxied one.</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) {
+ boolean proxied = beanMethodsAreProxied(host);
+ Map<String, MethodNode> beanMethodsByName = new LinkedHashMap<>();
+ // Every @Bean method on the host, not only the ones this block
generated. A class that
+ // mixes hand-written @Bean methods with the DSL is what a migration
looks like midway
+ // through, and a call to one of those from a generated body misses
the singleton in exactly
+ // the same way - more easily, in fact, since it was correct in the
@Configuration class the
+ // beans are being moved out of. Only generated bodies are scanned:
what a hand-written
+ // method does is its author's business, not this transform's.
+ for (MethodNode method : host.getMethods()) {
+ if (method.getAnnotations(ClassHelper.make(Bean.class)).isEmpty())
{
+ continue;
+ }
+ // A proxied host still cannot intercept a .staticMethod() bean:
the interception is
+ // CGLIB subclassing, and a static method cannot be overridden. So
on a full
+ // @Configuration class those are the only sibling calls still
worth rejecting.
+ if (!proxied || method.isStatic()) {
+ beanMethodsByName.put(method.getName(), method);
+ }
+ }
+ if (beanMethodsByName.isEmpty()) {
+ 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, siblingCallCause(host,
call.getMethodAsString(), proxied) +
+ ", 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.");
+ }
+ });
+ }
+ }
+
+ // Why this particular call misses the singleton. On a proxied host the
map holds only static
+ // bean methods, so reaching here means the target is one.
+ private String siblingCallCause(ClassNode host, String name, boolean
proxied) {
+ if (proxied) {
+ return "\"" + name + "(...)\" is another bean declared in this
block, and is declared " +
+ ".staticMethod(). A static @Bean method is never
intercepted by the container - not even " +
+ "on a proxied @Configuration class like " +
host.getNameWithoutPackage() + " - because " +
+ "that interception is CGLIB subclassing, which cannot
override a static method";
+ }
+ return "\"" + name + "(...)\" is another bean declared in this block,
and " +
+ host.getNameWithoutPackage() + " is not a proxied
@Configuration class";
+ }
+
+ /**
+ * Rejects an unqualified reference from an anonymous inner class in a
bean body to a member that
+ * moved to the generated sibling.
+ *
+ * <p>Only on a plugin descriptor, and only because the anonymous class
cannot follow. Groovy
+ * fixes an inner class's outer class when it creates the node and offers
no way to move it, so
+ * the class stays homed on the descriptor while the members it wants are
on the sibling. Its MOP
+ * dispatch methods then read a {@code this$0} typed as the descriptor
where the field holds the
+ * sibling, and the reference fails with {@code NoSuchFieldError} inside a
running application -
+ * or, under {@code @CompileStatic}, as a "cannot find matching method"
naming a synthetic class
+ * nobody wrote.</p>
+ *
+ * <p>The test is the narrow one: a name that is <i>both</i> 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 left
alone - it is only the
+ * names that actually moved that cannot be reached.</p>
+ */
+ private void rejectAnonymousClassReachingMovedMembers(ClassNode host,
ClassNode declaringClass,
+ List<MethodNode> generatedMethods, List<FieldNode>
generatedFields, SourceUnit source) {
+ if (host == declaringClass) {
+ // Not a plugin descriptor: the members and the anonymous class
share a home, so an
+ // unqualified reference resolves the way it reads.
+ return;
+ }
+ Set<String> moved = new HashSet<>();
+ for (MethodNode method : generatedMethods) {
+ moved.add(method.getName());
+ }
+ for (FieldNode field : generatedFields) {
+ moved.add(field.getName());
+ }
+ if (moved.isEmpty()) {
+ return;
+ }
+ for (MethodNode method : generatedMethods) {
+ if (method.getCode() == null) {
+ continue;
+ }
+ List<ConstructorCallExpression> anonymous = new ArrayList<>();
+ method.getCode().visit(new CodeVisitorSupport() {
+ @Override
+ public void
visitConstructorCallExpression(ConstructorCallExpression call) {
+ if (call.isUsingAnonymousInnerClass()) {
+ anonymous.add(call);
+ }
+ super.visitConstructorCallExpression(call);
+ }
+ });
+ for (ConstructorCallExpression call : anonymous) {
+ reportMovedMemberReferences(call, moved, host, source);
+ }
+ }
+ }
+
+ private void reportMovedMemberReferences(ConstructorCallExpression call,
Set<String> moved,
+ ClassNode host, SourceUnit source) {
+ ClassNode inner = call.getType();
+ Set<String> own = existingMemberNames(inner);
+ for (MethodNode method : inner.getMethods()) {
+ if (method.getCode() == null) {
+ continue;
+ }
+ method.getCode().visit(new CodeVisitorSupport() {
+ @Override
+ public void visitMethodCallExpression(MethodCallExpression
inner) {
+ super.visitMethodCallExpression(inner);
+ if (inner.isImplicitThis()) {
+ report(inner.getMethodAsString(), inner, "()");
+ }
+ }
+
+ @Override
+ public void visitVariableExpression(VariableExpression
expression) {
+ super.visitVariableExpression(expression);
+ if (expression.getAccessedVariable() instanceof
DynamicVariable) {
+ report(expression.getName(), expression, "");
+ }
+ }
+
+ private void report(String name, ASTNode at, String
callSuffix) {
+ if (name == null || own.contains(name) ||
!moved.contains(name)) {
Review Comment:
Comparing raw names misses property-style access to a moved accessor.
`method('getSuffix', String) { '!' }` on a descriptor plus `'hello' + suffix`
in the anonymous class compiles and fails with the same `NoSuchFieldError` at
runtime: `suffix` is a `DynamicVariable` here while `moved` holds `getSuffix`.
Deriving the `get`/`is`/`set` names from the variable, as `collectMethodNames`
already does for properties, would close it.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -786,16 +862,178 @@ private void processStatement(ClassNode classNode,
Statement statement, SourceUn
}
if (isBean) {
- processBeanStatement(classNode, outerCall, baseCall,
qualifierCalls, source, usedNames);
+ processBeanStatement(classNode, declaringClass, outerCall,
baseCall, qualifierCalls, source, usedNames);
+ }
+ else if (GROUP_CALL.equals(rootName)) {
+ processGroupStatement(classNode, declaringClass, outerCall,
baseCall, qualifierCalls, source, usedNames);
}
else if (FIELD_CALL.equals(rootName)) {
- processFieldStatement(classNode, baseCall, qualifierCalls, source,
usedNames);
+ processFieldStatement(classNode, declaringClass, baseCall,
qualifierCalls, source, usedNames);
}
else {
- processMethodStatement(classNode, outerCall, baseCall,
qualifierCalls, source, usedNames);
+ processMethodStatement(classNode, declaringClass, outerCall,
baseCall, qualifierCalls, source, usedNames);
}
}
+ /**
+ * Compiles {@code group("name").<conditions> { ... }} into a nested static
+ * {@code @Configuration(proxyBeanMethods = false)} class holding the
declarations in its body,
+ * with the chained qualifiers attached to that class rather than to each
bean.
+ *
+ * <p>This is the shape real auto-configurations take, and the one a
condition on an optional
+ * type has to take. Spring reads a condition from the bytecode before
loading anything, but a
+ * {@code @Bean} method's parameter and return types are resolved 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. Moving it into a nested class moves the guard with it,
and the nested class is
+ * never parsed when the condition fails. Spring Boot writes exactly this:
JacksonAutoConfiguration
+ * carries four nested {@code @ConditionalOnClass} configuration
classes.</p>
+ *
+ * <p>Spring finds the nested class itself - {@code
ConfigurationClassParser} processes the member
+ * classes of a configuration class - so nothing has to import or register
it.</p>
+ */
+ private void processGroupStatement(ClassNode classNode, ClassNode
declaringClass, MethodCallExpression outerCall,
+ MethodCallExpression baseCall, List<MethodCallExpression>
qualifierCalls, SourceUnit source,
+ Set<String> usedNames) {
+ List<Expression> closureCallArgs = flatten(outerCall.getArguments());
+ if (closureCallArgs.isEmpty() ||
!(closureCallArgs.get(closureCallArgs.size() - 1) instanceof
ClosureExpression)) {
+ addError(outerCall, source, "group(...) must end with a body
closure: group(\"name\") { ... }");
+ return;
+ }
+ ClosureExpression body = (ClosureExpression)
closureCallArgs.get(closureCallArgs.size() - 1);
+ if (body.getParameters() != null && body.getParameters().length > 0) {
+ addError(outerCall, source, "group(...) takes no closure
parameters - a group declares a class, " +
+ "not a bean, so there is nothing to inject into. Put the
parameters on the bean(...) " +
+ "declarations inside it");
+ return;
+ }
+
+ List<Expression> baseArgs = flatten(baseCall.getArguments());
+ if (baseCall == outerCall && !baseArgs.isEmpty()) {
+ baseArgs = baseArgs.subList(0, baseArgs.size() - 1);
+ }
+ if (baseArgs.size() != 1) {
+ addError(baseCall, source, "group(...) takes a name, e.g.
group(\"imageServing\") { ... }");
+ return;
+ }
+ String name = resolveStringConstant(baseArgs.get(0), declaringClass);
+ if (name == null ||
!isValidJavaIdentifier(BeanUtils.capitalize(name))) {
+ addError(baseArgs.get(0), source, "group(name) requires the name
to be a String literal or a " +
+ "compile-time String constant that is a valid Java
identifier - it becomes the nested " +
+ "class's name, e.g. group(\"imageServing\")");
+ return;
+ }
+
+ // JacksonObjectMapperConfiguration rather than JacksonObjectMapper:
the suffix is what says
+ // this is a configuration class when it turns up in a stack trace or
/actuator/beans.
+ String simpleName = BeanUtils.capitalize(name);
+ if (!simpleName.endsWith("Configuration")) {
+ simpleName = simpleName + "Configuration";
+ }
+ if (!registerName(simpleName, baseCall, source, usedNames,
+ "is already used by another member of the class - generated
member names must be unique")) {
+ return;
+ }
+
+ List<Statement> statements = beanStatements(body);
+ if (statements.isEmpty()) {
+ addError(outerCall, source, "group(\"" + name + "\") declares
nothing - a group exists to put a " +
+ "condition on the declarations inside it");
+ return;
+ }
+ for (Statement statement : statements) {
+ if (isGroupRootedStatement(statement)) {
+ addError(statement, source, "group(...) cannot be nested -
flatten it, or give the inner " +
+ "group its own conditions at the top level");
+ return;
+ }
+ }
+
+ InnerClassNode group = new InnerClassNode(classNode,
classNode.getName() + "$" + simpleName,
+ Modifier.PUBLIC | Modifier.STATIC, ClassHelper.OBJECT_TYPE);
+ group.setSourcePosition(baseCall);
+ source.getAST().addClass(group);
+
+ // proxyBeanMethods = false, matching what Spring Boot's own nested
configuration classes
+ // carry - and keeping the sibling-call check below meaningful inside
the group.
+ AnnotationNode configuration = new
AnnotationNode(ClassHelper.make(Configuration.class));
+ configuration.setMember(PROXY_BEAN_METHODS_MEMBER, new
ConstantExpression(Boolean.FALSE));
+ group.addAnnotation(withPosition(configuration, baseCall));
+
+ for (MethodCallExpression qualifierCall : qualifierCalls) {
+ List<Expression> qualifierArgs =
flatten(qualifierCall.getArguments());
+ if (qualifierCall == outerCall) {
+ qualifierArgs = qualifierArgs.subList(0, qualifierArgs.size()
- 1);
+ }
+ if (!applyGroupQualifier(group, qualifierCall, qualifierArgs,
source)) {
+ return;
+ }
+ }
+
+ Set<String> groupNames = existingMemberNames(group);
+ List<MethodNode> preExisting = new ArrayList<>(group.getMethods());
+ validateSharedBeanNames(statements, source);
+ for (Statement statement : statements) {
+ if (!isBeanRootedStatement(statement)) {
+ processStatement(group, declaringClass, statement, source,
groupNames);
+ }
+ }
+ for (Statement statement : statements) {
+ if (isBeanRootedStatement(statement)) {
+ processStatement(group, declaringClass, statement, source,
groupNames);
+ }
+ }
+ List<MethodNode> generated = generatedMembers(group, preExisting);
+ rejectUnproxiedSiblingBeanCalls(group, generated, source);
+ dumpGeneratedMembers(group, generated, new
ArrayList<>(group.getFields()), source);
+
+ // The group is compiled as its own class, so it needs the host's
static-compilation
+ // treatment in its own right - otherwise its bodies are dynamic
inside a @CompileStatic file.
+ applyStaticCompilation(classNode, group, source);
Review Comment:
Closure-only group bodies compile under `@CompileStatic` on both host kinds
now. A group body that constructs an anonymous inner class still fails in class
generation, and the deferral moved the same failure onto a `@CompileStatic`
descriptor with an anonymous class in a top-level bean body, which compiled at
cf68805. Root cause and a verified fix are on the re-homing loop in this round.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -913,6 +1514,164 @@ private Statement synthesizedConstruction(ClassNode
beanType, Parameter[] parame
return returnStatement;
}
+ /**
+ * Whether {@code candidate} is a {@code target}. Used for the
implementation type, where
+ * checking it here rather than leaving it to the generated {@code return
new Impl()} means the
+ * failure names both types and points at the {@code bean(...)} statement
instead of surfacing as
+ * an assignment error inside a body the author never wrote.
+ */
+ private boolean isSubtypeOf(ClassNode candidate, ClassNode target) {
+ ClassNode resolved = target.redirect();
+ return candidate.redirect().equals(resolved) ||
candidate.isDerivedFrom(resolved) ||
+ candidate.implementsInterface(resolved);
+ }
+
+ private boolean hasExplicitTypeArguments(List<MethodCallExpression>
qualifierCalls) {
+ for (MethodCallExpression qualifierCall : qualifierCalls) {
+ if (TYPE_ARGUMENTS_CALL.equals(qualifierCall.getMethodAsString()))
{
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * The type a factory closure constructs, when its body is exactly that
and nothing else: a last
+ * statement that is a {@code new ...} expression, which in Groovy is the
closure's return value.
+ * Anything else - a local, a method call, a conditional - is not evidence
of anything, and this
+ * returns null rather than guess.
+ */
+ private ClassNode constructedTypeFromBody(ClosureExpression factory) {
+ if (factory == null || !(factory.getCode() instanceof BlockStatement))
{
+ return null;
+ }
+ List<Statement> statements = ((BlockStatement)
factory.getCode()).getStatements();
+ if (statements.isEmpty()) {
+ return null;
+ }
+ Statement last = statements.get(statements.size() - 1);
+ Expression expression = null;
+ if (last instanceof ReturnStatement) {
+ expression = ((ReturnStatement) last).getExpression();
+ }
+ else if (last instanceof ExpressionStatement) {
+ expression = ((ExpressionStatement) last).getExpression();
+ }
+ return expression instanceof ConstructorCallExpression ?
expression.getType() : null;
+ }
+
+ /**
+ * The declared type parameterized by what {@code evidence} binds it to,
or null when that cannot
+ * be answered concretely - {@code evidence} is unrelated, the declared
type is not generic, or
+ * the binding is itself a type variable ({@code class Box<T> implements
Holder<T>} proves
+ * nothing about a {@code Holder} bean). Inference only ever adds
information the compiler could
+ * already see; where it cannot, the raw type stands exactly as before and
+ * {@code .typeArguments(...)} remains the way to say it.
+ */
+ private ClassNode inferTypeArguments(ClassNode declaredRaw, ClassNode
evidence) {
+ GenericsType[] declared = declaredRaw.redirect().getGenericsTypes();
+ if (evidence == null || declared == null || declared.length == 0) {
+ return null;
+ }
+ if (!isSubtypeOf(evidence, declaredRaw)) {
+ return null;
+ }
+ // A raw construction of a generic type proves nothing: Groovy
resolves its parameters to
+ // their bounds, so new GenericBox() would infer Holder<Object> - not
merely uninformative
+ // but wrong, since a bean typed Holder<Object> no longer matches a
Holder<String> injection
+ // point it previously did as a raw Holder.
+ GenericsType[] evidenceParameters =
evidence.redirect().getGenericsTypes();
+ if (evidenceParameters != null && evidenceParameters.length > 0 &&
+ (evidence.getGenericsTypes() == null ||
evidence.getGenericsTypes().length == 0)) {
+ return null;
+ }
+ ClassNode parameterized;
+ try {
+ parameterized = GenericsUtils.parameterizeType(evidence,
declaredRaw.redirect());
+ }
+ catch (RuntimeException ignored) {
+ // parameterizeType is best-effort on partially resolved
hierarchies; an unusable answer
+ // is the same as no answer.
+ return null;
+ }
+ GenericsType[] resolved = parameterized == null ? null :
parameterized.getGenericsTypes();
+ if (resolved == null || resolved.length != declared.length) {
+ return null;
+ }
+ for (GenericsType candidate : resolved) {
+ if (candidate.isPlaceholder() || candidate.isWildcard() ||
candidate.getType() == null ||
+ candidate.getType().isGenericsPlaceHolder()) {
+ return null;
+ }
+ }
+ return GenericsUtils.makeClassSafeWithGenerics(declaredRaw, resolved);
+ }
+
+ /**
+ * Re-homes anonymous inner classes in a body lifted out of the {@code
beans} closure.
+ *
+ * <p>Groovy's {@code InnerClassVisitor} runs at SEMANTIC_ANALYSIS, before
this transform, and
+ * gives an anonymous class its enclosing instance from wherever it was
written: a class
+ * declared inside a closure gets {@code final Closure this$0} and a
constructor taking a
+ * {@code Closure}, where one declared in a method gets the declaring
class. Lifting the body
+ * into a method moves the code and not that decision, so the generated
call passes {@code this}
+ * - the configuration class - to a constructor still expecting the
closure. It compiles, and
+ * fails at runtime with a {@code GroovyCastException} naming neither the
bean nor the DSL.</p>
+ *
+ * <p>So the three places that decision landed are corrected here: the
{@code this$0} field, the
+ * synthetic constructor's first parameter, and the argument at the call
site.</p>
+ */
+ private boolean rehomeAnonymousInnerClasses(Statement body, ClassNode
host, boolean staticMethod,
+ String beanName, SourceUnit source) {
+ List<ConstructorCallExpression> anonymous = new ArrayList<>();
+ body.visit(new CodeVisitorSupport() {
+ @Override
+ public void
visitConstructorCallExpression(ConstructorCallExpression call) {
+ if (call.isUsingAnonymousInnerClass()) {
+ anonymous.add(call);
+ }
+ super.visitConstructorCallExpression(call);
+ }
+ });
+ for (ConstructorCallExpression call : anonymous) {
+ ClassNode inner = call.getType();
+ FieldNode outerField = inner.getDeclaredField("this$0");
+ if (outerField == null ||
!ClassHelper.CLOSURE_TYPE.equals(outerField.getType())) {
+ continue; // already homed somewhere real, or static - nothing
the lift broke
+ }
+ // A static factory method has no enclosing instance to give it,
and the field cannot be
+ // dropped here: InnerClassVisitor added it and the constructor
body assigns it.
+ if (staticMethod) {
+ addError(call, source, "\"" + beanName + "\" is declared
.staticMethod() and its body " +
+ "constructs an anonymous inner class, which needs an
enclosing instance the " +
+ "static method has not got - give the anonymous class
a name and declare it " +
+ "as a static nested class, or drop .staticMethod()");
+ return false;
+ }
+ ClassNode enclosing = host.getPlainNodeReference();
Review Comment:
The direct-in-body and nested-closure shapes on a descriptor are rejected as
described, and an inherited method is left alone. The same runtime failure
exists for an anonymous class in a `group(...)` body on every host kind, and
this check does not reach it - see the early return 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]
