Skip to content

SONARJAVA-6298 Modify S2143 to also suggest using java.time instead of Joda-Time#5614

Merged
NoemieBenard merged 23 commits into
masterfrom
nb/sonarjava-6298-modify-S2143
May 27, 2026
Merged

SONARJAVA-6298 Modify S2143 to also suggest using java.time instead of Joda-Time#5614
NoemieBenard merged 23 commits into
masterfrom
nb/sonarjava-6298-modify-S2143

Conversation

@NoemieBenard
Copy link
Copy Markdown
Contributor

@NoemieBenard NoemieBenard commented May 8, 2026


Summary by Gitar

  • Refactored S2143 detection:
    • Modified the check to use string parsing of imports instead of the semantic model, allowing detection without full semantic analysis.
    • Updated the rule to report issues at the file level rather than on specific method invocations or constructors.
  • Expanded detection scope:
    • Added support for flagging Joda-Time imports as a recommendation to migrate to java.time.
    • Extended the check to cover additional legacy date-time types such as java.sql.Date, java.sql.Time, java.sql.Timestamp, and java.util.GregorianCalendar.
  • Updated rule documentation:
    • Improved S2143.html to clarify the transition from legacy date types and Joda-Time to java.time.
    • Added a comprehensive table of java.time mappings and updated code examples and resources.
  • Adjusted rule metadata:
    • Demoted defaultSeverity from Major to Info and updated MAINTAINABILITY impact in S2143.json.
    • Added S2143 to Sonar_way_profile.json.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod
Copy link
Copy Markdown
Contributor

hashicorp-vault-sonar-prod Bot commented May 8, 2026

SONARJAVA-6298

@sonar-review-alpha
Copy link
Copy Markdown
Contributor

sonar-review-alpha Bot commented May 8, 2026

Summary

S2143 Rule Enhancement: String-Based Import Detection

This PR modifies the "Use Java 8 Date and Time API instead" rule to detect problematic imports directly via string parsing, rather than relying solely on semantic analysis of method invocations and constructors.

Key Changes:

  • The check now analyzes import statements and flags imports of Joda-Time (org.joda.time.*) and legacy Java date/time classes (Date, Calendar, java.sql.Date, etc.)
  • Switched from semantic-based detection to syntactic import analysis using ExpressionsHelper
  • Added new test case for wildcard imports and a test mode that verifies the check works without semantic information
  • Integration test baselines significantly expanded, reflecting the increased coverage (more files now flagged due to import-level detection)

What reviewers should know

Where to Start

Read the core logic change in java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:

  • The new visitNode() override handles import analysis (lines ~44–52)
  • KNOWN_JAVA_UTIL_DATE_TIME_CLASSES set defines what legacy classes trigger the rule
  • The check still inherits semantic detection from AbstractMethodDetection for method calls/constructors

Key Implementation Details

  1. Dual Detection: The rule now detects issues two ways:

    • Import statements (new) — reported immediately when Joda-Time or legacy java.util/java.sql date classes are imported
    • Method invocations/constructors (existing) — still detected via parent class semantic matching
  2. String Parsing Approach: Uses ExpressionsHelper.concatenate() to extract the fully qualified import name from the syntax tree, then checks it against known problematic patterns. No semantic symbol resolution needed.

  3. Known Limitation: Wildcard imports like import java.util.* are not detected by the rule. See DateAndTimesCheckWildcardImport.java for expected behavior.

What the Baseline Changes Mean

The integration test files (.json baselines) show significantly more violations because:

  • Previously only flagged actual usage (e.g., new Date(), Calendar.getInstance())
  • Now also flags the imports themselves, increasing detection count per file
  • This is expected and correct behavior for the new approach

Testing

The test suite now includes:

  • Standard file test (with semantic analysis)
  • New test_wildcard_import() — verifies wildcard imports don't trigger false positives
  • New test_without_semantic() — confirms the rule works in non-semantic mode (string parsing only)

  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as resolved.

sonar-review-alpha[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@aurelien-coet-sonarsource aurelien-coet-sonarsource left a comment

Choose a reason for hiding this comment

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

Should we consider reporting issues for the old date and time API on imports of Date and Calendar rather than on their uses, to be consistent with what we do on JodaTime ?

sonar-review-alpha[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor

@aurelien-coet-sonarsource aurelien-coet-sonarsource left a comment

Choose a reason for hiding this comment

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

The current implementation for imports LGTM, but we should probably still report an issue on uses or insantiations of Date and Calendar when they are imported via a wildcard import. For example, we would report an issue for the rule on a call to new Date() when the class is imported with import java.util.*:

import java.util.*;

class ShouldRaiseOnUse {
    void foo() {
        Date now = new Date(); // Noncompliant ...
    }
}

sonar-review-alpha[bot]

This comment was marked as resolved.

sonar-review-alpha[bot]

This comment was marked as outdated.

sonar-review-alpha[bot]

This comment was marked as resolved.

sonar-review-alpha[bot]

This comment was marked as resolved.

sonar-review-alpha[bot]

This comment was marked as resolved.

@sonarqube-next
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 26, 2026

Agentic Analysis: Early Results

Agentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action.

17 issue(s) found across 4 file(s):

Rule File Line Message
java:S125 java-checks/src/test/files/checks/DateAndTimesCheck/JodaTime.java 6 This block of commented-out lines of code should be removed.
java:S2208 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 10 Explicitly import the specific classes needed.
java:S1854 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 21 Remove this useless assignment to local variable "now".
java:S1481 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 21 Remove this unused "now" local variable.
java:S1854 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 22 Remove this useless assignment to local variable "date".
java:S2148 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 22 Add underscores to this numeric value for readability
java:S1481 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 22 Remove this unused "date" local variable.
java:S1854 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 23 Remove this useless assignment to local variable "christmas".
java:S1481 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 23 Remove this unused "christmas" local variable.
java:S1854 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 24 Remove this useless assignment to local variable "epochDate".
java:S1481 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 24 Remove this unused "epochDate" local variable.
java:S125 java-checks/src/test/files/checks/DateAndTimesCheck/MultipleImports.java 28 This block of commented-out lines of code should be removed.
java:S125 java-checks/src/test/files/checks/DateAndTimesCheck/StaticImport.java 6 This block of commented-out lines of code should be removed.
java:S2208 java-checks/src/test/files/checks/DateAndTimesCheck/WildcardImport.java 1 Explicitly import the specific classes needed.
java:S1854 java-checks/src/test/files/checks/DateAndTimesCheck/WildcardImport.java 5 Remove this useless assignment to local variable "now".
java:S1481 java-checks/src/test/files/checks/DateAndTimesCheck/WildcardImport.java 5 Remove this unused "now" local variable.
java:S125 java-checks/src/test/files/checks/DateAndTimesCheck/WildcardImport.java 9 This block of commented-out lines of code should be removed.

Analyzed by SonarQube Agentic Analysis in 5.8 s

Comment thread java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java Outdated
Comment thread java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java Outdated
gitar-bot[bot]

This comment was marked as resolved.

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 26, 2026

Code Review 🚫 Blocked 1 resolved / 3 findings

Refactors S2143 to use string parsing for file-level detection of legacy date-time types and Joda-Time, but the implementation contains a critical message mismatch and a potential false positive due to loose string matching.

🚨 Bug: Test message does not match implementation constant

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:54 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:30 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:39 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:48 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:57 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:67

The ISSUE_MESSAGE constant is "Use the "java.time" API for date and time." but all test methods use verifyIssueOnFile("Use the Java 8 Date and Time API instead."). This mismatch means either the tests will fail, or if they pass due to the test framework ignoring message content in verifyIssueOnFile, they don't actually validate the correct message. Note that the test fixture files (e.g., JodaTime.java) use the correct message in their // Noncompliant comment.

Update all test assertions to use the actual message from ISSUE_MESSAGE.
.verifyIssueOnFile("Use the "java.time" API for date and time.");
⚠️ Bug: startsWith match causes false positives for similar class names

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:82

The check at line 82 uses KNOWN_JAVA_UTIL_DATE_TIME_CLASSES.stream().anyMatch(qualifiedName::startsWith) which checks if the import's qualified name starts with any known class. Since "java.util.Date" is in the set, imports like java.util.DateFormat or java.util.DateFormatSymbols will incorrectly trigger the rule because "java.util.DateFormat".startsWith("java.util.Date") is true in Java. The same issue applies to java.sql.DataSource matching java.sql.Date — wait, actually "java.sql.DataSource" does not start with "java.sql.Date" but classes like inner types could still match unexpectedly.

Use equals or startsWith with a dot separator to avoid matching unrelated classes that happen to share a common prefix (e.g., java.util.DateFormat).
if (qualifiedName.startsWith("org.joda.time.")
  || KNOWN_JAVA_UTIL_DATE_TIME_CLASSES.stream().anyMatch(known -> qualifiedName.equals(known) || qualifiedName.startsWith(known + "."))) {
  addIssueOnFile();
}
✅ 1 resolved
Bug: issueAlreadyRaised never reset between files

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:55 📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:57-62
The issueAlreadyRaised field is set to true when an issue is reported but is never reset to false between file analyses. After the first file triggers the rule, all subsequent files in the same scan will be silently skipped. The standard pattern in this codebase is to override setContext() or leaveFile() from IssuableSubscriptionVisitor to reset per-file state.

🤖 Prompt for agents
Code Review: Refactors S2143 to use string parsing for file-level detection of legacy date-time types and Joda-Time, but the implementation contains a critical message mismatch and a potential false positive due to loose string matching.

1. 🚨 Bug: Test message does not match implementation constant
   Files: java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:54, java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:30, java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:39, java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:48, java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:57, java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:67

   The `ISSUE_MESSAGE` constant is `"Use the "java.time" API for date and time."` but all test methods use `verifyIssueOnFile("Use the Java 8 Date and Time API instead.")`. This mismatch means either the tests will fail, or if they pass due to the test framework ignoring message content in `verifyIssueOnFile`, they don't actually validate the correct message. Note that the test *fixture* files (e.g., `JodaTime.java`) use the correct message in their `// Noncompliant` comment.

   Fix (Update all test assertions to use the actual message from ISSUE_MESSAGE.):
   .verifyIssueOnFile("Use the "java.time" API for date and time.");

2. ⚠️ Bug: startsWith match causes false positives for similar class names
   Files: java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:82

   The check at line 82 uses `KNOWN_JAVA_UTIL_DATE_TIME_CLASSES.stream().anyMatch(qualifiedName::startsWith)` which checks if the import's qualified name starts with any known class. Since `"java.util.Date"` is in the set, imports like `java.util.DateFormat` or `java.util.DateFormatSymbols` will incorrectly trigger the rule because `"java.util.DateFormat".startsWith("java.util.Date")` is `true` in Java. The same issue applies to `java.sql.DataSource` matching `java.sql.Date` — wait, actually "java.sql.DataSource" does not start with "java.sql.Date" but classes like inner types could still match unexpectedly.

   Fix (Use equals or startsWith with a dot separator to avoid matching unrelated classes that happen to share a common prefix (e.g., java.util.DateFormat).):
   if (qualifiedName.startsWith("org.joda.time.")
     || KNOWN_JAVA_UTIL_DATE_TIME_CLASSES.stream().anyMatch(known -> qualifiedName.equals(known) || qualifiedName.startsWith(known + "."))) {
     addIssueOnFile();
   }

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:

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

Was this helpful? React with 👍 / 👎 | Gitar

@gitar-bot gitar-bot Bot dismissed their stale review May 27, 2026 07:05

✅ All code review findings resolved.

Configure merge blocking

Copy link
Copy Markdown
Contributor

@aurelien-coet-sonarsource aurelien-coet-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM ! We'll just need to update the rule metadata once its severity level has been changed on RSPEC.

@NoemieBenard NoemieBenard force-pushed the nb/sonarjava-6298-modify-S2143 branch from 4f753af to 7e38733 Compare May 27, 2026 16:17
@sonarqube-next
Copy link
Copy Markdown

@NoemieBenard NoemieBenard merged commit 7cd902e into master May 27, 2026
15 checks passed
@NoemieBenard NoemieBenard deleted the nb/sonarjava-6298-modify-S2143 branch May 27, 2026 16:36
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 27, 2026

Code Review ✅ Approved 3 resolved / 3 findings

Extends S2143 detection to include Joda-Time and legacy java.sql date types using string-based import parsing. Resolved issues regarding state reset between files, incorrect test messages, and false-positive class matching.

✅ 3 resolved
Bug: issueAlreadyRaised never reset between files

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:55 📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:57-62
The issueAlreadyRaised field is set to true when an issue is reported but is never reset to false between file analyses. After the first file triggers the rule, all subsequent files in the same scan will be silently skipped. The standard pattern in this codebase is to override setContext() or leaveFile() from IssuableSubscriptionVisitor to reset per-file state.

Bug: Test message does not match implementation constant

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:54 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:30 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:39 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:48 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:57 📄 java-checks/src/test/java/org/sonar/java/checks/DateAndTimesCheckTest.java:67
The ISSUE_MESSAGE constant is "Use the "java.time" API for date and time." but all test methods use verifyIssueOnFile("Use the Java 8 Date and Time API instead."). This mismatch means either the tests will fail, or if they pass due to the test framework ignoring message content in verifyIssueOnFile, they don't actually validate the correct message. Note that the test fixture files (e.g., JodaTime.java) use the correct message in their // Noncompliant comment.

Bug: startsWith match causes false positives for similar class names

📄 java-checks/src/main/java/org/sonar/java/checks/DateAndTimesCheck.java:82
The check at line 82 uses KNOWN_JAVA_UTIL_DATE_TIME_CLASSES.stream().anyMatch(qualifiedName::startsWith) which checks if the import's qualified name starts with any known class. Since "java.util.Date" is in the set, imports like java.util.DateFormat or java.util.DateFormatSymbols will incorrectly trigger the rule because "java.util.DateFormat".startsWith("java.util.Date") is true in Java. The same issue applies to java.sql.DataSource matching java.sql.Date — wait, actually "java.sql.DataSource" does not start with "java.sql.Date" but classes like inner types could still match unexpectedly.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

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

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.

2 participants