SONARPHP-1721 S1155 An issue should not be raised even if empty() is used.

[ZiadMarey]

No description provided.

[hashicorp-vault-sonar-prod]

SONARPHP-1721

[felix-pauck-sonarsource]

Works well 🙂.

I think the code can be improved in parts, however, I did not manage to do a proper review yet. Feel free to take a look at what I wrote, and I will do a proper one next week.

[felix-pauck-sonarsource]

The review took a while. Never worked on sonar-php before and, hence, I had to understand a couple of things first. 😉

The implementation works! 👍
The tests appear to be sufficient maybe even complete 👍, although I still had no closer look 😅.

However, I think we must improve the implementation such that it leverages the Visitor pattern. Currently, we are going back and forth in the implementation, which makes it a little hard to understand and introduces some overhead and uncertainty.

In addition to the detailed comments below, I suggest to take inspiration from this alternative implementation - again 😅:

private boolean isEmptyUsedInCondition(FunctionCallTree countCallTree, @Nullable ExpressionTree countArgument) {
    if (!(countArgument instanceof VariableIdentifierTreeImpl countArgumentVariable)) {
      return false;
    }

    Tree condition = countCallTree.getParent();
    while (condition != null) {
      if (...) {
        break;
      }
      condition = condition.getParent();
    }

    EmptyMethodCallCheck emptyCheck = new EmptyMethodCallCheck(context(), countArgumentVariable.symbol());
    condition.accept(emptyCheck);
    return emptyCheck.containsEmpty();
  }

...

  private static class EmptyMethodCallCheck extends PHPVisitorCheck {
    private static final FunctionCall EMPTY_FUNCTION_CALL = new FunctionCall("empty");

    private boolean emptyFound = false;

    CheckContext parentContext;
    Symbol countArgumentSymbol;

    EmptyMethodCallCheck(CheckContext parentContext, Symbol countArgumentSymbol) {
      this.parentContext = parentContext;
      this.countArgumentSymbol = countArgumentSymbol;
    }

    @Override
    public void visitFunctionCall(FunctionCallTree tree) {
      if (!emptyFound && isEmptyFunction(tree) && !tree.callArguments().isEmpty()) {
        ExpressionTree firstArgument = tree.callArguments().get(0).value();
        if (firstArgument.is(Tree.Kind.VARIABLE_IDENTIFIER)
          && firstArgument instanceof VariableIdentifierTreeImpl firstArgumentVariable
          && firstArgumentVariable.symbol() == countArgumentSymbol) {
          emptyFound = true;
        }
      }
      super.visitFunctionCall(tree);
    }

    private boolean isEmptyFunction(FunctionCallTree tree) {
      return EMPTY_FUNCTION_CALL.test(TreeValues.of(tree, parentContext.symbolTable()));
    }

    public boolean containsEmpty() {
      return emptyFound;
    }
  }

[felix-pauck-sonarsource]

We are getting there 😉.
Please double-check previous comments as well 🙂.

[felix-pauck-sonarsource]

Please take another look on all unresolved conversations in the PR.

[felix-pauck-sonarsource]

Let us do a sync 😉

[felix-pauck-sonarsource]

LGTM! 👍

Two comments left which are super easy to fix 😊.
Still not approving as the parent skipping (see comment below) might be critical 😅.
Will immediately approve once fixed 😉.

If you want to increase coverage (there are two uncovered lines), I think you can only do so with some mock-tests. Feel free to add them, but I think they are not needed as both lines will never be reached in a real scenario. Hence, coverage is fine as it is. 👍

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
88.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[felix-pauck-sonarsource]

LGTM! 🎉