Skip to content

Commit

Permalink
SONARPHP-1476 S1144 should not raise an issue when a magic method is …
Browse files Browse the repository at this point in the history
…available via a trait (#1152)
  • Loading branch information
jonas-wielage-sonarsource authored Oct 25, 2023
1 parent 0f1aaca commit 5f84cac
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,11 @@
import org.sonar.check.Rule;
import org.sonar.php.checks.utils.CheckUtils;
import org.sonar.php.symbols.ClassSymbol;
import org.sonar.php.symbols.MethodSymbol;
import org.sonar.php.tree.TreeUtils;
import org.sonar.php.tree.impl.PHPTree;
import org.sonar.php.tree.impl.declaration.ClassNamespaceNameTreeImpl;
import org.sonar.php.tree.impl.expression.FunctionCallTreeImpl;
import org.sonar.php.tree.symbols.HasClassSymbol;
import org.sonar.php.tree.symbols.Scope;
import org.sonar.plugins.php.api.symbols.Symbol;
import org.sonar.plugins.php.api.symbols.Symbol.Kind;
import org.sonar.plugins.php.api.tree.Tree;
Expand All @@ -48,8 +47,10 @@
import org.sonar.plugins.php.api.tree.expression.MemberAccessTree;
import org.sonar.plugins.php.api.tree.expression.NameIdentifierTree;
import org.sonar.plugins.php.api.tree.expression.VariableIdentifierTree;
import org.sonar.plugins.php.api.tree.statement.UseTraitDeclarationTree;
import org.sonar.plugins.php.api.visitors.PHPVisitorCheck;

import static org.sonar.plugins.php.api.tree.Tree.Kind.USE_TRAIT_DECLARATION;
import static org.sonar.plugins.php.api.tree.Tree.Kind.VARIABLE_IDENTIFIER;

@Rule(key = "S1144")
Expand Down Expand Up @@ -90,7 +91,7 @@ public void visitMethodDeclaration(MethodDeclarationTree tree) {
@Override
public void visitCallableConvert(CallableConvertTree tree) {
if (tree.expression().is(Tree.Kind.OBJECT_MEMBER_ACCESS)) {
MemberAccessTree memberAccessTree = (MemberAccessTree) tree.expression();
var memberAccessTree = (MemberAccessTree) tree.expression();
if (isThis(memberAccessTree.object()) && memberAccessTree.member().is(Tree.Kind.NAME_IDENTIFIER)) {
methodsUsedInFirstClassCallables.add(((NameIdentifierTree) memberAccessTree.member()).text().toLowerCase(Locale.ROOT));
}
Expand All @@ -100,7 +101,7 @@ public void visitCallableConvert(CallableConvertTree tree) {
}

private void checkClass(ClassTree tree) {
Scope classScope = context().symbolTable().getScopeFor(tree);
var classScope = context().symbolTable().getScopeFor(tree);
for (Symbol methodSymbol : classScope.getSymbols(Kind.FUNCTION)) {

// For enums private and protected are equivalent as inheritance is not allowed.
Expand All @@ -124,12 +125,12 @@ private static boolean isMagicMethodCallDefined(ClassTree tree) {
Optional<ClassMemberTree> magicMethodCall = findMagicMethodCall((ClassDeclarationTree) tree);
if (magicMethodCall.isPresent() && containsCallUserFunction(magicMethodCall.get())) {
return true;
} else {
Optional<ClassSymbol> superClass = ((HasClassSymbol) tree).symbol().superClass();
if (superClass.isPresent()) {
return containsSuperClassCallMethod(superClass.get());
}
}
Optional<ClassSymbol> superClass = ((HasClassSymbol) tree).symbol().superClass();
if (superClass.isPresent() && containsSuperClassCallMethod(superClass.get())) {
return true;
}
return containsTraitWithMagicMethodCall(tree);
}
return false;
}
Expand All @@ -150,10 +151,7 @@ private static boolean containsCallUserFunction(ClassMemberTree magicMethodCall)
}

private static boolean containsSuperClassCallMethod(ClassSymbol tree) {
Optional<MethodSymbol> callMethod = tree.declaredMethods().stream()
.filter(m -> "__call".equals(m.name()))
.findFirst();
if (callMethod.isPresent()) {
if (containsMagicMethodCall(tree)) {
return true;
}
Optional<ClassSymbol> superClass = tree.superClass();
Expand All @@ -163,6 +161,21 @@ private static boolean containsSuperClassCallMethod(ClassSymbol tree) {
return false;
}

private static boolean containsTraitWithMagicMethodCall(ClassTree tree) {
return tree.members().stream()
.filter(member -> member.is(USE_TRAIT_DECLARATION))
.map(member -> ((UseTraitDeclarationTree) member).traits())
.flatMap(List::stream)
.filter(ClassNamespaceNameTreeImpl.class::isInstance)
.map(trait -> ((ClassNamespaceNameTreeImpl) trait).symbol())
.anyMatch(UnusedPrivateMethodCheck::containsMagicMethodCall);
}

private static boolean containsMagicMethodCall(ClassSymbol tree) {
return tree.declaredMethods().stream()
.anyMatch(m -> m.name().startsWith("__call"));
}

@Override
public void visitAnonymousClass(AnonymousClassTree tree) {
stringLiterals.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@
class UnusedPrivateMethodCheckTest {

@Test
void test() {
void shouldRaiseIssuesCorrectly() {
CheckVerifier.verify(new UnusedPrivateMethodCheck(), "UnusedPrivateMethodCheck.php");
}

@Test
void shouldRaiseIssuesWhenEncounteringTraits() {
CheckVerifier.verify(new UnusedPrivateMethodCheck(), "UnusedPrivateMethodCheckWithTraits.php");
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<?php

class MagicMethodCallBase
{
use MyTraitWithMagicMethodCallWithCallUserFuncArrayFunction;

private function bar()
{
// Compliant as bar() can be called in the trait
}
}

class MagicMethodCallChild extends MagicMethodCallBase
{

// TODO SONARPHP-1482 FP because we can't access traits of a superclass in symbols
private function foo() // Noncompliant
{
}
}

trait MyTraitWithMagicMethodCallWithCallUserFuncArrayFunction
{
public function __call($method, $arguments)
{
if (method_exists($this, $method)) {
return call_user_func_array([$this, $method], $arguments);
}
trigger_error('Call to undefined method '.__CLASS__.'::'.$method.'()', E_USER_ERROR);
}
}

class MagicMethodCallFalseNegativeBase
{
use MyTraitWithMagicMethodCallWithoutBody;

// TODO SONARPHP-1481 FN because in trait in __call the call of call_user_func_array or call_user_func is missing
private function bar()
{
}
}

trait MyTraitWithMagicMethodCallWithoutBody
{
public function __call($method, $arguments)
{
// The call of call_user_func_array or call_user_func is missing
}
}

class MagicMethodCallBase2
{
use MyTraitWithoutMagicMethodCall;

private function bar() // Noncompliant
{
}
}

class MagicMethodCallChild2 extends MagicMethodCallBase2
{
private function foo() // Noncompliant
{
}
}

trait MyTraitWithoutMagicMethodCall
{
public function foo()
{
}
}

class MagicMethodCall3
{
use NonExistingTrait;

private function bar() // Noncompliant
{
}
}

class MagicMethodCall4 extends MagicMethodCall3
{
private function foo() // Noncompliant
{
}
}

0 comments on commit 5f84cac

Please sign in to comment.