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

Incorrect behavior of BooleanInBooleanNotRuleFixerRector #8663

Closed
rtm-ctrlz opened this issue Jun 2, 2024 · 1 comment · Fixed by rectorphp/rector-src#5940
Closed

Incorrect behavior of BooleanInBooleanNotRuleFixerRector #8663

rtm-ctrlz opened this issue Jun 2, 2024 · 1 comment · Fixed by rectorphp/rector-src#5940
Labels

Comments

@rtm-ctrlz
Copy link

Bug Report

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/6c9e6aa3-5582-4c51-ad40-d9e27c5303a0

<?php

final class DemoFile
{
    public function run(bool $param): bool
    {
        if (!preg_match('/^.+$/', $param)) {
	        return true;
        }
        return false;
    }
}
@@ -4,7 +4,7 @@
 {
     public function run(bool $param): bool
     {
-        if (!preg_match('/^.+$/', $param)) {
+        if (preg_match('/^.+$/', $param) === 0 || preg_match('/^.+$/', $param) === 0 || preg_match('/^.+$/', $param) === false) {
 	        return true;
         }
         return false;

Responsible rules

  • BooleanInBooleanNotRuleFixerRector

BTW custom function creates a bit different diff

See https://getrector.com/demo/6c9e6aa3-5582-4c51-ad40-d9e27c5303a0

<?php
function test(string $a): int|false {
    return preg_match('/^.+$/', $a);
}
function run(bool $param): bool
{
    if (!test($param)) {
        return true;
    }
    return false;
}
@@ -4,8 +4,8 @@
 }
 function run(bool $param): bool
 {
-    if (!test($param)) {
+    if (test($param) === 0 || test($param) === false) {
         return true;
     }
     return false;
 }

Expected Behavior

I see couple ways:

  • adding variable (yes, naming is hard)
  • skip or add a warning (if it is possible)

I'm not sure which way is better, but changing code to calling function more than one time (for this case) may lead to bugs, because we never know if the function have any side-effects or not.

@rtm-ctrlz rtm-ctrlz added the bug label Jun 2, 2024
@samsonasik
Copy link
Member

I think we can focus to fixing duplicated === 0 comparison first, as it is complex logic :)

then if possible, make use of in_array() for Callike usage, but that may in separate PR

samsonasik referenced this issue in rectorphp/rector-src Jun 3, 2024
…ector (#5940)

* [Strict] Fix double === 0 comparison on BooleanInBooleanNotRuleFixerRector

* [Strict] Fix double === 0 comparison on BooleanInBooleanNotRuleFixerRector

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

Successfully merging a pull request may close this issue.

2 participants