codeconsole commented on code in PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#discussion_r3972533969
##########
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:
Fixed in 67d20dc. `inner.setEnclosingMethod(liftedMethod)` alongside the
`this$0` field, the constructor parameter and the call argument the lift
already corrects, with the bean/helper `MethodNode` threaded in from both call
sites.
Coverage added 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, dynamic and `@CompileStatic`. All the `@CompileStatic`
shapes fail with the `GroovyBugError` when the one line is backed out.
##########
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:
Fixed in 191830e. The check runs for each group as well as the sibling, and
the early return moved to the sibling call site.
A group passes no moved set: it is a static nested class with no enclosing
instance behind it, so anything the anonymous class cannot resolve on itself or
what it inherits is rejected. Tests for a group-level member and a host-level
member, on a plain host and on a descriptor, plus a self-contained control and
a captured-local control — the two ways the message names.
##########
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:
Fixed in 635f284. A `DynamicVariable` now matches on the `get`/`is`/`set`
spellings of its name as well as on the name itself, the same set
`collectMethodNames` reserves. Test: `method('getSuffix', String)` plus
`suffix` in the anonymous class body.
--
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]
