Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PYI011 and add auto-fix #3492

Merged
merged 2 commits into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI011.pyi
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
import math
import os
import sys
from math import inf

import numpy as np

def f12(
x,
y: str = os.pathsep, # Error PYI011 Only simple default values allowed for typed arguments
Expand Down Expand Up @@ -101,3 +108,9 @@ def f35(
x: complex = math.inf # Error PYI011 Only simple default values allowed for typed arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment will be removed.
I am working on another PR to add a method to aggregate comments from Expression to keep them.

+ 1j,
) -> None: ...
def f36(
*, x: str = sys.version, # OK
) -> None: ...
def f37(
*, x: str = "" + "", # Error PYI011 Only simple default values allowed for typed arguments
) -> None: ...
43 changes: 32 additions & 11 deletions crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,25 @@
use rustpython_parser::ast::{Arguments, Constant, Expr, ExprKind, Operator, Unaryop};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::types::Range;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

#[violation]
pub struct TypedArgumentSimpleDefaults;

/// PYI011
impl Violation for TypedArgumentSimpleDefaults {
impl AlwaysAutofixableViolation for TypedArgumentSimpleDefaults {
#[derive_message_formats]
fn message(&self) -> String {
format!("Only simple default values allowed for typed arguments")
}

fn autofix_title(&self) -> String {
"Replace default value by `...`".to_string()
}
}

#[violation]
Expand Down Expand Up @@ -111,7 +116,7 @@ fn is_valid_default_value_with_annotation(default: &Expr, checker: &Checker) ->
if let ExprKind::Attribute { .. } = &operand.node {
if checker
.ctx
.resolve_call_path(default)
.resolve_call_path(operand)
.map_or(false, |call_path| {
ALLOWED_MATH_ATTRIBUTES_IN_DEFAULTS.iter().any(|target| {
// reject `-math.nan`
Expand Down Expand Up @@ -188,10 +193,18 @@ pub fn typed_argument_simple_defaults(checker: &mut Checker, args: &Arguments) {
{
if arg.node.annotation.is_some() {
if !is_valid_default_value_with_annotation(default, checker) {
checker.diagnostics.push(Diagnostic::new(
TypedArgumentSimpleDefaults,
Range::from(default),
));
let mut diagnostic =
Diagnostic::new(TypedArgumentSimpleDefaults, Range::from(default));

if checker.patch(diagnostic.kind.rule()) {
diagnostic.amend(Fix::replacement(
"...".to_string(),
default.location,
default.end_location.unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a SAFETY: <reason> comment here and on line 234 explaining why it's safe to call unwrap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done in a lot of places in the code.
I do not think it is needed.

));
}

checker.diagnostics.push(diagnostic);
}
}
}
Expand All @@ -207,10 +220,18 @@ pub fn typed_argument_simple_defaults(checker: &mut Checker, args: &Arguments) {
{
if kwarg.node.annotation.is_some() {
if !is_valid_default_value_with_annotation(default, checker) {
checker.diagnostics.push(Diagnostic::new(
TypedArgumentSimpleDefaults,
Range::from(default),
));
let mut diagnostic =
Diagnostic::new(TypedArgumentSimpleDefaults, Range::from(default));

if checker.patch(diagnostic.kind.rule()) {
diagnostic.amend(Fix::replacement(
"...".to_string(),
default.location,
default.end_location.unwrap(),
));
}

checker.diagnostics.push(diagnostic);
}
}
}
Expand Down