Repository navigation
WG012: flag MutableSideEffect with reference-equality value type - #72
Merged
Merged
Conversation
New declarative rule type, mutable-side-effect-equality, alongside the existing forbidden-method: flags Workflow.mutableSideEffect(id, valueClass, updateFunction, func) calls whose value type relies on inherited, identity-based Object.equals(). Since func constructs a new instance every call, equals() never reports "unchanged", so every call appends a new history event regardless of whether the logical value actually changed. RuleDefinition gains a valueTypeArgumentIndex field. A new ValueBasedEqualityArgumentTarget (wogu-temporal.callgraph, reusable) resolves the named argument's Class<T> literal via the symbol solver and checks for value-based equals(): a Java record, a declared or inherited non-Object equals(), or an unresolvable type (outside WoGu's own classpath) are all treated as safe, matching the false-negative- preferring default every other rule uses for unresolvable code. MutableSideEffectEqualityRule builds the target from a RuleDefinition and reuses TemporalRuleSupport, the same shape ForbiddenMethodRule uses for its own type. Also fixes SourceRootParser silently failing to parse record declarations and pattern-matching instanceof (both valid since Java 16, routine in code targeting this project's Java 17 baseline) because neither ParserConfiguration ever set a LanguageLevel. This was making every rule's analysis silently incomplete against such files, not just WG012's own tests. Adds docs/rules/WG012.md, and a PaymentService.refreshCachedBalance() violation in sample-temporal-project demonstrating that mutableSideEffect()'s updateFunction is only as good as the value type's equals(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds WG012 to detect ChangesWG012 mutable-side-effect equality
Java 17 parsing support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowSource
participant EqualityTarget
participant TypeResolver
participant WG012Rule
WorkflowSource->>EqualityTarget: inspect mutableSideEffect call
EqualityTarget->>TypeResolver: resolve Class<T> argument
TypeResolver-->>EqualityTarget: equality information
EqualityTarget-->>WG012Rule: matching call target
WG012Rule-->>WorkflowSource: emit violation and suggested fix
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java (1)
119-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWrap parse failures with
sourceNamefor consistency.Unlike
requiredString/stringList,optionalIntdoesn't attribute failures to the offending rule file. A malformedvalueTypeArgumentIndex(e.g.1.5,true) throws a bareNumberFormatExceptionwith no indication of which YAML resource caused it.🔧 Proposed fix
- private static Integer optionalInt(Map<String, Object> data, String key) { - Object value = data.get(key); - return value == null ? null : Integer.valueOf(value.toString()); - } + private static Integer optionalInt(Map<String, Object> data, String key, String sourceName) { + Object value = data.get(key); + if (value == null) { + return null; + } + try { + return Integer.valueOf(value.toString()); + } catch (NumberFormatException e) { + throw new IllegalStateException( + "Rule definition '" + sourceName + "' has an invalid '" + key + "': " + value, e); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java` around lines 119 - 123, Update optionalInt to accept the relevant sourceName and wrap Integer parsing failures with that source context, matching the error-handling behavior of requiredString and stringList. Update every optionalInt call site to pass the rule file’s sourceName while preserving null handling for absent values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rules/WG012.md`:
- Around line 81-85: Update the WG012 guidance and corresponding matcher
behavior so an updateFunction that explicitly compares relevant fields is
recognized as a safe remediation for mutableSideEffect values such as
CachedBalance.class. If the matcher cannot inspect or validate updateFunction
comparisons, revise the documentation to clearly identify explicit comparison as
a runtime workaround that does not suppress the diagnostic, rather than
presenting it as a rule-compliant fix.
In
`@wogu-temporal/src/main/java/dev/wogu/temporal/callgraph/ValueBasedEqualityArgumentTarget.java`:
- Around line 90-112: Update hasValueBasedEquality so its equals-method match
requires the sole parameter type to be java.lang.Object in addition to the
existing name, arity, and non-Object declaring-type checks; continue accepting
records and preserving the current fallback behavior.
In
`@wogu-temporal/src/main/java/dev/wogu/temporal/MutableSideEffectEqualityRule.java`:
- Around line 38-54: The MutableSideEffectEqualityRule constructor must retain
RuleDefinition’s suppressedContexts and requiredContexts when creating its
target or rule metadata. Pass both context sets through the relevant
ValueBasedEqualityArgumentTarget/TemporalRuleSupport path instead of using empty
sets, so findViolations honors declarative context constraints.
---
Nitpick comments:
In `@wogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.java`:
- Around line 119-123: Update optionalInt to accept the relevant sourceName and
wrap Integer parsing failures with that source context, matching the
error-handling behavior of requiredString and stringList. Update every
optionalInt call site to pass the rule file’s sourceName while preserving null
handling for absent values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9594b07-b97d-45a5-ade3-4b387c5f4714
📒 Files selected for processing (19)
CHANGELOG.mddocs/rules/WG012.mdsample-temporal-project/pom.xmlsample-temporal-project/src/main/java/com/example/wogu/sample/PaymentWorkflowImpl.javasample-temporal-project/src/main/java/com/example/wogu/sample/services/CachedBalance.javasample-temporal-project/src/main/java/com/example/wogu/sample/services/PaymentService.javawogu-temporal/src/main/java/dev/wogu/temporal/MutableSideEffectEqualityRule.javawogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinition.javawogu-temporal/src/main/java/dev/wogu/temporal/RuleDefinitionLoader.javawogu-temporal/src/main/java/dev/wogu/temporal/RuleRegistry.javawogu-temporal/src/main/java/dev/wogu/temporal/SourceRootParser.javawogu-temporal/src/main/java/dev/wogu/temporal/TemporalWorkflowValidator.javawogu-temporal/src/main/java/dev/wogu/temporal/callgraph/ValueBasedEqualityArgumentTarget.javawogu-temporal/src/main/resources/rules/wg012.yamlwogu-temporal/src/test/java/dev/wogu/temporal/MutableSideEffectEqualityRuleTest.javawogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionLoaderTest.javawogu-temporal/src/test/java/dev/wogu/temporal/RuleDefinitionTest.javawogu-temporal/src/test/java/dev/wogu/temporal/RuleRegistryTest.javawogu-temporal/src/test/java/dev/wogu/temporal/TemporalWorkflowValidatorTest.java
Three fixes from code review, each verified against current behavior before changing anything: - docs/rules/WG012.md and wg012.yaml's replacement text claimed an updateFunction that explicitly compares fields is an alternative fix. It isn't: ValueBasedEqualityArgumentTarget only ever inspects the valueClass argument, never updateFunction, so that advice would not have suppressed the diagnostic. Corrected both to say so plainly, and explained why the matcher doesn't attempt to validate a lambda's comparison logic (a trivial or inverted comparator would look the same syntactically). - hasValueBasedEquality's equals-detection matched any 1-param method named "equals" not declared on Object, which incorrectly treated a same-named overload like equals(Price) as a real Object.equals(Object) override. Added a parameter-type check (must be java.lang.Object) so only a genuine override (or a record's synthesized one) counts. Verified by reverting the fix and confirming the new regression test fails without it. - MutableSideEffectEqualityRule's constructor hardcoded Set.of() for both suppressedContexts and requiredContexts instead of reading them off the RuleDefinition, silently ignoring both fields (wg012.yaml itself doesn't use them yet, so no observable effect today, but any future mutable-side-effect-equality rule declaring either would have been broken). Reused ForbiddenMethodRule's toExecutionContext (widened to package-private) to parse and wire both through, mirroring ForbiddenMethodRule exactly. Verified the same way: reverted, watched the new regression test fail, restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix issue #3
New declarative rule type, mutable-side-effect-equality, alongside the existing forbidden-method: flags Workflow.mutableSideEffect(id, valueClass, updateFunction, func) calls whose value type relies on inherited, identity-based Object.equals(). Since func constructs a new instance every call, equals() never reports "unchanged", so every call appends a new history event regardless of whether the logical value actually changed.
RuleDefinition gains a valueTypeArgumentIndex field. A new ValueBasedEqualityArgumentTarget (wogu-temporal.callgraph, reusable) resolves the named argument's Class literal via the symbol solver and checks for value-based equals(): a Java record, a declared or inherited non-Object equals(), or an unresolvable type (outside WoGu's own classpath) are all treated as safe, matching the false-negative- preferring default every other rule uses for unresolvable code. MutableSideEffectEqualityRule builds the target from a RuleDefinition and reuses TemporalRuleSupport, the same shape ForbiddenMethodRule uses for its own type.
Also fixes SourceRootParser silently failing to parse record declarations and pattern-matching instanceof (both valid since Java 16, routine in code targeting this project's Java 17 baseline) because neither ParserConfiguration ever set a LanguageLevel. This was making every rule's analysis silently incomplete against such files, not just WG012's own tests.
Adds docs/rules/WG012.md, and a PaymentService.refreshCachedBalance() violation in sample-temporal-project demonstrating that mutableSideEffect()'s updateFunction is only as good as the value type's equals().
Summary by CodeRabbit
New Features
mutableSideEffect()when the value type uses reference/identity-based equality.equals()/hashCode()(e.g., records) and that changing the comparison logic alone may not help.Bug Fixes
instanceofpattern matching).Documentation
Tests