codeconsole commented on code in PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#discussion_r3961350269
##########
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:
Valid, and I took the second of your two options — documented and pinned
rather than detected — in 902f082.
You are right that it is not a regression: the shape could not be
constructed before at all. The javadoc did advertise anonymous classes in bean
bodies without the caveat, so both the guide and the re-homing javadoc now
state it: on a plugin descriptor the anonymous class keeps the descriptor as
its outer class, so one that touches nothing outside itself is fine while an
unqualified reference back to a field(...)/method(...) member fails with
NoSuchFieldError at runtime, or is rejected under @CompileStatic. The note
names the ways out — pass what it needs as a constructor argument or a captured
local, or declare the bean somewhere that is not a descriptor.
I stopped short of the compile-time error deliberately. Recognising an
implicit-this reference from an anonymous class body means deciding which names
are the class own, and a false positive there rejects working code — twice in
this PR a check that descended too far did exactly that (the sibling-call check
into tap { }, and the re-homing you caught above). I would rather land the
limitation stated than a check I cannot yet bound. Happy to take it as a
follow-up if you would prefer the error.
##########
grails-beans-dsl/src/main/java/org/grails/compiler/beans/GrailsBeansASTTransformation.java:
##########
@@ -582,8 +635,8 @@ private void validateSharedBeanNames(List<Statement>
statements, SourceUnit sour
addError(use.baseCall, source, "\"" + use.beanName + "\"
is already used as the Spring " +
Review Comment:
Valid — confirmed with a control before fixing, and fixed in 902f082.
Three probes: a duplicate three-argument declaration, a duplicate
constant-named declaration, and a duplicate literal-named one. The first two
compiled; the third was rejected. So the check was intact and precisely the two
new forms had fallen out of it, exactly as you describe.
Routed through the same head parsing, as you suggested: the implementation
split is now one helper both readers call, and the name goes through
resolveStringConstant with the declaring class threaded in — which also means a
constant name resolves the same way in both places rather than by a second copy
of the rules. All three cases are now in the spec, including the literal one as
a control so a future regression cannot pass by rejecting nothing.
--
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]
