SONARPHP-1386 S3415 wrong arguments order in assertSame($expectedCoun… (

#1108)
SonarSource · Aug 17, 2023 · 78eb0d3 · 78eb0d3
1 parent b0751ea
commit 78eb0d3
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 16 deletions.
diff --git a/its/ruling/src/test/resources/expected/PhpSpreadsheet/php-S3415.json b/its/ruling/src/test/resources/expected/PhpSpreadsheet/php-S3415.json
@@ -19,8 +19,10 @@
 108
 ],
 "PhpSpreadsheet:tests/PhpSpreadsheetTests/Cell/CoordinateTest.php": [
+24,
 40,
 55,
+74,
 121,
 136,
 183,
@@ -48,18 +50,12 @@
 100,
 103
 ],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/ColumnCellIterator2Test.php": [
-67
-],
 "PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/DrawingTest.php": [
 57,
 58,
 84,
 85
 ],
-"PhpSpreadsheet:tests/PhpSpreadsheetTests/Worksheet/RowCellIterator2Test.php": [
-67
-],
 "PhpSpreadsheet:tests/PhpSpreadsheetTests/Writer/Html/VisibilityTest.php": [
 31,
 34,

diff --git a/its/ruling/src/test/resources/expected/psysh/php-S3415.json b/its/ruling/src/test/resources/expected/psysh/php-S3415.json
@@ -12,6 +12,7 @@
 82
 ],
 "psysh:test/ShellTest.php": [
-385
+385,
+593
 ]
 }
diff --git a/php-checks/src/main/java/org/sonar/php/checks/phpunit/AssertionArgumentOrderCheck.java b/php-checks/src/main/java/org/sonar/php/checks/phpunit/AssertionArgumentOrderCheck.java
@@ -20,14 +20,19 @@
 package org.sonar.php.checks.phpunit;
 
 import java.util.Optional;
+import javax.annotation.CheckForNull;
 import org.sonar.check.Rule;
 import org.sonar.php.checks.utils.PhpUnitCheck;
+import org.sonar.php.tree.TreeUtils;
 import org.sonar.plugins.php.api.tree.SeparatedList;
+import org.sonar.plugins.php.api.tree.Tree;
 import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
+import org.sonar.plugins.php.api.tree.declaration.MethodDeclarationTree;
+import org.sonar.plugins.php.api.tree.declaration.ParameterTree;
 import org.sonar.plugins.php.api.tree.expression.ExpressionTree;
 import org.sonar.plugins.php.api.tree.expression.FunctionCallTree;
+import org.sonar.plugins.php.api.tree.expression.IdentifierTree;
 import org.sonar.plugins.php.api.tree.expression.MemberAccessTree;
-import org.sonar.plugins.php.api.tree.expression.NameIdentifierTree;
 
 import static org.sonar.php.checks.utils.CheckUtils.assignedValue;
 import static org.sonar.php.checks.utils.CheckUtils.hasNamedArgument;
@@ -52,23 +57,47 @@ public void visitFunctionCall(FunctionCallTree tree) {
     if (arguments.size() >= 2 && assertion.isPresent() && assertion.get().hasExpectedValue() && !hasNamedArgument(tree)) {
       ExpressionTree expected = arguments.get(0).value();
       ExpressionTree actual = arguments.get(1).value();
-      if (isLiteralOrClassName(actual) && !isLiteralOrClassName(expected)) {
+      if (isLiteralOrClassNameOrParameter(actual) && !isLiteralOrClassNameOrParameter(expected)) {
         newIssue(actual, MESSAGE).secondary(expected, SECONDARY_MESSAGE);
       }
     }
 
     super.visitFunctionCall(tree);
   }
 
-  private static boolean isLiteralOrClassName(ExpressionTree expression) {
+  private static boolean isLiteralOrClassNameOrParameter(ExpressionTree expression) {
     return assignedValue(expression).is(LITERAL) ||
-      (expression instanceof MemberAccessTree &&
-        isStaticAccessWithName((MemberAccessTree) expression, "class"));
+      isStaticAccessWithName(expression, "class") ||
+      isDefinedAsParameter(expression);
   }
 
-  private static boolean isStaticAccessWithName(MemberAccessTree tree, String memberName) {
-    return tree.isStatic() &&
-      tree.member().is(Kind.NAME_IDENTIFIER) &&
-      memberName.equals(((NameIdentifierTree) tree.member()).text());
+  private static boolean isStaticAccessWithName(ExpressionTree expression, String memberName) {
+    if (expression instanceof MemberAccessTree) {
+      MemberAccessTree tree = (MemberAccessTree) expression;
+      return tree.isStatic() && memberName.equals(name(tree.member()));
+    }
+    return false;
+  }
+
+  private static boolean isDefinedAsParameter(ExpressionTree expression) {
+    MethodDeclarationTree method = (MethodDeclarationTree) TreeUtils.findAncestorWithKind(expression, Kind.METHOD_DECLARATION);
+    if (method != null) {
+      String name = name(expression);
+      for (ParameterTree parameter : method.parameters().parameters()) {
+        String text = parameter.variableIdentifier().text();
+        if (text.equals(name)) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  @CheckForNull
+  private static String name(Tree expression) {
+    if (expression instanceof IdentifierTree) {
+      return ((IdentifierTree) expression).text();
+    }
+    return null;
   }
 }
diff --git a/php-checks/src/test/resources/checks/phpunit/AssertionArgumentOrderCheck.php b/php-checks/src/test/resources/checks/phpunit/AssertionArgumentOrderCheck.php
@@ -32,9 +32,22 @@ public function test()
     $this->assertInstanceOf($e, LogicException::class);
     $this->assertInstanceOf(\LogicException::class, $e);
     $this->assertNotInstanceOf(LogicException::class, $e);
+
+    self::assertSame(0, $runner.getExitCode()); // OK
+    $this->assertSame($expected, $runner.getExitCode()); // OK
+    $this->assertSame($runner.getExitCode(), $expected); // Noncompliant
+    $this->assertNotSame($expected, $runner.getExitCode()); // OK
+    $this->assertNotSame($runner.getExitCode(), $expected); // Noncompliant
   }
 
   private function no_test() {
     doSomethind();
   }
+
+  public function testExport(bool $foobar, bool $staticValueExpected = false)
+  {
+    $isStaticValue = true;
+    $this->assertSame($staticValueExpected, $isStaticValue); // OK
+    $this->assertSame(true, $isStaticValue); // OK
+  }
 }
diff --git a/php-frontend/src/main/java/org/sonar/php/tree/TreeUtils.java b/php-frontend/src/main/java/org/sonar/php/tree/TreeUtils.java
@@ -31,6 +31,8 @@
 import org.sonar.php.tree.impl.PHPTree;
 import org.sonar.plugins.php.api.tree.Tree;
 
+import static java.util.Arrays.asList;
+
 public class TreeUtils {
 
   private TreeUtils() {
@@ -54,6 +56,11 @@ public static Tree findAncestorWithKind(Tree tree, Collection<Tree.Kind> kinds)
     return parent;
   }
 
+  @CheckForNull
+  public static Tree findAncestorWithKind(Tree tree, Tree.Kind... kinds) {
+    return findAncestorWithKind(tree, asList(kinds));
+  }
+
   public static Stream<Tree> descendants(@Nullable Tree root) {
     if (root == null || ((PHPTree) root).isLeaf()) {
       return Stream.empty();

diff --git a/php-frontend/src/test/java/org/sonar/php/tree/TreeUtilsTest.java b/php-frontend/src/test/java/org/sonar/php/tree/TreeUtilsTest.java
@@ -57,6 +57,8 @@ public void test_findAncestorWithKind() {
     assertThat(findAncestorWithKind(statementTree, singletonList(Tree.Kind.WHILE_STATEMENT))).isNull();
 
     assertThat(findAncestorWithKind(func, singletonList(Tree.Kind.FUNCTION_DECLARATION))).isEqualTo(func);
+    assertThat(findAncestorWithKind(func, Tree.Kind.FUNCTION_DECLARATION)).isEqualTo(func);
+    assertThat(findAncestorWithKind(func, Tree.Kind.SCRIPT, Tree.Kind.FUNCTION_DECLARATION)).isEqualTo(func);
   }
 
 }