[
https://issues.apache.org/jira/browse/GROOVY-12726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140242#comment-18140242
]
ASF GitHub Bot commented on GROOVY-12726:
-----------------------------------------
codecov-commenter commented on PR #3262:
URL: https://github.com/apache/groovy/pull/3262#issuecomment-6706713163
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3262?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `54.16667%` with `22 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 74.4011%. Comparing base
([`d020012`](https://app.codecov.io/gh/apache/groovy/commit/d02001286af335c8777fb9f5770e48ec4d07119f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`b9f9efa`](https://app.codecov.io/gh/apache/groovy/commit/b9f9efa593bffa10d5645f2afb807b1bc14ac87a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3262?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[src/main/java/groovy/beans/BindableASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FBindableASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvQmluZGFibGVBU1RTdHViYmVyLmphdmE=)
| 42.8571% | [1 Missing and 3 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[src/main/java/groovy/beans/VetoableASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FVetoableASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvVmV0b2FibGVBU1RTdHViYmVyLmphdmE=)
| 42.8571% | [1 Missing and 3 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...vy/transform/IndexedPropertyASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FIndexedPropertyASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9JbmRleGVkUHJvcGVydHlBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| 66.6667% | [2 Missing and 2 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...us/groovy/transform/IndexedPropertyASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FIndexedPropertyASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9JbmRleGVkUHJvcGVydHlBU1RTdHViYmVyLmphdmE=)
| 0.0000% | [3 Missing :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...n/java/groovy/beans/BindableASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FBindableASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvQmluZGFibGVBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| 75.0000% | [1 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...n/java/groovy/beans/VetoableASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FVetoableASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvVmV0b2FibGVBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| 75.0000% | [1 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[.../transform/EqualsAndHashCodeASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FEqualsAndHashCodeASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9FcXVhbHNBbmRIYXNoQ29kZUFTVFRyYW5zZm9ybWF0aW9uLmphdmE=)
| 0.0000% | [0 Missing and 2 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...us/groovy/transform/ToStringASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FToStringASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9Ub1N0cmluZ0FTVFRyYW5zZm9ybWF0aW9uLmphdmE=)
| 0.0000% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3262 +/- ##
==================================================
+ Coverage 74.4010% 74.4011% +0.0001%
- Complexity 44345 44352 +7
==================================================
Files 1699 1699
Lines 150998 151034 +36
Branches 28770 28779 +9
==================================================
+ Hits 112344 112371 +27
+ Misses 28380 28379 -1
- Partials 10274 10284 +10
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3262?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...us/groovy/transform/ToStringASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FToStringASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9Ub1N0cmluZ0FTVFRyYW5zZm9ybWF0aW9uLmphdmE=)
| `85.2349% <0.0000%> (+0.5682%)` | :arrow_up: |
|
[...n/java/groovy/beans/BindableASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FBindableASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvQmluZGFibGVBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| `91.8518% <75.0000%> (-1.0615%)` | :arrow_down: |
|
[...n/java/groovy/beans/VetoableASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FVetoableASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvVmV0b2FibGVBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| `91.7241% <75.0000%> (-0.9766%)` | :arrow_down: |
|
[.../transform/EqualsAndHashCodeASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FEqualsAndHashCodeASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9FcXVhbHNBbmRIYXNoQ29kZUFTVFRyYW5zZm9ybWF0aW9uLmphdmE=)
| `82.9960% <0.0000%> (+0.9959%)` | :arrow_up: |
|
[...us/groovy/transform/IndexedPropertyASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FIndexedPropertyASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9JbmRleGVkUHJvcGVydHlBU1RTdHViYmVyLmphdmE=)
| `67.5000% <0.0000%> (+6.9737%)` | :arrow_up: |
|
[src/main/java/groovy/beans/BindableASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FBindableASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvQmluZGFibGVBU1RTdHViYmVyLmphdmE=)
| `71.4286% <42.8571%> (-3.5714%)` | :arrow_down: |
|
[src/main/java/groovy/beans/VetoableASTStubber.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fbeans%2FVetoableASTStubber.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvYmVhbnMvVmV0b2FibGVBU1RTdHViYmVyLmphdmE=)
| `72.0930% <42.8571%> (-3.5827%)` | :arrow_down: |
|
[...vy/transform/IndexedPropertyASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/3262?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FIndexedPropertyASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9JbmRleGVkUHJvcGVydHlBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| `80.2817% <66.6667%> (-1.6855%)` | :arrow_down: |
... and [11 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3262/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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)
