driftingly / rector-laravel

Rector upgrades rules for Laravel
http://getrector.org
MIT License
540 stars 52 forks source link

Add AvoidNegatedCollectionFilterOrRejectRector #234

Closed spawnia closed 2 months ago

spawnia commented 3 months ago

The test currently fails and I am not sure how to make it work.

 (new Collection([0, 1, null, -1]))
     ->reject(fn (?int $number): bool => is_null($number))
-    ->filter(fn (int $number): bool => $number > 0);
+    ->reject(fn (int $number): bool => ! ($number > 0));

I think there can be additional logic to simplify conditionals that use negated comparisons:

- ->filter(fn (int $n): bool => $n !== 0)
+ ->reject(fn (int $n): bool => $n === 0)

I also wonder where this rule would be included, perhaps in the code quality set list?

What do you think about this idea in general? Would you be up for inclusion?

GeniJaho commented 3 months ago

@spawnia thanks for your PR. I think it would make a great addition to the Code Quality set. At first, it can be simple with just the negated expressions, not the negated comparisons. At least, until we have some feedback from users. I will also try this on a few real-life projects and check for any edge cases.

We also need to support normal functions, not just arrow functions, so I would add a test for them as well. You can use dump_node() and print_node() functions to help you working with different node types. Also this can be helpful https://getrector.com/ast.

The test is failing because Rector does not know what the Collection->filter() method returns. We need to supply a stub in stubs/Illuminate/Support/Collection.php, check how the other stubs are implemented, you can copy the filter() and reject() methods' signatures from the real class in the Laravel framework. After you implement a stub, you need composer dump-autoload locally and the tests should be passing. If it's unclear :sweat_smile: I can fix that part in your PR.

spawnia commented 2 months ago

We also need to support normal functions, not just arrow functions

I thought about this, but found that it can become really tricky. For example, what are we supposed to do if there are multiple return statements? I would leave it for now.