[
https://issues.apache.org/jira/browse/GROOVY-12726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140462#comment-18140462
]
ASF GitHub Bot commented on GROOVY-12726:
-----------------------------------------
paulk-asert commented on PR #3262:
URL: https://github.com/apache/groovy/pull/3262#issuecomment-6715916457
Thanks, this is a good catch. I reproduced the cases on master with `groovyc
-j` and here's where I landed.
**Confirmed and important (item 1).** In joint compilation, a class with
`@ToString` plus a `_toString()` helper ends up with `toString()` returning
`''`; likewise `hashCode()` returns `0` and `equals` is always `false` (`new
B(v:1) == new B(v:1)` is false). Plain compilation is unaffected. Moving the
`isStub` check ahead of the helper check is the right fix, and I traced the
remaining combinations (user-written method with/without helper, generated
non-stub method) — behaviour is unchanged apart from the placeholder now being
completed. Since the stubbers shipped in 6.0.0 and `GROOVY_6_0_X` has the same
code, this should be backported.
**Confirmed (item 4).** A Java class calling `addPropertyChangeListener` on
a Groovy class that only uses field-level `@Bindable` fails with `cannot find
symbol` on master. The new `classReceivingListenerStubs` rules (non-static,
non-final, a property) match what the full transform accepts, the duplicate
guards cover multiple annotated fields, and every field-level path (including
`@Bindable` + `@Vetoable` via `VetoableASTTransformation.createListenerSetter`)
reaches the placeholder removal. Item 3 is what makes this safe.
**Item 2 (immutable indexed setter).** I couldn't reproduce this end to end:
with the exact `@MyImmutable(defaults = false, propertyHandler =
ImmutablePropertyHandler)` setup from
`IndexedPropertyJointCompilationStubTest`, master has no `setItems(int,
String)` in either the stub or the class file. The removal is harmless and
reasonable as a safeguard, but the JIRA description should say it was only
shown with the hand-built AST in `IndexedPropertyStubHandoffTest`.
**Records.** While checking the `@RecordType`/`@RecordBase` change I found
that the compiled record *does* get `setItems(int, V)` from `@IndexedProperty`,
in plain compilation as well as joint — so as it stands the stub and the class
file disagree. That's a pre-existing bug, now GROOVY-12730 (fix for 7.0, since
it removes a public method). With 12730 in place the stubber change here is
consistent, so I'd keep it. Could you add a class-file assertion for
`RecordBox`/`BaseBox` alongside the stub assertion (as the test already does
for `ImmutableBox`) once 12730 lands? For the 6.0.x backport, the record part
can stay: the stub would offer one method fewer than the class file has, and
Java code can't call a method it can't see, so nothing breaks at runtime.
A few smaller things:
1. The Bindable/Vetoable stubber Javadoc now says "Unrelated stubber
placeholders on the same class are left in place", but `addListenerToClass`
(class-level path) still removes *every* stub-tagged method. That's harmless
today — completed placeholders are untagged via `clearStub`, and the other
transforms regenerate anything missing — but either narrow that removal to the
listener placeholders too, or adjust the sentence.
2. The placeholder removal now lives inside `protected
needsPropertyChangeSupport` / `needsVetoableChangeSupport`. A subclass
overriding either would silently skip it, and a "needs…" query changing the
class is surprising. Calling the removal from the callers would keep those
methods side-effect free.
3. The literal-to-constant changes in Bindable/Vetoable (~40 lines) and the
`IndexedPropertyASTTransformation` restructuring make the fix harder to review
and to backport. If they're there for Sonar, could they go in a separate commit?
4. Please squash to a single commit before merge. On the JIRA, Affects
Version 6.0.0, Fix Versions 6.0.x and 7.0.0, and a component would help.
With those addressed I'm +1 to merge to master and backport to 6.0.x.
> Fix joint-compilation stubber placeholder cleanup and field-level
> @Bindable/@Vetoable stubbing
> ----------------------------------------------------------------------------------------------
>
> Key: GROOVY-12726
> URL: https://issues.apache.org/jira/browse/GROOVY-12726
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Assignee: Daniel Sun
> Priority: Major
>
> h2. Overview
> During joint compilation, stubber AST transformations
> ({{ToStringASTStubber}}, {{EqualsAndHashCodeASTStubber}},
> {{IndexedPropertyASTStubber}}, {{BindableASTStubber}}, and
> {{VetoableASTStubber}}) run in the {{CONVERSION}} phase to insert placeholder
> {{MethodNode}} stubs (marked via {{StubberSupport}}) so that the Java stub
> generator can emit method signatures before the full AST transformations run
> in later phases. Several edge cases in the handoff between
> {{CONVERSION}}-phase stubbers and their corresponding full AST
> transformations can leave dummy stub methods in final bytecode or omit
> required methods from Java stubs.
> h2. Problem Description
> h3. 1. {{@ToString}} and {{@EqualsAndHashCode}} leak dummy stub methods when
> {{_toString}}, {{_hashCode}}, or {{_equals}} is declared
> In {{ToStringASTTransformation.createToString}} and
> {{EqualsAndHashCodeASTTransformation}} ({{createHashCode}} and
> {{createEquals}}), the presence of a {{CONVERSION}}-phase stub sets
> {{hasExistingToString}}, {{hasExistingHashCode}}, or {{hasExistingEquals}} to
> {{true}}. However, the transformations checked {{hasDeclaredMethod(cNode,
> UNDER_TO_STRING, 0)}} (and {{UNDER_HASH_CODE}} / {{UNDER_EQUALS}}) and
> returned early *before* checking {{StubberSupport.isStub(...)}}:
> {code:java}
> if (hasExistingToString) {
> if (hasDeclaredMethod(cNode, UNDER_TO_STRING, 0)) return;
> MethodNode toString = cNode.getDeclaredMethod(TO_STRING,
> Parameter.EMPTY_ARRAY);
> if (StubberSupport.isStub(toString)) {
> stubberPlaceholder = toString;
> hasExistingToString = false;
> }
> ...
> }
> {code}
> As a result, if a class annotated with {{@ToString}} or
> {{@EqualsAndHashCode}} also declared a helper method ({{_toString()}},
> {{_hashCode()}}, or {{_equals(Object)}}), the transformation returned early
> during {{CANONICALIZATION}} without replacing the stubber placeholder,
> leaving the dummy stub implementation ({{return ""}}, {{return 0}}, or
> {{return false}}) in the compiled class.
> h3. 2. {{@IndexedProperty}} leaks setter stubs on immutable classes
> {{IndexedPropertyASTStubber}} runs in {{CONVERSION}}, before meta-annotations
> (such as a custom {{@AnnotationCollector}} expanding to {{@ImmutableBase}})
> are expanded in {{SEMANTIC_ANALYSIS}}, and therefore emits both {{getX(int)}}
> and {{setX(int, V)}} placeholders. Later, in {{CANONICALIZATION}},
> {{IndexedPropertyASTTransformation}} detects {{IMMUTABLE_BREADCRUMB}} on the
> field and skips {{addArraySetter}} / {{addListSetter}}, but did not remove
> the {{CONVERSION}}-phase setter placeholder, leaving a no-op {{setX(int, V)}}
> method in the bytecode of the immutable class. In addition,
> {{IndexedPropertyASTStubber.isClassImmutable}} did not check for
> {{@RecordType}} or {{@RecordBase}}.
> h3. 3. {{BindableASTTransformation}} and {{VetoableASTTransformation}} do not
> strip stub placeholders in {{needsPropertyChangeSupport}} /
> {{needsVetoableChangeSupport}}
> {{BindableASTTransformation.needsPropertyChangeSupport}} and
> {{VetoableASTTransformation.needsVetoableChangeSupport}} scan
> {{declaringClass.getMethods()}} for existing listener registration and firing
> methods without first removing {{StubberSupport.isStub(m)}} placeholders,
> which can cause them to treat stubber placeholders as user-supplied
> implementations.
> h3. 4. {{BindableASTStubber}} and {{VetoableASTStubber}} ignore field-level
> {{@Bindable}} and {{@Vetoable}} annotations
> {{@Bindable}} and {{@Vetoable}} can be applied to individual fields as well
> as classes, and both forms generate class-level listener methods
> ({{addPropertyChangeListener}}, {{addVetoableChangeListener}}, etc.).
> However, {{BindableASTStubber}} and {{VetoableASTStubber}} returned early
> whenever {{!(parent instanceof ClassNode)}}, so Java sources in a joint
> compilation unit could not call listener methods on Groovy classes that only
> used field-level {{@Bindable}} or {{@Vetoable}}.
> h2. Solution
> * Check {{StubberSupport.isStub(...)}} before checking for {{_toString}},
> {{_hashCode}}, and {{_equals}} in {{ToStringASTTransformation}} and
> {{EqualsAndHashCodeASTTransformation}} so stubber placeholders are always
> replaced with the generated implementation.
> * Remove any stubber-generated indexed setter placeholder in
> {{IndexedPropertyASTTransformation}} when {{immutable}} is {{true}}, and
> include {{@RecordType}} and {{@RecordBase}} in
> {{IndexedPropertyASTStubber.isClassImmutable}}.
> * Remove stubber placeholders at the start of
> {{BindableASTTransformation.needsPropertyChangeSupport}} and
> {{VetoableASTTransformation.needsVetoableChangeSupport}}.
> * Update {{BindableASTStubber}} and {{VetoableASTStubber}} to also emit
> listener method stubs on the declaring class when {{@Bindable}} or
> {{@Vetoable}} annotates a non-static, non-final property field.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
