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. -- 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]
