SONARPY-243 FP in UnusedLocalVariableCheck with literal string interpolation (SonarSource#126)

alban-auzeill · web-flow · commit 4216962d57e6 · 2018-01-09T16:55:39.000+01:00
diff --git a/python-checks/src/main/java/org/sonar/python/checks/CheckUtils.java b/python-checks/src/main/java/org/sonar/python/checks/CheckUtils.java
@@ -22,12 +22,17 @@
 import com.sonar.sslr.api.AstNode;
 import com.sonar.sslr.api.Token;
 import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import org.sonar.python.api.PythonGrammar;
 import org.sonar.python.api.PythonPunctuator;
 import org.sonar.python.api.PythonTokenType;
 
 public class CheckUtils {
 
+  private static final Pattern STRING_INTERPOLATION_PREFIX = Pattern.compile("^[^'\"fF]*+[fF]");
+  private static final Pattern STRING_LITERAL_QUOTE = Pattern.compile("[\"']");
+
   private CheckUtils() {
 
   }
@@ -98,4 +103,18 @@ public static boolean containsValue(List<Token> list, String value) {
     }
     return false;
   }
+
+  public static boolean isStringInterpolation(Token token) {
+    return token.getType().equals(PythonTokenType.STRING) &&
+      STRING_INTERPOLATION_PREFIX.matcher(token.getOriginalValue()).find();
+  }
+
+  public static String stringLiteralContent(String stringLiteral) {
+    Matcher quote = STRING_LITERAL_QUOTE.matcher(stringLiteral);
+    if (!quote.find()) {
+      throw new IllegalStateException("Invalid string literal: " + stringLiteral);
+    }
+    return stringLiteral.substring(quote.end(), stringLiteral.length() - 1);
+  }
+
 }
diff --git a/python-checks/src/main/java/org/sonar/python/checks/UnusedLocalVariableCheck.java b/python-checks/src/main/java/org/sonar/python/checks/UnusedLocalVariableCheck.java
@@ -21,8 +21,12 @@
 
 import com.sonar.sslr.api.AstNode;
 import com.sonar.sslr.api.AstNodeType;
+import com.sonar.sslr.api.Token;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 import org.sonar.check.Rule;
 import org.sonar.python.PythonCheck;
 import org.sonar.python.api.PythonGrammar;
@@ -31,6 +35,8 @@
 @Rule(key = "S1481")
 public class UnusedLocalVariableCheck extends PythonCheck {
 
+  private static final Pattern IDENTIFIER_SEPARATOR = Pattern.compile("[^a-zA-Z0-9_]+");
+
   private static final String MESSAGE = "Remove the unused local variable \"%s\".";
 
   @Override
@@ -44,8 +50,11 @@ public void visitNode(AstNode functionTree) {
     if (isCallingLocalsFunction(functionTree)) {
       return;
     }
+    Set<String> interpolationIdentifiers = extractStringInterpolationIdentifiers(functionTree);
     for (Symbol symbol : getContext().symbolTable().symbols(functionTree)) {
-      checkSymbol(symbol);
+      if (!interpolationIdentifiers.contains(symbol.name())) {
+        checkSymbol(symbol);
+      }
     }
   }
 
@@ -56,6 +65,16 @@ private static boolean isCallingLocalsFunction(AstNode functionTree) {
       .anyMatch(node -> "locals".equals(node.getTokenValue()));
   }
 
+  private static Set<String> extractStringInterpolationIdentifiers(AstNode functionTree) {
+    return functionTree.getTokens().stream()
+      .filter(CheckUtils::isStringInterpolation)
+      .map(Token::getOriginalValue)
+      .map(CheckUtils::stringLiteralContent)
+      .map(IDENTIFIER_SEPARATOR::split)
+      .flatMap(Arrays::stream)
+      .collect(Collectors.toSet());
+  }
+
   private void checkSymbol(Symbol symbol) {
     if (symbol.readUsages().isEmpty()) {
       for (AstNode writeUsage : symbol.writeUsages()) {
diff --git a/python-checks/src/test/java/org/sonar/python/checks/CheckUtilsTest.java b/python-checks/src/test/java/org/sonar/python/checks/CheckUtilsTest.java
@@ -22,19 +22,27 @@
 import com.google.common.collect.ImmutableSet;
 import com.sonar.sslr.api.AstNode;
 import com.sonar.sslr.api.AstNodeType;
+import com.sonar.sslr.api.Grammar;
+import com.sonar.sslr.impl.Parser;
 import java.lang.reflect.Constructor;
+import java.nio.charset.StandardCharsets;
 import java.util.Set;
+import java.util.function.Function;
 import org.junit.Test;
 import org.sonar.python.PythonCheck;
+import org.sonar.python.PythonConfiguration;
 import org.sonar.python.api.PythonGrammar;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
+import org.sonar.python.parser.PythonParser;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 public class CheckUtilsTest {
 
+  private static final Parser<Grammar> PARSER = PythonParser.create(new PythonConfiguration(StandardCharsets.UTF_8));
+
   @Test
   public void private_constructor() throws Exception {
     Constructor constructor = CheckUtils.class.getDeclaredConstructor();
@@ -113,4 +121,29 @@ public void visitNode(AstNode astNode) {
     }
   }
 
+  @Test
+  public void string_interpolation() throws Exception {
+    Function<String, Boolean> isStringInterpolation = (source) -> CheckUtils.isStringInterpolation(PARSER.parse(source).getTokens().get(0));
+    assertThat(isStringInterpolation.apply("\"abc\"")).isFalse();
+    assertThat(isStringInterpolation.apply("r'abc'")).isFalse();
+    assertThat(isStringInterpolation.apply("f'abc'")).isTrue();
+    assertThat(isStringInterpolation.apply("Rf'abc'")).isTrue();
+    assertThat(isStringInterpolation.apply("rF'abc'")).isTrue();
+    assertThat(isStringInterpolation.apply("fr\"\"")).isTrue();
+  }
+
+  @Test
+  public void string_literal_content() throws Exception {
+    Function<String, String> stringLiteralContent = (source) ->
+      CheckUtils.stringLiteralContent(PARSER.parse(source).getTokens().get(0).getOriginalValue());
+    assertThat(stringLiteralContent.apply("\"abc\"")).isEqualTo("abc");
+    assertThat(stringLiteralContent.apply("r''")).isEqualTo("");
+    assertThat(stringLiteralContent.apply("fr\"abc abc\"")).isEqualTo("abc abc");
+  }
+
+  @Test(expected = IllegalStateException.class)
+  public void invalid_string_literal_content() throws Exception {
+    CheckUtils.stringLiteralContent(PARSER.parse("2").getTokens().get(0).getOriginalValue());
+  }
+
 }
diff --git a/python-checks/src/test/resources/checks/unusedLocalVariable.py b/python-checks/src/test/resources/checks/unusedLocalVariable.py
@@ -15,3 +15,10 @@ def using_locals(a, b):
   c = a + b
   # "locals" will include the "c" value
   return locals()
+
+def string_interpolation():
+    value1 = 1
+    value2 = 2
+    value3 = 3 # Noncompliant
+    value4 = 4 # false-negative, value4 is not used as a variable in the string interpolation, see SONARPY-245
+    return f'{value1}, {2*value2}, value3bis, value4'
