SONARPY-1640 S6883: The 12-hour format should be used with the AM/PM marker, otherwise 24-hour format should be used (#1736)

Author: ghislainpiot

python-checks/src/main/java/org/sonar/python/checks/CheckList.java

@@ -338,6 +338,7 @@ public static Iterable<Class> getChecks() {
       SpecialMethodReturnTypeCheck.class,
       SQLQueriesCheck.class,
       StandardInputCheck.class,
+      StrftimeConfusingHourSystemCheck.class,
       StringFormatCorrectnessCheck.class,
       StringFormatMisuseCheck.class,
       StringLiteralDuplicationCheck.class,

python-checks/src/main/java/org/sonar/python/checks/StrftimeConfusingHourSystemCheck.java

@@ -0,0 +1,91 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2024 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks;
+
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.StringLiteral;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.python.checks.utils.Expressions;
+import org.sonar.python.tree.TreeUtils;
+
+@Rule(key = "S6883")
+public class StrftimeConfusingHourSystemCheck extends PythonSubscriptionCheck {
+
+  public static final String MESSAGE = "Use %I (12-hour clock) or %H (24-hour clock) without %p (AM/PM).";
+  public static final String MESSAGE_12_HOURS = "Use %I (12-hour clock) with %p (AM/PM).";
+  private static final String MESSAGE_SECONDARY_LOCATION = "Wrong format created here.";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, StrftimeConfusingHourSystemCheck::checkCallExpr);
+  }
+
+  private static void checkExpression(SubscriptionContext context, Expression expression) {
+    checkExpression(context, expression, expression);
+  }
+
+  private static void checkExpression(SubscriptionContext context, Expression expression, Tree primaryLocation) {
+    Expressions.ifNameGetSingleAssignedNonNameValue(expression)
+      .filter(StringLiteral.class::isInstance)
+      .map(StringLiteral.class::cast)
+      .filter(stringLiteral -> stringLiteral.stringElements().stream().noneMatch(stringElement -> "f".equalsIgnoreCase(stringElement.prefix())))
+      .ifPresent(stringLiteral -> checkDateFormatStringLiteral(context, primaryLocation, stringLiteral));
+  }
+
+  private static void checkDateFormatStringLiteral(SubscriptionContext context, Tree primaryLocation, StringLiteral stringLiteral) {
+    String value = stringLiteral.trimmedQuotesValue();
+    String effectiveMessage = null;
+    if (value.contains("%H") && value.contains("%p")) {
+      effectiveMessage = MESSAGE;
+    } else if (value.contains("%I") && !value.contains("%p")) {
+      effectiveMessage = MESSAGE_12_HOURS;
+    }
+    if (effectiveMessage != null) {
+      var issue = context.addIssue(primaryLocation, effectiveMessage);
+      if (primaryLocation != stringLiteral) {
+        issue.secondary(stringLiteral, MESSAGE_SECONDARY_LOCATION);
+      }
+    }
+  }
+
+  private static void checkCallExpr(SubscriptionContext context) {
+    CallExpression callExpression = (CallExpression) context.syntaxNode();
+    Symbol calleeSymbol = callExpression.calleeSymbol();
+    if (calleeSymbol == null) {
+      return;
+    }
+    String fullyQualifiedName = calleeSymbol.fullyQualifiedName();
+    if (!"datetime.time.strftime".equals(fullyQualifiedName)) {
+      return;
+    }
+
+    RegularArgument formatArgument = TreeUtils.nthArgumentOrKeyword(0, "format", callExpression.arguments());
+    if (formatArgument == null) {
+      return;
+    }
+    checkExpression(context, formatArgument.expression());
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6883.html

@@ -0,0 +1,44 @@
+<p>This rule raises an issue when a date format string has the 24-hour code with the AM/PM marker or the 12-hour code without the AM/PM marker.</p>
+<h2>Why is this an issue?</h2>
+<p>The <code>%p</code> directive in the <code>strftime</code> method is used to represent the AM/PM marker in a time string. It is commonly used in
+conjunction with the <code>%I</code> directive, which represents the hour in a 12-hour clock.</p>
+<p>Using the 24-hour format directive with an AM/PM marker can lead to unwanted results e.g.:</p>
+<pre>
+time_string = time(16,0).strftime("%H:%M %p")
+print(time_string)
+</pre>
+<p>will print <code>16:00 PM</code> which makes no sense.</p>
+<p>On the other hand the AM/PM marker is needed when the 12-hour format directive is used to show complete information about an hour e.g.:</p>
+<pre>
+time_string = time(16,0).strftime("%I:%M")
+print(time_string)
+</pre>
+<p>will print 04:00 without indicating if the time is in the morning or the afternoon.</p>
+<h2>How to fix it</h2>
+<p>Use either the 12-hour time format with an AM/PM marker or the 24-hour format without.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+from datetime import time
+
+def foo():
+    t = time(16, 0)
+    formatted_time1 = t.strftime("%H:%M %p") # Noncompliant: 16:00 PM
+    formatted_time2 = t.strftime("%I:%M") # Noncompliant: 04:00
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+from datetime import time
+
+def foo():
+    t = time(16, 0)
+    formatted_time1 = t.strftime("%I:%M %p") # OK: 04:00 PM
+    formatted_time2 = t.strftime("%H:%M") # OK: 16:00
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Python documentation - <a href="https://docs.python.org/3/library/datetime.html#strftime-and-strptime-format-codes">strftime() and strptime()
+  Format Codes</a> </li>
+</ul>
+

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6883.json

@@ -0,0 +1,22 @@
+{
+  "title": "The 12-hour format should be used with the AM\/PM marker, otherwise the 24-hour format should be used",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6883",
+  "sqKey": "S6883",
+  "scope": "All",
+  "quickfix": "unknown",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "HIGH",
+      "RELIABILITY": "MEDIUM"
+    },
+    "attribute": "CONVENTIONAL"
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

@@ -226,6 +226,7 @@
     "S6796",
     "S6799",
     "S6882",
+    "S6883",
     "S6887",
     "S6903",
     "S6894"

python-checks/src/test/java/org/sonar/python/checks/StrftimeConfusingHourSystemCheckTest.java

@@ -0,0 +1,31 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2024 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+class StrftimeConfusingHourSystemCheckTest {
+  @Test
+  void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/strftimeConfusingHourSystem.py", new StrftimeConfusingHourSystemCheck());
+  }
+
+}

python-checks/src/test/resources/checks/strftimeConfusingHourSystem.py

@@ -0,0 +1,47 @@
+
+unrelated_unknown_function()
+def simple_cases():
+    from datetime import time
+    t = time(16, 0)
+    formatted_time1 = t.strftime("%H:%M %p") # Noncompliant {{Use %I (12-hour clock) or %H (24-hour clock) without %p (AM/PM).}}
+                                #^^^^^^^^^^
+    formatted_time2 = t.strftime("%I:%M") # Noncompliant {{Use %I (12-hour clock) with %p (AM/PM).}}
+                                #^^^^^^^
+    formatted_time3 = t.strftime("%I:%M %p")
+    formatted_time4 = t.strftime("%H:%M")
+    formatted_time5 = t.strftime()
+    formatted_time6 = t.strftime(True)
+    formatted_time7 = t.strftime("No formatting")
+    formatted_time8 = t.strftime("%I:%M:%S %p")
+    formatted_time9 = t.strftime("%I:%M:%S %p %z")
+    formatted_time10 = t.strftime("%I:%M:%S %p %Z")
+    formatted_time11 = t.strftime("%I:%M:%S %p %f")
+    formatted_time12 = t.strftime("%I:%M:%S %p %j")
+    formatted_time13 = t.strftime("%I:%M:%S %p %U")
+    formatted_time14 = t.strftime("%I:%M:%S %p %W")
+    formatted_time15 = t.strftime("%I:%M:%S %p %c")
+    formatted_time16 = t.strftime("%I:%M:%S %p %x")
+    formatted_time17 = t.strftime("%I:%M:%S %p %X")
+    formatted_time18 = t.strftime("%I:%M:%S %p %G")
+    formatted_time19 = t.strftime(" %p %H %M") # Noncompliant {{Use %I (12-hour clock) or %H (24-hour clock) without %p (AM/PM).}}
+                                 #^^^^^^^^^^^
+    formatted_time20 = t.strftime("Hour :%I , minutes :  %M") # Noncompliant {{Use %I (12-hour clock) with %p (AM/PM).}}
+    formatted_time21 = t.strftime("Hour :%H , minutes :  %M , useless : %p") # Noncompliant {{Use %I (12-hour clock) or %H (24-hour clock) without %p (AM/PM).}}
+    format1 = "%H:%M %p"
+             #^^^^^^^^^^> 1 {{Wrong format created here.}}
+    t.strftime(format1) # Noncompliant {{Use %I (12-hour clock) or %H (24-hour clock) without %p (AM/PM).}}
+              #^^^^^^^
+
+    if cond():
+        format2 = "%I:%M"
+    else:
+        format2 = "%H:%M"
+    t.strftime(format2) # FN because of the limitations of SingleAssignedValue
+
+    # This one is a FN because we don't support f-strings in this rule
+    format19 = f"%I:%M"
+    t.strftime(format19)
+
+    # This one is a FN because we don't support f-strings in this rule
+    format_marker = "%p"
+    t.strftime(f"%H:%M {format_marker}")