[flake8-pyi] Implement PYI063
#11699
Conversation
| let semantic = checker.semantic(); | ||
| // TODO: this scope is wrong. | ||
| let scope = semantic.current_scope(); | ||
| let function_type = function_type::classify( |
There was a problem hiding this comment.
This call currently doesn't correctly identify methods, which is why the issue is not correctly raised on line 26 of PYI063.pyi. I suspect it is because scope is not correct.
I didn't look super hard into it, but I don't see a very straightforward way to get the correct scope from a FunctionStmt node.
There was a problem hiding this comment.
I fixed this by moving it out of the definition phase and into the standard statement checker.
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PYI063 | 23 | 23 | 0 | 0 | 0 |
charliermarsh
added
rule
| if checker.enabled(Rule::OldStylePositionalOnlyArg) { | ||
| flake8_pyi::rules::old_style_positional_only_arg(checker, definition); | ||
| } |
There was a problem hiding this comment.
We should make sure only to run this rule if the target version is set to Python 3.8 or greater (Ruff still supports Python 3.7), or we'll be recommending invalid syntax for Python 3.7 users
There was a problem hiding this comment.
Any reason not to do this for .py files as well? I think type checkers understand the pre-PEP-570 convention for runtime source code as well as stubs
There was a problem hiding this comment.
For regular python files, code like:
def add(__a, __b):
return __a + __bIt's not necessary that the user intended to make them positional only. Whereas in stub files it is intended. Raising it on python files can cause false positives.
There was a problem hiding this comment.
Although you certainly can do calls such as add(__a=1, __b=2), in my opinion, I think the vast majority of Python functions with parameters like that have probably been written to take advantage of the type-checker convention to understand those parameters as being positional-only. But let's see what Charlie thinks!
There was a problem hiding this comment.
I ended up enabling this based on Alex's suggestion.
| def okay(__x__: int) -> None: ... | ||
| # The first argument isn't positional-only, so logically the second can't be either: | ||
| def also_okay(x: int, __y: str) -> None: ... | ||
| def fine(x: bytes, /) -> None: ... |
There was a problem hiding this comment.
Can I suggest the test case
def still_fine(_x: int) -> None: ...This should be fine right?
There was a problem hiding this comment.
seems good, I'll add it.
| if is_old_style_positional_only_arg(second_arg) { | ||
| checker.diagnostics.push(Diagnostic::new( | ||
| OldStylePositionalOnlyArg, | ||
| second_arg.range(), |
There was a problem hiding this comment.
Why is the check limited to the first two arguments?
Summary
Implements
Y063fromflake8-pyi.Test Plan
cargo test/cargo insta review