Skip to content

Commit

Permalink
[flake8-pyi] Implement PYI063 (#11699)
Browse files Browse the repository at this point in the history
## Summary
Implements `Y063` from `flake8-pyi`.

## Test Plan
`cargo test` / `cargo insta review`
  • Loading branch information
tusharsadhwani authored Jun 4, 2024
1 parent 2f8ac1e commit e1133a2
Show file tree
Hide file tree
Showing 11 changed files with 463 additions and 14 deletions.
64 changes: 64 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI063.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# See https://peps.python.org/pep-0484/#positional-only-arguments
# for the full details on which arguments using the older syntax should/shouldn't
# be considered positional-only arguments by type checkers.
from typing import Self

def bad(__x: int) -> None: ... # PYI063
def also_bad(__x: int, __y: str) -> None: ... # PYI063
def still_bad(__x_: int) -> None: ... # PYI063

def no_args() -> None: ...
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: ...
def no_idea_why_youd_do_this(__x: int, /, __y: str) -> None: ...
def cool(_x__: int) -> None: ...
def also_cool(x__: int) -> None: ...
def unclear_from_pep_484_if_this_is_positional_or_not(__: str) -> None: ...
def _(_: int) -> None: ...

class Foo:
def bad(__self) -> None: ... # PYI063
@staticmethod
def bad2(__self) -> None: ... # PYI063
def bad3(__self, __x: int) -> None: ... # PYI063
def still_bad(self, __x_: int) -> None: ... # PYI063
@staticmethod
def this_is_bad_too(__x: int) -> None: ... # PYI063
@classmethod
def not_good(cls, __foo: int) -> None: ... # PYI063

# The first non-self argument isn't positional-only, so logically the second can't be either:
def okay1(self, x: int, __y: int) -> None: ...
# Same here:
@staticmethod
def okay2(x: int, __y_: int) -> None: ...
@staticmethod
def no_args() -> int: ...
def okay3(__self__, __x__: int, __y: str) -> None: ...
def okay4(self, /) -> None: ...
def okay5(self, x: int, /) -> None: ...
def okay6(__self, /) -> None: ...
def cool(_self__: int) -> None: ...
def also_cool(self__: int) -> None: ...
def unclear_from_pep_484_if_this_is_positional_or_not(__: str) -> None: ...
def _(_: int) -> None: ...
@classmethod
def fine(cls, foo: int, /) -> None: ...

class Metaclass(type):
@classmethod
def __new__(mcls, __name: str, __bases: tuple[type, ...], __namespace: dict, **kwds) -> Self: ... # PYI063

class Metaclass2(type):
@classmethod
def __new__(metacls, __name: str, __bases: tuple[type, ...], __namespace: dict, **kwds) -> Self: ... # PYI063

class GoodMetaclass(type):
@classmethod
def __new__(mcls, name: str, bases: tuple[type, ...], namespace: dict, /, **kwds) -> Self: ...

class GoodMetaclass2(type):
@classmethod
def __new__(metacls, name: str, bases: tuple[type, ...], namespace: dict, /, **kwds) -> Self: ...
64 changes: 64 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI063.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# See https://peps.python.org/pep-0484/#positional-only-arguments
# for the full details on which arguments using the older syntax should/shouldn't
# be considered positional-only arguments by type checkers.
from typing import Self

def bad(__x: int) -> None: ... # PYI063
def also_bad(__x: int, __y: str) -> None: ... # PYI063
def still_bad(__x_: int) -> None: ... # PYI063

def no_args() -> None: ...
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: ...
def no_idea_why_youd_do_this(__x: int, /, __y: str) -> None: ...
def cool(_x__: int) -> None: ...
def also_cool(x__: int) -> None: ...
def unclear_from_pep_484_if_this_is_positional_or_not(__: str) -> None: ...
def _(_: int) -> None: ...

class Foo:
def bad(__self) -> None: ... # PYI063
@staticmethod
def bad2(__self) -> None: ... # PYI063
def bad3(__self, __x: int) -> None: ... # PYI063
def still_bad(self, __x_: int) -> None: ... # PYI063 # TODO: doesn't get raised here
@staticmethod
def this_is_bad_too(__x: int) -> None: ... # PYI063
@classmethod
def not_good(cls, __foo: int) -> None: ... # PYI063

# The first non-self argument isn't positional-only, so logically the second can't be either:
def okay1(self, x: int, __y: int) -> None: ...
# Same here:
@staticmethod
def okay2(x: int, __y_: int) -> None: ...
@staticmethod
def no_args() -> int: ...
def okay3(__self__, __x__: int, __y: str) -> None: ...
def okay4(self, /) -> None: ...
def okay5(self, x: int, /) -> None: ...
def okay6(__self, /) -> None: ...
def cool(_self__: int) -> None: ...
def also_cool(self__: int) -> None: ...
def unclear_from_pep_484_if_this_is_positional_or_not(__: str) -> None: ...
def _(_: int) -> None: ...
@classmethod
def fine(cls, foo: int, /) -> None: ...

class Metaclass(type):
@classmethod
def __new__(mcls, __name: str, __bases: tuple[type, ...], __namespace: dict, **kwds) -> Self: ... # PYI063

class Metaclass2(type):
@classmethod
def __new__(metacls, __name: str, __bases: tuple[type, ...], __namespace: dict, **kwds) -> Self: ... # PYI063

class GoodMetaclass(type):
@classmethod
def __new__(mcls, name: str, bases: tuple[type, ...], namespace: dict, /, **kwds) -> Self: ...

class GoodMetaclass2(type):
@classmethod
def __new__(metacls, name: str, bases: tuple[type, ...], namespace: dict, /, **kwds) -> Self: ...
22 changes: 8 additions & 14 deletions crates/ruff_linter/src/checkers/ast/analyze/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,26 +146,20 @@ pub(crate) fn definitions(checker: &mut Checker) {

// flake8-pyi
if enforce_stubs {
if checker.enabled(Rule::DocstringInStub) {
flake8_pyi::rules::docstring_in_stubs(checker, docstring);
}
flake8_pyi::rules::docstring_in_stubs(checker, docstring);
}
if enforce_stubs_and_runtime {
if checker.enabled(Rule::IterMethodReturnIterable) {
flake8_pyi::rules::iter_method_return_iterable(checker, definition);
}
flake8_pyi::rules::iter_method_return_iterable(checker, definition);
}

// pylint
if enforce_dunder_method {
if checker.enabled(Rule::BadDunderMethodName) {
if let Definition::Member(Member {
kind: MemberKind::Method(method),
..
}) = definition
{
pylint::rules::bad_dunder_method_name(checker, method);
}
if let Definition::Member(Member {
kind: MemberKind::Method(method),
..
}) = definition
{
pylint::rules::bad_dunder_method_name(checker, method);
}
}

Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::RedundantNumericUnion) {
flake8_pyi::rules::redundant_numeric_union(checker, parameters);
}
if checker.enabled(Rule::PrePep570PositionalArgument) {
flake8_pyi::rules::pre_pep570_positional_argument(checker, function_def);
}
if checker.enabled(Rule::DunderFunctionName) {
if let Some(diagnostic) = pep8_naming::rules::dunder_function_name(
checker.semantic.current_scope(),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "057") => (RuleGroup::Preview, rules::flake8_pyi::rules::ByteStringUsage),
(Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass),
(Flake8Pyi, "062") => (RuleGroup::Preview, rules::flake8_pyi::rules::DuplicateLiteralMember),
(Flake8Pyi, "063") => (RuleGroup::Preview, rules::flake8_pyi::rules::PrePep570PositionalArgument),
(Flake8Pyi, "064") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantFinalLiteral),
(Flake8Pyi, "066") => (RuleGroup::Preview, rules::flake8_pyi::rules::BadVersionInfoOrder),

Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ mod tests {
#[test_case(Rule::PatchVersionComparison, Path::new("PYI004.pyi"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.py"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.pyi"))]
#[test_case(Rule::PrePep570PositionalArgument, Path::new("PYI063.py"))]
#[test_case(Rule::PrePep570PositionalArgument, Path::new("PYI063.pyi"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.py"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.pyi"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub(crate) use non_self_return_type::*;
pub(crate) use numeric_literal_too_long::*;
pub(crate) use pass_in_class_body::*;
pub(crate) use pass_statement_stub_body::*;
pub(crate) use pre_pep570_positional_argument::*;
pub(crate) use prefix_type_params::*;
pub(crate) use quoted_annotation_in_stub::*;
pub(crate) use redundant_final_literal::*;
Expand Down Expand Up @@ -62,6 +63,7 @@ mod non_self_return_type;
mod numeric_literal_too_long;
mod pass_in_class_body;
mod pass_statement_stub_body;
mod pre_pep570_positional_argument;
mod prefix_type_params;
mod quoted_annotation_in_stub;
mod redundant_final_literal;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::identifier::Identifier;
use ruff_python_ast::{self as ast, ParameterWithDefault};
use ruff_python_semantic::analyze::function_type;

use crate::checkers::ast::Checker;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for the presence of [PEP 484]-style positional-only arguments.
///
/// ## Why is this bad?
/// Historically, [PEP 484] recommended prefixing positional-only arguments
/// with a double underscore (`__`). However, [PEP 570] introduced a dedicated
/// syntax for positional-only arguments, which should be preferred.
///
/// ## Example
///
/// ```python
/// def foo(__x: int) -> None:
/// ...
/// ```
///
/// Use instead:
///
/// ```python
/// def foo(x: int, /) -> None:
/// ...
/// ```
///
/// [PEP 484]: https://peps.python.org/pep-0484/#positional-only-arguments
/// [PEP 570]: https://peps.python.org/pep-0570
#[violation]
pub struct PrePep570PositionalArgument;

impl Violation for PrePep570PositionalArgument {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use PEP 570 syntax for positional-only arguments")
}

fn fix_title(&self) -> Option<String> {
Some("Add `/` to function signature".to_string())
}
}

/// PYI063
pub(crate) fn pre_pep570_positional_argument(
checker: &mut Checker,
function_def: &ast::StmtFunctionDef,
) {
// PEP 570 was introduced in Python 3.8.
if checker.settings.target_version < PythonVersion::Py38 {
return;
}

if !function_def.parameters.posonlyargs.is_empty() {
return;
}

if function_def.parameters.args.is_empty() {
return;
}

let semantic = checker.semantic();
let scope = semantic.current_scope();
let function_type = function_type::classify(
&function_def.name,
&function_def.decorator_list,
scope,
semantic,
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
);

// If the method has a `self` or `cls` argument, skip it.
let skip = usize::from(matches!(
function_type,
function_type::FunctionType::Method | function_type::FunctionType::ClassMethod
));

if let Some(arg) = function_def.parameters.args.get(skip) {
if is_pre_pep570_positional_only(arg) {
checker.diagnostics.push(Diagnostic::new(
PrePep570PositionalArgument,
arg.identifier(),
));
}
}
}

/// Returns `true` if the [`ParameterWithDefault`] is an old-style positional-only argument (i.e.,
/// its name starts with `__` and does not end with `__`).
fn is_pre_pep570_positional_only(arg: &ParameterWithDefault) -> bool {
let arg_name = &arg.parameter.name;
arg_name.starts_with("__") && !arg_name.ends_with("__")
}
Loading

0 comments on commit e1133a2

Please sign in to comment.