[flake8-simplify] add fix safety section (SIM103)#18086
[flake8-simplify] add fix safety section (SIM103)#18086ntBre merged 3 commits intoastral-sh:mainfrom
flake8-simplify] add fix safety section (SIM103)#18086Conversation
|
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! Great catch again on the __eq__ example too. Just one comment about comments and an idea for incorporating the findings from your example.
| /// This fix is marked as unsafe because it may change the program’s behavior if the condition does not | ||
| /// return a proper Boolean. Additionally, the fix could remove comments. |
There was a problem hiding this comment.
I don't think the part about comments is true. The code checks for comments in the fix range and avoids a fix in that case (although that happens way earlier in the code than the fix generation, so it's not that clear).
I wanted to say that the boolean condition part wasn't true either because we do try to add bool calls in cases where it doesn't look like a boolean, but your clever example does change the behavior. Do you think it's worth mentioning __eq__ or the other comparison methods that could cause problems? Maybe a second sentence like:
While the fix will try to wrap non-boolean values in a call to
bool, custom implementations of comparison functions like__eq__can avoid theboolcall and still lead to altered behavior.
There was a problem hiding this comment.
I don't think the part about comments is true. The code checks for comments in the fix range and avoids a fix in that case (although that happens way earlier in the code than the fix generation, so it's not that clear).
You are right. I delete this part.
I wanted to say that the boolean condition part wasn't true either because we do try to add bool calls in cases where it doesn't look like a boolean, but your clever example does change the behavior. Do you think it's worth mentioning eq or the other comparison methods that could cause problems? Maybe a second sentence like:
Fine for me. I update the fix safety section.
…rals * origin/main: [ty] Add type-expression syntax link to invalid-type-expression (#18104) [`flake8-simplify`] add fix safety section (`SIM103`) (#18086) [ty] mypy_primer: fix static-frame setup (#18103) [`flake8-simplify`] Correct behavior for `str.split`/`rsplit` with `maxsplit=0` (`SIM905`) (#18075) [ty] Fix more generics-related TODOs (#18062)
The PR add the `fix safety` section for rule `SIM210` (#15584 ) It is a little cheating, as the Fix safety section is copy/pasted by #18086 as the problem is the same. ### Unsafe Fix Example ```python class Foo(): def __eq__(self, other): return 0 def foo(): return True if Foo() == 0 else False def foo_fix(): return Foo() == 0 print(foo()) # False print(foo_fix()) # 0 ```
The PR add the `fix safety` section for rule `SIM103` (astral-sh#15584 ) ### Unsafe Fix Example ```python class Foo: def __eq__(self, other): return 1 def foo(): if Foo() == 1: return True return False def foo_fix(): return Foo() == 1 print(foo()) # True print(foo_fix()) # 1 ``` ### Note I updated the code snippet example, because I thought it was cool to have a correct example, i.e., that I can paste inside the playground and it works :-)
The PR add the `fix safety` section for rule `SIM210` (astral-sh#15584 ) It is a little cheating, as the Fix safety section is copy/pasted by astral-sh#18086 as the problem is the same. ### Unsafe Fix Example ```python class Foo(): def __eq__(self, other): return 0 def foo(): return True if Foo() == 0 else False def foo_fix(): return Foo() == 0 print(foo()) # False print(foo_fix()) # 0 ```
2 participants
The PR add the
fix safetysection for ruleSIM103(#15584 )Unsafe Fix Example
Note
I updated the code snippet example, because I thought it was cool to have a correct example, i.e., that I can paste inside the playground and it works :-)