Skip to content

Conversation

VascoSch92
Copy link
Contributor

@VascoSch92 VascoSch92 commented May 15, 2025

The PR add the fix safety section for rule SIM110 (#15584 )

Unsafe Fix Example

def predicate(item):
    global called
    called += 1
    if called == 1:
    # after first call we change the method
        def new_predicate(_): return False
        globals()['predicate'] = new_predicate
    return True

def foo():
    for item in range(10):
        if predicate(item):
            return True
    return False

def foo_gen():
    return any(predicate(item) for item in range(10))

called = 0
print(foo())      # true – returns immediately on first call

called = 0
print(foo_gen())  # false – second call uses new `predicate`

Note

I notice that here we have two rules, SIM110 & SIM111. The second one seems not anymore active. Should I delete SIM111?

Copy link
Contributor

github-actions bot commented May 15, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@VascoSch92 VascoSch92 mentioned this pull request May 15, 2025
71 tasks
@ntBre ntBre added the documentation Improvements or additions to documentation label May 15, 2025
@ntBre
Copy link
Contributor

ntBre commented May 15, 2025

That's quite an exotic example! I'm not sure if it's doing exactly what you think, though. I tried switching the order in which I call foo and foo_gen, and the output was still True and then False:

Original order

>>> def predicate(item):
...     global called
...     called += 1
...     if called == 1:
...     # after first call we change the method
...         def new_predicate(_): return False
...         globals()['predicate'] = new_predicate
...     return True
...
... def foo():
...     for item in range(10):
...         if predicate(item):
...             return True
...     return False
...
... def foo_gen():
...     return any(predicate(item) for item in range(10))
...
... called = 0
... print(foo())      # true – returns immediately on first call
...
... called = 0
... print(foo_gen())  # false – second call uses new `predicate`
...
True
False

Swapped order

>>> def predicate(item):
...     global called
...     called += 1
...     if called == 1:
...     # after first call we change the method
...         def new_predicate(_): return False
...         globals()['predicate'] = new_predicate
...     return True
...
... def foo():
...     for item in range(10):
...         if predicate(item):
...             return True
...     return False
...
... def foo_gen():
...     return any(predicate(item) for item in range(10))
...
... called = 0
... print(foo_gen())      # true – returns immediately on first call
...
... called = 0
... print(foo())  # false – second call uses new `predicate`
...
True
False

I think both versions use the same predicate for the whole loop, but called isn't actually getting reset between calls.

So removing comments might be the main reason for unsafety here.

@VascoSch92
Copy link
Contributor Author

@ntBre You are right. :-)

I just added the fact it could remove comment.

Other question: we could make the fix safe when it doesn't remove comments right?

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Other question: we could make the fix safe when it doesn't remove comments right?

Yes, I think that's right since we couldn't come up with any other reason for the unsafety.

@ntBre ntBre merged commit b302d89 into astral-sh:main May 19, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants