JS-1308 Fix S3516 false positives when same return value has different semantic meanings#6588
Open
sonar-nigel[bot] wants to merge 4 commits intomasterfrom
Conversation
Tests cover the scenario where a function returns the same literal value across multiple branches but each branch performs a distinct side effect, giving each return a different semantic meaning (e.g. "skipped" vs "completed", "handled escape" vs "handled enter" vs "not handled"). The valid cases verify suppression when all return-containing branches have side effects and no pure-return branch exists. The new invalid case verifies that suppression does not apply when a pure-return guard branch is present alongside a side-effect+return branch. Relates to JS-1308
Rule S3516 raised false positives on functions that intentionally return the same literal value across branches when each return represents a different semantic outcome (e.g., event handlers using false for distinct propagation decisions, or status functions using true for "skipped" vs "completed"). The root cause was that the rule flagged all literal invariant returns without distinguishing whether each return branch was doing meaningful work. Fix adds two flags to FunctionContext: hasSideEffectBranchWithReturn (set when a branch has both side effects and a return) and hasReturnBranchWithoutSideEffect (set when a branch has a return but no side effects). Suppression now applies only when ALL branching returns are accompanied by side effects, ensuring that pure early-exit branches (no side effect before returning the same literal) still trigger the rule. This preserves the original noncompliant spec example and the Redux switch-case pattern while fixing the identified false positives. Relates to JS-1308
Reviewed 13 ruling entries across ace, TypeScript, redux, desktop, and mootools-core projects. Confirmed that the implementation correctly distinguishes between: - Pure returns (should raise): Functions with at least one branch that returns the same value without side effects (suspicious code) - Semantic returns (should not raise): Functions where ALL return paths perform meaningful side effects before returning the same value (intentional patterns like event handlers, status functions, and guards) All 13 entries match expected behavior. No implementation changes needed. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Ticket: JS-1308 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Ruling ReportCode no longer flagged (4 issues)S3516ace/lib/ace/mode/html/saxparser.js:1766 1764 | }
1765 |
> 1766 | function markup_declaration_open_state(buffer) {
1767 | var chars = buffer.shift(2);
1768 | if (chars === '--') {ace/lib/ace/mode/html/saxparser.js:2007 2005 | }
2006 |
> 2007 | function after_doctype_name_state(buffer) {
2008 | var data = buffer.char();
2009 | if (data === InputStream.EOF) {custom-yaml/lambda-block-folded.yaml:111 109 | responseData.Key = key;
110 | console.log("LAMBDA FUNCTION BUCKET:",bucket,"KEY:",key);
> 111 | (async function(){
112 | if (event.RequestType && event.RequestType == 'Create'){
113 | params.Bucket = bucket;mootools-core/Source/Slick/Slick.Parser.js:106 104 | );
105 |
> 106 | function parser(
107 | rawMatch,
108 | |
|
Contributor
Contributor
Author
|
Thank you for the review and approval! |
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.
Rule S3516 raised false positives on functions that intentionally return the same literal value across branches where each return represents a distinct semantic outcome — for example, event handlers returning
falsefor different propagation decisions, or status functions returningtruefor "skipped" vs "completed".Root Cause
The rule flagged all literal invariant returns without checking whether the return branches performed meaningful work beforehand. Pure early-exit branches and side-effect branches were treated identically.
Fix
Two flags were added to
FunctionContext:hasSideEffectBranchWithReturn— set when a branch has both side effects and a returnhasReturnBranchWithoutSideEffect— set when a branch returns without side effectsSuppression now applies only when all branching returns are accompanied by side effects. A pure early-exit branch (returning the same literal with no side effect) still triggers the rule.
Behavior Preserved
switch-case pattern (all branches have side effects) is correctly suppressedValidation
Reviewed 13 ruling entries across
ace,TypeScript,redux,desktop, andmootools-coreprojects — all match expected behavior.Relates to JS-1308