SONARPY-911 Provide global regex flags to the parser (#968)

karim-ouerghemmi-sonarsource · web-flow · commit 351d3dac6b26 · 2021-10-29T16:13:45.000+02:00
* SONARPY-911 Provide global regex flags to the parser * Add Pattern.UNICODE_CASE by default and fix code smell * Add some tests for flags * Some fixes after rebase on master
diff --git a/python-checks/src/main/java/org/sonar/python/checks/regex/AbstractRegexCheck.java b/python-checks/src/main/java/org/sonar/python/checks/regex/AbstractRegexCheck.java
@@ -19,17 +19,22 @@
  */
 package org.sonar.python.checks.regex;
 
-import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.regex.Pattern;
 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;
+import org.sonar.plugins.python.api.tree.BinaryExpression;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.QualifiedExpression;
 import org.sonar.plugins.python.api.tree.RegularArgument;
 import org.sonar.plugins.python.api.tree.StringLiteral;
 import org.sonar.plugins.python.api.tree.Tree;
@@ -38,19 +43,36 @@
 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.FlagSet;
 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"));
+  private static final Map<String, Integer> REGEX_FUNCTIONS_TO_FLAG_PARAM = new HashMap<>();
+
+  static {
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.sub", 4);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.subn", 4);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.compile", null);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.search", 2);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.match", 2);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.fullmatch", 2);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.split", 3);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.findall", 2);
+    REGEX_FUNCTIONS_TO_FLAG_PARAM.put("re.finditer", 2);
+  }
+
   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;
+  /**
+   * Should return a map whose keys are the functions the check is interested in, and the values are the position of the flags parameter.
+   * Set the position of the flags parameter to {@code null} if there is none.
+   */
+  protected Map<String, Integer> lookedUpFunctions() {
+    return REGEX_FUNCTIONS_TO_FLAG_PARAM;
   }
 
   @Override
@@ -67,17 +89,20 @@ private void checkCall(SubscriptionContext ctx) {
     if (calleeSymbol == null || calleeSymbol.fullyQualifiedName() == null) {
       return;
     }
-    if (lookedUpFunctionNames().contains(calleeSymbol.fullyQualifiedName())) {
+    String functionFqn = calleeSymbol.fullyQualifiedName();
+    if (functionFqn != null && lookedUpFunctions().containsKey(functionFqn)) {
+      FlagSet flagSet = getFlagSet(callExpression, functionFqn);
+
       patternArgStringLiteral(callExpression)
-        .flatMap(this::regexForStringLiteral)
+        .flatMap(l -> regexForStringLiteral(l, flagSet))
         .ifPresent(parseResult -> checkRegex(parseResult, callExpression));
     }
   }
 
-  private Optional<RegexParseResult> regexForStringLiteral(StringLiteral literal) {
+  private Optional<RegexParseResult> regexForStringLiteral(StringLiteral literal, FlagSet flagSet) {
     // TODO: for now we only handle strings with an "r" prefix. This will be extended.
     if (literal.stringElements().size() == 1 && "r".equalsIgnoreCase(literal.stringElements().get(0).prefix())) {
-      return Optional.of(regexContext.regexForStringElement(literal.stringElements().get(0)));
+      return Optional.of(regexContext.regexForStringElement(literal.stringElements().get(0), flagSet));
     }
     return Optional.empty();
   }
@@ -94,6 +119,94 @@ private static Optional<StringLiteral> patternArgStringLiteral(CallExpression re
     return Optional.empty();
   }
 
+  private FlagSet getFlagSet(CallExpression callExpression, String functionFqn) {
+    HashSet<QualifiedExpression> flags = new HashSet<>();
+    getFlagsArgValue(callExpression, lookedUpFunctions().get(functionFqn)).ifPresent(f -> flags.addAll(extractFlagExpressions(f)));
+    FlagSet flagSet = new FlagSet();
+    flags.stream()
+      .map(AbstractRegexCheck::mapPythonFlag)
+      .filter(Optional::isPresent)
+      .map(Optional::get)
+      .forEach(flagSet::add);
+
+    // TODO: Don't do this when PYTHON_VERSION is 2
+    // We used Pattern.LITERAL to represent re.ASCII. So we are checking if re.ASCII is set here.
+    // For python3 matches are Unicode by default, and re.ASCII can be used to deactivate that.
+    if (!flagSet.contains(Pattern.LITERAL)) {
+      flagSet.add(Pattern.UNICODE_CHARACTER_CLASS);
+      flagSet.add(Pattern.UNICODE_CASE);
+    }
+    flagSet.removeAll(new FlagSet(Pattern.LITERAL));
+
+    return flagSet;
+  }
+
+  private static Optional<Expression> getFlagsArgValue(CallExpression regexFunctionCall, @Nullable Integer argPosition) {
+    if (argPosition == null) {
+      return Optional.empty();
+    }
+    RegularArgument patternArgument = TreeUtils.nthArgumentOrKeyword(argPosition, "flags", regexFunctionCall.arguments());
+    return patternArgument != null ? Optional.of(patternArgument.expression()) : Optional.empty();
+  }
+
+  private static HashSet<QualifiedExpression> extractFlagExpressions(Tree flagsSubexpr) {
+    if (flagsSubexpr.is(Tree.Kind.QUALIFIED_EXPR)) {
+      return new HashSet<>(Collections.singletonList((QualifiedExpression) flagsSubexpr));
+    } else if (flagsSubexpr.is(Tree.Kind.BITWISE_OR)) {
+      // recurse into left and right branch
+      BinaryExpression orExpr = (BinaryExpression) flagsSubexpr;
+      HashSet<QualifiedExpression> flags = extractFlagExpressions(orExpr.leftOperand());
+      flags.addAll(extractFlagExpressions(orExpr.rightOperand()));
+      return flags;
+    } else {
+      // failed to interpret. Ignore leaf.
+      return new HashSet<>();
+    }
+  }
+
+  public static Optional<Integer> mapPythonFlag(QualifiedExpression ch) {
+    Symbol symbol = ch.symbol();
+    if (symbol == null) {
+      return Optional.empty();
+    }
+    String symbolFqn = symbol.fullyQualifiedName();
+    if (symbolFqn == null) {
+      return Optional.empty();
+    }
+
+    Integer result;
+    switch (symbolFqn) {
+      case "re.IGNORECASE":
+      case "re.I":
+        result = Pattern.CASE_INSENSITIVE;
+        break;
+      case "re.MULTILINE":
+      case "re.M":
+        result = Pattern.MULTILINE;
+        break;
+      case "re.DOTALL":
+      case "re.S":
+        result = Pattern.DOTALL;
+        break;
+      case "re.VERBOSE":
+      case "re.X":
+        result = Pattern.COMMENTS;
+        break;
+      case "re.UNICODE":
+      case "re.U":
+        result = Pattern.UNICODE_CHARACTER_CLASS;
+        break;
+      case "re.ASCII":
+      case "re.A":
+        // We misuse Pattern.LITERAL to represent re.ASCII. It will be removed before being provided to the parser.
+        result = Pattern.LITERAL;
+        break;
+      default:
+        result = null;
+    }
+    return Optional.ofNullable(result);
+  }
+
   public void addIssue(RegexSyntaxElement regexTree, String message, @Nullable Integer cost, List<RegexIssueLocation> secondaries) {
     if (reportedRegexTrees.add(regexTree)) {
       PreciseIssue issue = regexContext.addIssue(regexTree, message);
diff --git a/python-checks/src/main/java/org/sonar/python/checks/regex/StringReplaceCheck.java b/python-checks/src/main/java/org/sonar/python/checks/regex/StringReplaceCheck.java
@@ -19,9 +19,8 @@
  */
 package org.sonar.python.checks.regex;
 
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.HashMap;
+import java.util.Map;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.python.regex.PythonRegexIssueLocation;
@@ -36,8 +35,10 @@ public class StringReplaceCheck extends AbstractRegexCheck {
   private static final String SECONDARY_MESSAGE = "Expression without regular expression features.";
 
   @Override
-  protected Set<String> lookedUpFunctionNames() {
-    return new HashSet<>(Collections.singletonList("re.sub"));
+  protected Map<String, Integer> lookedUpFunctions() {
+    Map<String, Integer> result = new HashMap<>();
+    result.put("re.sub", 4);
+    return result;
   }
 
   @Override
diff --git a/python-checks/src/test/java/org/sonar/python/checks/regex/AbstractRegexCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/regex/AbstractRegexCheckTest.java
@@ -24,13 +24,15 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.regex.Pattern;
 import org.junit.Test;
 import org.sonar.plugins.python.api.PythonVisitorContext;
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.python.SubscriptionVisitor;
 import org.sonar.python.TestPythonVisitorRunner;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
 import org.sonarsource.analyzer.commons.regex.RegexParseResult;
+import org.sonarsource.analyzer.commons.regex.ast.FlagSet;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -42,7 +44,6 @@ public class AbstractRegexCheckTest {
   public void test_regex_is_visited() {
     Check check = new Check();
     PythonCheckVerifier.verify(FILE.getAbsolutePath(), check);
-    assertThat(check.receivedRegexParseResults).hasSize(10);
   }
 
   @Test
@@ -73,13 +74,58 @@ public void test_regex_parse_result_is_retrieved_from_cache_in_context() {
     }
   }
 
+  @Test
+  public void test_flags() {
+    Check check = new Check(true);
+    PythonCheckVerifier.verify("src/test/resources/checks/regex/abstractRegexCheckFlags.py", check);
+  }
+
   private static class Check extends AbstractRegexCheck {
     private final List<RegexParseResult> receivedRegexParseResults = new ArrayList<>();
+    private boolean printFlags;
+
+    private Check() {
+      new Check(false);
+    }
+
+    private Check(boolean printFlags) {
+      this.printFlags = printFlags;
+    }
 
     @Override
     public void checkRegex(RegexParseResult regexParseResult, CallExpression regexFunctionCall) {
       receivedRegexParseResults.add(regexParseResult);
-      addIssue(regexParseResult.getResult(), "MESSAGE", null, Collections.emptyList());
+      addIssue(regexParseResult.getResult(), printFlags ? flagsToString(regexParseResult) : "MESSAGE", null, Collections.emptyList());
+    }
+
+    private String flagsToString(RegexParseResult regexParseResult) {
+      List<String> flags = new ArrayList<>();
+      FlagSet flagSet = regexParseResult.getInitialFlags();
+      if (flagSet.contains(Pattern.CASE_INSENSITIVE)) {
+        flags.add("CASE_INSENSITIVE");
+      }
+      if (flagSet.contains(Pattern.MULTILINE)) {
+        flags.add("MULTILINE");
+      }
+      if (flagSet.contains(Pattern.UNICODE_CASE)) {
+        flags.add("UNICODE_CASE");
+      }
+      if (flagSet.contains(Pattern.UNICODE_CHARACTER_CLASS)) {
+        flags.add("UNICODE_CHARACTER_CLASS");
+      }
+      if (flagSet.contains(Pattern.DOTALL)) {
+        flags.add("DOTALL");
+      }
+      if (flagSet.contains(Pattern.COMMENTS)) {
+        flags.add("VERBOSE");
+      }
+      if (!flagSet.contains(Pattern.UNICODE_CHARACTER_CLASS)) {
+        flags.add("ASCII");
+      }
+      if (flags.isEmpty()) {
+        return "NO FLAGS";
+      }
+      return String.join("|", flags);
     }
   }
 
diff --git a/python-checks/src/test/resources/checks/regex/abstractRegexCheckFlags.py b/python-checks/src/test/resources/checks/regex/abstractRegexCheckFlags.py
@@ -0,0 +1,25 @@
+import re
+
+re.match(r'.*', "foo", re.I); # Noncompliant {{CASE_INSENSITIVE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+re.match(r'.*', "foo", re.IGNORECASE); # Noncompliant {{CASE_INSENSITIVE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+
+re.match(r'.*', "foo", re.M); # Noncompliant {{MULTILINE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+re.match(r'.*', "foo", re.MULTILINE); # Noncompliant {{MULTILINE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+
+re.match(r'.*', "foo", re.S); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS|DOTALL}}
+re.match(r'.*', "foo", re.DOTALL); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS|DOTALL}}
+
+re.match(r'.*', "foo", re.X); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS|VERBOSE}}
+re.match(r'.*', "foo", re.VERBOSE); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS|VERBOSE}}
+
+re.match(r'.*', "foo", re.U); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+re.match(r'.*', "foo", re.UNICODE); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+
+re.match(r'.*', "foo", re.A); # Noncompliant {{ASCII}}
+re.match(r'.*', "foo", re.ASCII); # Noncompliant {{ASCII}}
+
+re.match(r'.*', "foo", re.UNKNOWN); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+re.match(r'.*', "foo", not_re.UNKNOWN); # Noncompliant {{UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+re.match(r'.*', "foo", re.I|UNKNOWN); # Noncompliant {{CASE_INSENSITIVE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
+
+re.match(r'.*', "foo", re.I|re.M); # Noncompliant {{CASE_INSENSITIVE|MULTILINE|UNICODE_CASE|UNICODE_CHARACTER_CLASS}}
diff --git a/python-frontend/src/main/java/org/sonar/python/SubscriptionVisitor.java b/python-frontend/src/main/java/org/sonar/python/SubscriptionVisitor.java
@@ -168,10 +168,9 @@ public File workingDirectory() {
       return pythonVisitorContext.workingDirectory();
     }
 
-    public RegexParseResult regexForStringElement(StringElement stringElement) {
-      // TODO: Real flags have to be provided
+    public RegexParseResult regexForStringElement(StringElement stringElement, FlagSet flagSet) {
       return regexCache.computeIfAbsent(stringElement,
-        s -> new RegexParser(new PythonAnalyzerRegexSource(s), new FlagSet()).parse());
+        s -> new RegexParser(new PythonAnalyzerRegexSource(s), flagSet).parse());
     }
   }
 }
diff --git a/python-frontend/src/main/java/org/sonar/python/regex/RegexContext.java b/python-frontend/src/main/java/org/sonar/python/regex/RegexContext.java
@@ -24,11 +24,12 @@
 import org.sonar.plugins.python.api.tree.StringElement;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonarsource.analyzer.commons.regex.RegexParseResult;
+import org.sonarsource.analyzer.commons.regex.ast.FlagSet;
 import org.sonarsource.analyzer.commons.regex.ast.RegexSyntaxElement;
 
 public interface RegexContext {
 
-  RegexParseResult regexForStringElement(StringElement stringElement);
+  RegexParseResult regexForStringElement(StringElement stringElement, FlagSet flagSet);
 
   PythonCheck.PreciseIssue addIssue(Tree element, @Nullable String message);
 

Original file line number	Diff line number	Diff line change
`@@ -168,10 +168,9 @@ public File workingDirectory() {`
`168`	`168`	`return pythonVisitorContext.workingDirectory();`
`169`	`169`	`}`
`170`	`170`
`171`		`- public RegexParseResult regexForStringElement(StringElement stringElement) {`
`172`		`- // TODO: Real flags have to be provided`
	`171`	`+ public RegexParseResult regexForStringElement(StringElement stringElement, FlagSet flagSet) {`
`173`	`172`	`return regexCache.computeIfAbsent(stringElement,`
`174`		`- s -> new RegexParser(new PythonAnalyzerRegexSource(s), new FlagSet()).parse());`
	`173`	`+ s -> new RegexParser(new PythonAnalyzerRegexSource(s), flagSet).parse());`
`175`	`174`	`}`
`176`	`175`	`}`
`177`	`176`	`}`