Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--strict option is ignored by CountInLoopExpression rule #1020

Closed
6 tasks done
DannyvdSluijs opened this issue Jul 17, 2023 · 2 comments · Fixed by #1044
Closed
6 tasks done

--strict option is ignored by CountInLoopExpression rule #1020

DannyvdSluijs opened this issue Jul 17, 2023 · 2 comments · Fixed by #1044
Labels

Comments

@DannyvdSluijs
Copy link
Contributor

DannyvdSluijs commented Jul 17, 2023

  • PHPMD version: 2.13.0
  • PHP Version: 8.1.19
  • Installation type: composer
  • Operating System / Distribution & Version: macOS Ventura 13.4.1

Current Behavior

Using the following code snippet

<?php

declare(strict_types=1);

namespace Test\Bas\TradeWebsite;

class Something
{
    /**
     * @SuppressWarnings(PHPMD.UndefinedVariable)
     * @SuppressWarnings(PHPMD.UnusedLocalVariable)
     * @SuppressWarnings(PHPMD.CountInLoopExpression)
     */
    public function test(): void
    {
        while(count([])) {
            break;
        }

        $y;
    }
}

when running ./vendor/bin/phpmd tests/Something.php ansi phpmd.xml I get Found 0 violations and 0 errors in 101ms
when I add the --strict option (./vendor/bin/phpmd tests/Something.php ansi phpmd.xml --strict) the output shows:

FILE: ***********/Something.php
------------------------------------------------------------------
 20 | VIOLATION | Avoid unused local variables such as '$y'.
 20 | VIOLATION | Avoid using undefined variables such as '$y' which will lead to PHP notices.

Expected Behavior

I would expect the CountInLoopExpression violation to be found as well when running in strict mode.

FILE: ***********/Something.php
------------------------------------------------------------------
 11 | VIOLATION | Avoid using Test\Bas\TradeWebsite\count() function in while loops.
 15 | VIOLATION | Avoid unused local variables such as '$y'.
 15 | VIOLATION | Avoid using undefined variables such as '$y' which will lead to PHP notices.

I can see in the XML output the priority of CountInLoopExpression is set to 2 and the others are priority 3. This could explain why it doesn't show in strict mode. However as a user of PHMD on the CLI and visitor of the website this behaviour isn't documented. So in my eyes the expected behaviour could also be as is but some clarification in the tool or website is missing.

Steps To Reproduce:

Take the above code snippet and run it with ./vendor/bin/phpmd tests/Something.php ansi phpmd.xml --strict

Checks before submitting

  • Be sure that there isn't already an issue about this. See: Issues list
  • Be sure that there isn't already a pull request about this. See: Pull requests
  • I have added every step to reproduce the bug.
  • If possible I added relevant code examples.
  • This issue is about 1 bug and nothing more.
  • The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".
@tvbeek tvbeek added the Bug label Jul 27, 2023
@tvbeek
Copy link
Member

tvbeek commented Jul 27, 2023

Thanks for reporting, I have already debugged a part and can confirm that it doesn't do what it need to do. I just need to find out why 😆

@tvbeek
Copy link
Member

tvbeek commented Sep 15, 2023

I suspect this impact more rules.

On the AbstractRule class we have:

    protected function applyOnClassMethods(AbstractTypeNode $node)
    {
        foreach ($node->getMethods() as $method) {
            if ($method->hasSuppressWarningsAnnotationFor($this)) {
                continue;
            }

            $this->apply($method);
        }
    }

That is used in the apply method from the CountInLoopExpression. And added for #677

I'm not yet sure why this rule has another method to add the violations. But it is a next step in fixing this issue 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants