Repository navigation
[flake8_pyi] Fix PYI041's fix causing TypeError with None | None | ... - #18637
Conversation
|
10a6bad to
be1fde1
Compare
| 23 23 | | ||
| 24 |-def f1(arg1: float, *, arg2: float | list[str] | type[bool] | complex) -> None: ... # PYI041 | ||
| 24 |+def f1(arg1: float, *, arg2: list[str] | type[bool] | complex) -> None: ... # PYI041 | ||
| 24 |+def f1(arg1: float, *, arg2: complex | list[str] | type[bool]) -> None: ... # PYI041 |
There was a problem hiding this comment.
this is a clever way of fixing the issue, but I'm not totally sure that users will like it if the fix ends up reordering unions like this, even when the unions don't contain None. I think that would be pretty unexpected if I were a Ruff user.
I'm leaning towards just not providing a fix for the rule if there are multiple Nones in the union. It feels easy to understand, and well-written code should never have multiple Nones in a union anyway -- we even have other lints that would already detect that!
There was a problem hiding this comment.
Sure. Your project, your direction. I personally wouldn't be surprised. I've seen ruff fixes go beyond such changes. Further, I'd think of this more as merging elements in the union than reordering them. The PR will put the super-type in the place of the first type it's subsuming.
Anyway, I'll change the PR to just omit the fix. However, I suggest to only suppress the fix if not in a typing only context (as this is only a problem at runtime).
There was a problem hiding this comment.
While working on this fix, I've noticed that this checker is run before deferred string type annotations are visited and resolved. This means that the lint won't trigger on those. Not sure you'd want that to be changed or whether that's a deliberate design decision. Happy to pour this into an issue to tackle it later if you're interested.
There was a problem hiding this comment.
Yeah I think it makes sense to follow up on this, thanks! While testing this in the playground, I noticed that PYI016 does apply to string annotations, so you may be able to copy however it handles this.
ntBre
left a comment
There was a problem hiding this comment.
Thanks, I think this looks good overall. I just had a couple of questions to make sure I'm understanding what's going on.
There was a problem hiding this comment.
Yeah I think it makes sense to follow up on this, thanks! While testing this in the playground, I noticed that PYI016 does apply to string annotations, so you may be able to copy however it handles this.
ntBre
left a comment
There was a problem hiding this comment.
Questions resolved! I was just confused, as expected.
## Summary While working on #18637, I [noticed](#18637 (comment)) that PYI041 does not properly resolve string annotations. This PR fixes that. ## Test Plan A whole new file with tests has been added. --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
## Summary While working on astral-sh/ruff#18637, I [noticed](astral-sh/ruff#18637 (comment)) that PYI041 does not properly resolve string annotations. This PR fixes that. ## Test Plan A whole new file with tests has been added. --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
Summary
Fix
PYI041's fix turningNone | int | None | floatintoNone | None | float, which raises aTypeErrorwhen executed.The fix consists of making sure that the merged super-type is inserted where the first type that is merged was before.
Test Plan
Tests have been expanded with examples from the issue.
Related Issue
Fixes #18298