SONARPY-894 Rule S5842 Regex repetition pattern's body should not match the empty String (#960)

* SONARPY-894 Rule S5842 Regex repetition pattern's body should not match the empty String

* Increase code coverage

* Address review comments

Author: nils-werner-sonarsource

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

@@ -49,6 +49,7 @@
 import org.sonar.python.checks.hotspots.StrongCryptographicKeysCheck;
 import org.sonar.python.checks.hotspots.UnsafeHttpMethodsCheck;
 import org.sonar.python.checks.hotspots.UnverifiedHostnameCheck;
+import org.sonar.python.checks.regex.EmptyStringRepetitionCheck;
 import org.sonar.python.checks.regex.StringReplaceCheck;
 
 public final class CheckList {
@@ -105,6 +106,7 @@ public static Iterable<Class> getChecks() {
       EmailSendingCheck.class,
       EmptyFunctionCheck.class,
       EmptyNestedBlockCheck.class,
+      EmptyStringRepetitionCheck.class,
       ExceptionCauseTypeCheck.class,
       ExceptionNotThrownCheck.class,
       ExceptionSuperClassDeclarationCheck.class,

python-checks/src/main/java/org/sonar/python/checks/regex/AbstractRegexCheck.java

@@ -21,8 +21,10 @@
 
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import javax.annotation.Nullable;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
 import org.sonar.plugins.python.api.symbols.Symbol;
@@ -33,14 +35,19 @@
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.regex.RegexContext;
 import org.sonar.python.tree.TreeUtils;
+import org.sonarsource.analyzer.commons.regex.RegexIssueLocation;
 import org.sonarsource.analyzer.commons.regex.RegexParseResult;
+import org.sonarsource.analyzer.commons.regex.ast.RegexSyntaxElement;
 
 public abstract class AbstractRegexCheck extends PythonSubscriptionCheck {
 
   private static final Set<String> REGEX_FUNCTIONS = new HashSet<>(Arrays.asList("re.sub", "re.subn", "re.compile", "re.search", "re.match",
     "re.fullmatch", "re.split", "re.findall", "re.finditer"));
   protected RegexContext regexContext;
 
+  // We want to report only one issue per element for one rule.
+  protected final Set<RegexSyntaxElement> reportedRegexTrees = new HashSet<>();
+
   protected Set<String> lookedUpFunctionNames() {
     return REGEX_FUNCTIONS;
   }
@@ -86,4 +93,11 @@ private static Optional<StringLiteral> patternArgStringLiteral(CallExpression re
     return Optional.empty();
   }
 
+  public void addIssue(RegexSyntaxElement regexTree, String message, @Nullable Integer cost, List<RegexIssueLocation> secondaries) {
+    if (reportedRegexTrees.add(regexTree)) {
+      regexContext.addIssue(regexTree, message);
+      // TODO: Add secondary location to the issue SONARPY-886
+      // TODO: Add cost to the issue SONARPY-893
+    }
+  }
 }

python-checks/src/main/java/org/sonar/python/checks/regex/EmptyStringRepetitionCheck.java

@@ -0,0 +1,34 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2021 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.regex;
+
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonarsource.analyzer.commons.regex.RegexParseResult;
+import org.sonarsource.analyzer.commons.regex.finders.EmptyStringRepetitionFinder;
+
+@Rule(key = "S5842")
+public class EmptyStringRepetitionCheck extends AbstractRegexCheck {
+
+  @Override
+  public void checkRegex(RegexParseResult regexParseResult, CallExpression regexFunctionCall) {
+    new EmptyStringRepetitionFinder(this::addIssue).visit(regexParseResult);
+  }
+}

python-checks/src/main/java/org/sonar/python/checks/regex/StringReplaceCheck.java

@@ -24,6 +24,7 @@
 import java.util.Set;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.python.regex.PythonRegexIssueLocation;
 import org.sonarsource.analyzer.commons.regex.RegexParseResult;
 import org.sonarsource.analyzer.commons.regex.ast.RegexTree;
 import org.sonarsource.analyzer.commons.regex.ast.SequenceTree;
@@ -32,6 +33,7 @@
 public class StringReplaceCheck extends AbstractRegexCheck {
 
   private static final String MESSAGE = "Replace this \"re.sub()\" call by a \"str.replace()\" function call.";
+  private static final String SECONDARY_MESSAGE = "Expression without regular expression features.";
 
   @Override
   protected Set<String> lookedUpFunctionNames() {
@@ -42,7 +44,8 @@ protected Set<String> lookedUpFunctionNames() {
   public void checkRegex(RegexParseResult regexParseResult, CallExpression callExpression) {
     RegexTree regex = regexParseResult.getResult();
     if (!regexParseResult.hasSyntaxErrors() && isPlainString(regex)) {
-      regexContext.addIssue(callExpression.callee(), MESSAGE);
+      regexContext.addIssue(callExpression.callee(), MESSAGE)
+        .secondary(PythonRegexIssueLocation.preciseLocation(regex, SECONDARY_MESSAGE));
     }
   }
 

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

@@ -0,0 +1,16 @@
+<p>A regex should never include a repetitive pattern whose body would match the empty string. This is usually a sign that a part of the regex is
+redundant or does not do what the author intended.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+r"(?:)*"      # same as the empty regex, the '*' accomplishes nothing
+r"(?:|x)*"    # same as the empty regex, the alternative has no effect
+r"(?:x|)*"    # same as 'x*', the empty alternative has no effect
+r"(?:x*|y*)*" # same as 'x*', the first alternative would always match, y* is never tried
+r"(?:x?)*"    # same as 'x*'
+r"(?:x?)+"    # same as 'x*'
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+r"x*"
+</pre>
+

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

@@ -0,0 +1,17 @@
+{
+  "title": "Repeated patterns in regular expressions should not match the empty string",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "regex"
+  ],
+  "defaultSeverity": "Minor",
+  "ruleSpecification": "RSPEC-5842",
+  "sqKey": "S5842",
+  "scope": "Main",
+  "quickfix": "unknown"
+}

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

@@ -133,6 +133,7 @@
     "S5806",
     "S5807",
     "S5828",
+    "S5842",
     "S5864",
     "S5886",
     "S5890"

python-checks/src/test/java/org/sonar/python/checks/regex/AbstractRegexCheckTest.java

@@ -22,6 +22,7 @@
 import java.io.File;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import org.junit.Test;
 import org.sonar.plugins.python.api.PythonVisitorContext;
@@ -44,6 +45,22 @@ public void test_regex_is_visited() {
     assertThat(check.receivedRegexParseResults).hasSize(10);
   }
 
+  @Test
+  public void test_regex_element_is_only_reported_once() {
+    Check check = new Check() {
+      @Override
+      public void checkRegex(RegexParseResult regexParseResult, CallExpression regexFunctionCall) {
+        super.checkRegex(regexParseResult, regexFunctionCall);
+        addIssue(regexParseResult.getResult(), "MESSAGE", null, Collections.emptyList());
+      }
+    };
+
+    PythonVisitorContext fileContext = TestPythonVisitorRunner.createContext(FILE);
+    SubscriptionVisitor.analyze(Collections.singletonList(check), fileContext);
+    assertThat(check.reportedRegexTrees).hasSize(10);
+    assertThat(fileContext.getIssues()).hasSize(10);
+  }
+
   @Test
   public void test_regex_parse_result_is_retrieved_from_cache_in_context() {
     PythonVisitorContext fileContext = TestPythonVisitorRunner.createContext(FILE);
@@ -59,10 +76,11 @@ public void test_regex_parse_result_is_retrieved_from_cache_in_context() {
   private static class Check extends AbstractRegexCheck {
     private final List<RegexParseResult> receivedRegexParseResults = new ArrayList<>();
 
+    @Override
     public void checkRegex(RegexParseResult regexParseResult, CallExpression regexFunctionCall) {
       receivedRegexParseResults.add(regexParseResult);
-      regexContext.addIssue(regexFunctionCall, "MESSAGE");
+      addIssue(regexParseResult.getResult(), "MESSAGE", null, Collections.emptyList());
     }
   }
 
-}
+}

python-checks/src/test/java/org/sonar/python/checks/regex/EmptyStringRepetitionCheckTest.java

@@ -0,0 +1,31 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2021 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.regex;
+
+import org.junit.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+public class EmptyStringRepetitionCheckTest {
+
+  @Test
+  public void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/regex/emptyStringRepetitionCheck.py", new EmptyStringRepetitionCheck());
+  }
+}

python-checks/src/test/resources/checks/regex/emptyStringRepetitionCheck.py

@@ -0,0 +1,47 @@
+import re
+
+
+def non_compliant(input):
+    re.match(r"(?:)*", input)  # Noncompliant {{Rework this part of the regex to not match the empty string.}}
+    #          ^^^^
+    re.match(r"(?:)?", input)  # Noncompliant
+    re.match(r"(?:)+", input)  # Noncompliant
+    re.match(r"()*", input)  # Noncompliant
+    re.match(r"()?", input)  # Noncompliant
+    re.match(r"()+", input)  # Noncompliant
+    re.match(r"xyz|(?:)*", input)  # Noncompliant
+    re.match(r"(?:|x)*", input)  # Noncompliant
+    re.match(r"(?:x|)*", input)  # Noncompliant
+    re.match(r"(?:x|y*)*", input)  # Noncompliant
+    re.match(r"(?:x*|y*)*", input)  # Noncompliant
+    re.match(r"(?:x?|y*)*", input)  # Noncompliant
+    re.match(r"(?:x*)*", input)  # Noncompliant
+    re.match(r"(?:x?)*", input)  # Noncompliant
+    re.match(r"(?:x*)?", input)  # Noncompliant
+    re.match(r"(?:x?)?", input)  # Noncompliant
+    re.match(r"(?:x*)+", input)  # Noncompliant
+    re.match(r"(?:x?)+", input)  # Noncompliant
+    re.match(r"(x*)*", input)  # Noncompliant
+    re.match(r"((x*))*", input)  # Noncompliant
+    re.match(r"(?:x*y*)*", input)  # Noncompliant
+    re.match(r"(?:())*", input)  # Noncompliant
+    re.match(r"(?:(?:))*", input)  # Noncompliant
+    re.match(r"((?i))*", input)  # Noncompliant
+    re.match(r"(())*", input)  # Noncompliant
+    re.match(r"(()x*)*", input)  # Noncompliant
+    re.match(r"(()|x)*", input)  # Noncompliant
+    re.match(r"($)*", input)  # Noncompliant
+    re.match(r"(\b)*", input)  # Noncompliant
+    re.match(r"((?!x))*", input)  # Noncompliant
+
+
+def compliant(input):
+    re.match(r"x*|", input)
+    re.match(r"x*|", input)
+    re.match(r"x*", input)
+    re.match(r"x?", input)
+    re.match(r"(?:x|y)*", input)
+    re.match(r"(?:x+)+", input)
+    re.match(r"(?:x+)*", input)
+    re.match(r"(?:x+)?", input)
+    re.match(r"((x+))*", input)