Skip to content

SONARJAVA-6685 Implement new rule S1944 - #5842

Closed
romainbrenguier wants to merge 3 commits into
masterfrom
new-rule/SONARJAVA-6685-S1944
Closed

SONARJAVA-6685 Implement new rule S1944#5842
romainbrenguier wants to merge 3 commits into
masterfrom
new-rule/SONARJAVA-6685-S1944

Conversation

@romainbrenguier

Copy link
Copy Markdown
Contributor

Detect inappropriate type casts that will always fail at runtime with a ClassCastException. The rule flags casts between unrelated concrete classes and casts between a final class and an unrelated interface.

Part of

Detect inappropriate type casts that will always fail at runtime with
a ClassCastException. The rule flags casts between unrelated concrete
classes and casts between a final class and an unrelated interface.
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6685

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was done using the wrong version of rule-api

Comment on lines +49 to +53
if (areNeitherInterfaces(sourceType, targetType) || areTypesFinalClassAndInterface(sourceType, targetType)) {
reportIssue(castTree.type(),
String.format("\"%s\" cannot be cast to \"%s\" without a risk of \"ClassCastException\".",
sourceType.name(), targetType.name()));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Bug: Rule only matches casts that are compile-time errors

Every Noncompliant case detected by the two conditions in visitNode (areNeitherInterfaces = both are concrete classes; areTypesFinalClassAndInterface = a final class vs. an unrelated interface) is rejected by the Java compiler itself as "inconvertible types". I compiled the six Noncompliant samples with javac and all six fail to compile (Dog→Car, String→Dog, FinalDog→Vehicle, Vehicle→FinalDog, Color→Size, Color→Drawable). Since SonarJava only analyzes code that compiles, this rule can effectively never raise an issue on a real project, and the test sample file (InappropriateCastCheckSample.java) does not compile. A working S1944 must target casts that DO compile but fail at runtime (e.g. generic-argument casts via erasure, or interface→non-final-class casts where the concrete object is unrelated), not casts already forbidden by the compiler.

Was this helpful? React with 👍 / 👎

Comment on lines +4 to +18
<h3>Noncompliant code example</h3>
<pre>
public class S1944 {

public static void main(String[] args) {
List&lt;String&gt; list = (List&lt;String&gt;) getAttributes(); // Noncompliant; List&lt;Integer&gt; return by getAttributes() is not be casted to List&lt;String&gt;
String s = list.get(0); // java.lang.ClassCastException will be raised here
}

private static List&lt;?&gt; getAttributes() {
List&lt;Integer&gt; result = new ArrayList&lt;&gt;();
result.add(0);
return result;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Quality: Implementation contradicts documented S1944 behavior

S1944.html documents the rule as flagging generic casts that fail at runtime, e.g. (List<String>) getAttributes() where the actual value is a List<Integer>. The implementation uses erasure() and explicitly treats exactly this scenario as Compliant (sample line 81: (List<String>) wildcardList). So the shipped documentation describes a case the code never detects, while the code detects only compile-error casts the docs never mention. Align the implementation with the documented intent (or update the HTML/JSON to reflect the actual, narrower behavior) before this leaves draft.

Was this helpful? React with 👍 / 👎

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This PR is blocked due to unresolved code review findings.

Comment gitar unblock to override this block and allow merging.

Configure merge blocking · Maintainers can dismiss this review.

@github-actions

Copy link
Copy Markdown
Contributor

Ruling needs updating. A fix PR has been created: #5843

Please review and merge it into your branch.

romainbrenguier and others added 2 commits July 28, 2026 14:59
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Ruling needs updating. A fix PR has been created: #5843

Please review and merge it into your branch.

@romainbrenguier

Copy link
Copy Markdown
Contributor Author

It looks like to raise anything useful, we would need some dataflow analysis, so I'll not implement this rule right now.

@gitar-bot

gitar-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown
CI failed: Integration test failures occurred due to unexpected issue differences in the Java ruling tests (eclipse_jetty_incremental) introduced by the new rule S1944 implementation, along with missing directories in autoscan tests.

Overview

Analysis of 12 CI logs across parallel jobs identified test failures related to the implementation of new rule S1944 in sonar-java. The failures span integration test baseline mismatches in JavaRulingTest and missing output directories in the autoscan integration tests.

Failures

JavaRulingTest Issue Differences (confidence: high)

  • Type: test
  • Affected jobs: github-windows-latest / ruling tests
  • Related to change: yes
  • Root cause: The integration test org.sonar.java.it.JavaRulingTest.eclipse_jetty_incremental failed with an AssertionError (Expecting empty but was: "Issues differences: 2"). The new rule implementation (S1944) produces a different set of issues than expected by the existing ruling baseline.
  • Suggested fix: Review the test diff report located in its/ruling/target/ to inspect the 2 unexpected issues and update the test baseline in its/ruling/src/test/resources or refine the rule logic.

Autoscan Integration Test Missing Directory (confidence: high)

  • Type: test
  • Affected jobs: Autoscan Tests
  • Related to change: yes
  • Root cause: The autoscan integration test expected target/actual/autoscan-diffs to be generated by a prior step, but the directory was missing (diff: target/actual/autoscan-diffs: No such file or directory), causing diff2html-cli to receive empty input.
  • Suggested fix: Ensure the autoscan test preparation step successfully generates the expected output directories prior to running the diff comparison.

Summary

  • Change-related failures: 2 failure groups (JavaRulingTest baseline mismatch from rule S1944 changes, and missing autoscan diff directory output).
  • Infrastructure/flaky failures: None.
  • Recommended action: Inspect the rule S1944 behavior against the ruling test baseline and update expected test resources or baseline snapshots accordingly.
Code Review 🚫 Blocked 0 resolved / 2 findings

Implements rule S1944 to detect inappropriate type casts, but the current logic only matches casts that are already compile-time errors and contradicts documented behavior.

🚨 Bug: Rule only matches casts that are compile-time errors

📄 java-checks/src/main/java/org/sonar/java/checks/InappropriateCastCheck.java:49-53 📄 java-checks/src/main/java/org/sonar/java/checks/InappropriateCastCheck.java:66-73 📄 java-checks-test-sources/default/src/main/java/checks/InappropriateCastCheckSample.java:24-38

Every Noncompliant case detected by the two conditions in visitNode (areNeitherInterfaces = both are concrete classes; areTypesFinalClassAndInterface = a final class vs. an unrelated interface) is rejected by the Java compiler itself as "inconvertible types". I compiled the six Noncompliant samples with javac and all six fail to compile (Dog→Car, String→Dog, FinalDog→Vehicle, Vehicle→FinalDog, Color→Size, Color→Drawable). Since SonarJava only analyzes code that compiles, this rule can effectively never raise an issue on a real project, and the test sample file (InappropriateCastCheckSample.java) does not compile. A working S1944 must target casts that DO compile but fail at runtime (e.g. generic-argument casts via erasure, or interface→non-final-class casts where the concrete object is unrelated), not casts already forbidden by the compiler.

⚠️ Quality: Implementation contradicts documented S1944 behavior

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1944.html:4-18 📄 java-checks-test-sources/default/src/main/java/checks/InappropriateCastCheckSample.java:80-83

S1944.html documents the rule as flagging generic casts that fail at runtime, e.g. (List<String>) getAttributes() where the actual value is a List<Integer>. The implementation uses erasure() and explicitly treats exactly this scenario as Compliant (sample line 81: (List<String>) wildcardList). So the shipped documentation describes a case the code never detects, while the code detects only compile-error casts the docs never mention. Align the implementation with the documented intent (or update the HTML/JSON to reflect the actual, narrower behavior) before this leaves draft.

🤖 Prompt for agents
Code Review: Implements rule S1944 to detect inappropriate type casts, but the current logic only matches casts that are already compile-time errors and contradicts documented behavior.

1. 🚨 Bug: Rule only matches casts that are compile-time errors
   Files: java-checks/src/main/java/org/sonar/java/checks/InappropriateCastCheck.java:49-53, java-checks/src/main/java/org/sonar/java/checks/InappropriateCastCheck.java:66-73, java-checks-test-sources/default/src/main/java/checks/InappropriateCastCheckSample.java:24-38

   Every Noncompliant case detected by the two conditions in visitNode (`areNeitherInterfaces` = both are concrete classes; `areTypesFinalClassAndInterface` = a final class vs. an unrelated interface) is rejected by the Java compiler itself as "inconvertible types". I compiled the six Noncompliant samples with javac and all six fail to compile (Dog→Car, String→Dog, FinalDog→Vehicle, Vehicle→FinalDog, Color→Size, Color→Drawable). Since SonarJava only analyzes code that compiles, this rule can effectively never raise an issue on a real project, and the test sample file (InappropriateCastCheckSample.java) does not compile. A working S1944 must target casts that DO compile but fail at runtime (e.g. generic-argument casts via erasure, or interface→non-final-class casts where the concrete object is unrelated), not casts already forbidden by the compiler.

2. ⚠️ Quality: Implementation contradicts documented S1944 behavior
   Files: sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1944.html:4-18, java-checks-test-sources/default/src/main/java/checks/InappropriateCastCheckSample.java:80-83

   S1944.html documents the rule as flagging generic casts that fail at runtime, e.g. `(List<String>) getAttributes()` where the actual value is a `List<Integer>`. The implementation uses `erasure()` and explicitly treats exactly this scenario as Compliant (sample line 81: `(List<String>) wildcardList`). So the shipped documentation describes a case the code never detects, while the code detects only compile-error casts the docs never mention. Align the implementation with the documented intent (or update the HTML/JSON to reflect the actual, narrower behavior) before this leaves draft.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change the behavior for this request:

Auto-apply Compact Unblock
gitar auto-apply:on         
gitar display:verbose         
gitar unblock         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant