sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.51k stars 65 forks source link

The `min-max-identity` does not trigger where expected due to limited type analysis #321

Closed ruancomelli closed 1 year ago

ruancomelli commented 1 year ago

Checklist

Description

The user Tortoise#3130 reported on Discord that the min-max-identity refactoring is not triggering where expected, and I can reproduce the issue:

Sourcery correctly refactors

if self.health < 0:
    self.health = 0

as

self.health = max(self.health, 0)

However,

if self.health > self.max_health:
    self.health = self.max_health

does not get refactored as:

self.health = min(self.health, self.max_health)

I investigated this issue and discovered that min-max-identity requires the compared value self.max_health to have a known, "simple" type, such as int or float. However, since our type inference engine is still not powerful enough to do class-level analyses, we cannot infer that self.max_health is an integer, and hence the refactoring is not suggested.

Simply annotating self.max_health is enough to get the refactoring to trigger:

self.max_health: int
if self.health > self.max_health: # min-max-identity gets proposed!
    self.health = self.max_health

but this is far from ideal.

The expected behavior is to get the refactoring to trigger even without this additional type annotation.

Potential solutions

There are two potential solutions for this bug:

  1. More powerful type analysis: this is already in our roadmap but will still take some time to be implemented. A powerful type analysis engine would potentially be able to infer that self.max_health has a "simple type" in the snippet above;
  2. Lift the restriction on the type: I don't see a strong reason to disable the refactoring for unknown types. Possibly there's room for lifting or at least relaxing the restriction on the operand types here that would allow this refactoring to trigger more often;

Debug Information

IDE Version: VS Code 1.74.3

Sourcery Version: Sourcery v1.0.3

Operating system and Version: Ubuntu 20.04.5 LTS

ruancomelli commented 1 year ago

A fix for this issue was just merged and will be available in the next Sourcery release :tada:

Hellebore commented 1 year ago

Released with 1.0.4