Skip to content

Add Checker::import_from_typing #17340

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.

Sign up for GitHub

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

Merged
ntBre merged 5 commits into from
Apr 11, 2025
Merged

Add Checker::import_from_typing #17340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
795553c
Add `Checker::import_from_typing`
ntBre Apr 10, 2025
684ae0d
convert AutoPythonType::into_expression to use import_from_typing
ntBre Apr 10, 2025
f4c0317
replace other typing imports
ntBre Apr 10, 2025
1a453ea
add PythonVersion::lowest and use it instead of hard-coding 3.8
ntBre Apr 10, 2025
3a8aadc
update python version parameter name and extend docs
ntBre Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions crates/ruff_linter/src/checkers/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use ruff_python_parser::semantic_errors::{
};
use rustc_hash::{FxHashMap, FxHashSet};

use ruff_diagnostics::{Diagnostic, IsolationLevel};
use ruff_diagnostics::{Diagnostic, Edit, IsolationLevel};
use ruff_notebook::{CellOffsets, NotebookIndex};
use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path};
use ruff_python_ast::identifier::Identifier;
Expand Down Expand Up @@ -62,7 +62,7 @@ use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::checkers::ast::annotation::AnnotationContext;
use crate::docstrings::extraction::ExtractionTarget;
use crate::importer::Importer;
use crate::importer::{ImportRequest, Importer, ResolutionError};
use crate::noqa::NoqaMapping;
use crate::package::PackageRoot;
use crate::registry::Rule;
Expand Down Expand Up @@ -530,6 +530,28 @@ impl<'a> Checker<'a> {
f(&mut checker, self);
self.semantic_checker = checker;
}

/// Attempt to create an [`Edit`] that imports `member`.
///
/// On Python <`version_added_to_typing`, `member` is imported from `typing_extensions`, while
/// on Python >=`version_added_to_typing`, it is imported from `typing`.
///
/// See [`Importer::get_or_import_symbol`] for more details on the returned values.
pub(crate) fn import_from_typing(
Copy link
Member

@dhruvmanila dhruvmanila Apr 10, 2025

Choose a reason for hiding this comment

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

Should this be a method on the Importer instead? This is, I think, the main interface to generate import edits:

ruff/crates/ruff_linter/src/checkers/ast/mod.rs

Lines 434 to 437 in 172af7b

/// The [`Importer`] for the current file, which enables importing of other modules.
pub(crate) const fn importer(&self) -> &Importer<'a> {
&self.importer
}

Copy link
Contributor Author

@ntBre ntBre Apr 10, 2025

Choose a reason for hiding this comment

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

It could be, but we get the Python version from Checker and also the SemanticModel for Importer::get_or_import_symbol, so we'd have to pass a couple more arguments if it were an Importer method.

I also think the Checker will be more likely to contain any additional information we need to fix #9761, such as the LinterSettings, if we add a configuration option.

Copy link
Member

@dhruvmanila dhruvmanila Apr 10, 2025

Choose a reason for hiding this comment

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

The Checker creates the Importer so we can just update the Importer to also contain a reference to the SemanticModel but I trust your judgement about the issue that needs to be fixed so we can leave it as is for now and do a follow-up change if required.

Copy link
Contributor Author

@ntBre ntBre Apr 10, 2025

Choose a reason for hiding this comment

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

Oh interesting, I see. I think I'll lean toward leaving it like this for now, but I'll keep this in mind if we don't end up using any more code from the Checker. It would logically make more sense on the Importer, as you said.

&self,
member: &str,
position: TextSize,
version_added_to_typing: PythonVersion,
) -> Result<(Edit, String), ResolutionError> {
Copy link
Member

@AlexWaygood AlexWaygood Apr 11, 2025

Choose a reason for hiding this comment

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

not really something to be addressed in this PR, but FWIW I don't love the return type of get_or_import_symbol. It seems too easy to misuse it by throwing away the second element of the tuple when you get an Ok(Edit, String) returned. E.g. #15853

It also seems like it's too easy to call .ok() on the Result returned, when what you're meant to do is to use it in combination with try_set_fix() so that the information from the error is logged to the terminal when --verbose is specified on the CLI

Copy link
Contributor Author

@ntBre ntBre Apr 11, 2025

Choose a reason for hiding this comment

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

Agreed, I thought that return type looked a bit suspicious (especially when I considered adding a third element to the tuple 😆)

I'll keep an eye on this and see if I can come up with anything.

let source_module = if self.target_version() >= version_added_to_typing {
"typing"
} else {
"typing_extensions"
};
let request = ImportRequest::import_from(source_module, member);
self.importer()
.get_or_import_symbol(&request, position, self.semantic())
}
}

impl SemanticSyntaxContext for Checker<'_> {
Expand Down
12 changes: 3 additions & 9 deletions crates/ruff_linter/src/rules/fastapi/rules/fastapi_non_annotated_dependency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use ruff_python_semantic::Modules;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;
use crate::rules::fastapi::rules::is_fastapi_route;
use ruff_python_ast::PythonVersion;

Expand Down Expand Up @@ -232,15 +231,10 @@ fn create_diagnostic(
);

let try_generate_fix = || {
let module = if checker.target_version() >= PythonVersion::PY39 {
"typing"
} else {
"typing_extensions"
};
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import_from(module, "Annotated"),
let (import_edit, binding) = checker.import_from_typing(
"Annotated",
parameter.range.start(),
checker.semantic(),
PythonVersion::PY39,
)?;

// Each of these classes takes a single, optional default
Expand Down
43 changes: 14 additions & 29 deletions crates/ruff_linter/src/rules/flake8_annotations/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use ruff_python_semantic::analyze::visibility;
use ruff_python_semantic::{Definition, SemanticModel};
use ruff_text_size::{TextRange, TextSize};

use crate::importer::{ImportRequest, Importer};
use crate::checkers::ast::Checker;
use ruff_python_ast::PythonVersion;

/// Return the name of the function, if it's overloaded.
Expand Down Expand Up @@ -119,26 +119,19 @@ impl AutoPythonType {
/// additional edits.
pub(crate) fn into_expression(
self,
importer: &Importer,
checker: &Checker,
at: TextSize,
semantic: &SemanticModel,
target_version: PythonVersion,
) -> Option<(Expr, Vec<Edit>)> {
let target_version = checker.target_version();
match self {
AutoPythonType::Never => {
let (no_return_edit, binding) = importer
.get_or_import_symbol(
&ImportRequest::import_from(
"typing",
if target_version >= PythonVersion::PY311 {
"Never"
} else {
"NoReturn"
},
),
at,
semantic,
)
let member = if target_version >= PythonVersion::PY311 {
"Never"
} else {
"NoReturn"
};
let (no_return_edit, binding) = checker
.import_from_typing(member, at, PythonVersion::lowest())
.ok()?;
let expr = Expr::Name(ast::ExprName {
id: Name::from(binding),
Expand Down Expand Up @@ -175,12 +168,8 @@ impl AutoPythonType {
let element = type_expr(*python_type)?;

// Ex) `Optional[int]`
let (optional_edit, binding) = importer
.get_or_import_symbol(
&ImportRequest::import_from("typing", "Optional"),
at,
semantic,
)
let (optional_edit, binding) = checker
.import_from_typing("Optional", at, PythonVersion::lowest())
.ok()?;
let expr = typing_optional(element, Name::from(binding));
Some((expr, vec![optional_edit]))
Expand All @@ -192,12 +181,8 @@ impl AutoPythonType {
.collect::<Option<Vec<_>>>()?;

// Ex) `Union[int, str]`
let (union_edit, binding) = importer
.get_or_import_symbol(
&ImportRequest::import_from("typing", "Union"),
at,
semantic,
)
let (union_edit, binding) = checker
.import_from_typing("Union", at, PythonVersion::lowest())
.ok()?;
let expr = typing_union(&elements, Name::from(binding));
Some((expr, vec![union_edit]))
Expand Down
30 changes: 6 additions & 24 deletions crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,12 +721,7 @@ pub(crate) fn definition(
} else {
auto_return_type(function)
.and_then(|return_type| {
return_type.into_expression(
checker.importer(),
function.parameters.start(),
checker.semantic(),
checker.target_version(),
)
return_type.into_expression(checker, function.parameters.start())
})
.map(|(return_type, edits)| (checker.generator().expr(&return_type), edits))
};
Expand All @@ -752,12 +747,7 @@ pub(crate) fn definition(
} else {
auto_return_type(function)
.and_then(|return_type| {
return_type.into_expression(
checker.importer(),
function.parameters.start(),
checker.semantic(),
checker.target_version(),
)
return_type.into_expression(checker, function.parameters.start())
})
.map(|(return_type, edits)| (checker.generator().expr(&return_type), edits))
};
Expand Down Expand Up @@ -822,12 +812,8 @@ pub(crate) fn definition(
} else {
auto_return_type(function)
.and_then(|return_type| {
return_type.into_expression(
checker.importer(),
function.parameters.start(),
checker.semantic(),
checker.target_version(),
)
return_type
.into_expression(checker, function.parameters.start())
})
.map(|(return_type, edits)| {
(checker.generator().expr(&return_type), edits)
Expand Down Expand Up @@ -861,12 +847,8 @@ pub(crate) fn definition(
} else {
auto_return_type(function)
.and_then(|return_type| {
return_type.into_expression(
checker.importer(),
function.parameters.start(),
checker.semantic(),
checker.target_version(),
)
return_type
.into_expression(checker, function.parameters.start())
})
.map(|(return_type, edits)| {
(checker.generator().expr(&return_type), edits)
Expand Down
24 changes: 3 additions & 21 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_for_self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ use ruff_python_semantic::analyze::class::is_metaclass;
use ruff_python_semantic::analyze::function_type::{self, FunctionType};
use ruff_python_semantic::analyze::visibility::{is_abstract, is_overload};
use ruff_python_semantic::{Binding, ResolvedReference, ScopeId, SemanticModel};
use ruff_text_size::{Ranged, TextRange, TextSize};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::importer::{ImportRequest, ResolutionError};
use ruff_python_ast::PythonVersion;

/// ## What it does
Expand Down Expand Up @@ -317,7 +316,8 @@ fn replace_custom_typevar_with_self(
self_or_cls_annotation: &ast::Expr,
) -> anyhow::Result<Fix> {
// (1) Import `Self` (if necessary)
let (import_edit, self_symbol_binding) = import_self(checker, function_def.start())?;
let (import_edit, self_symbol_binding) =
checker.import_from_typing("Self", function_def.start(), PythonVersion::PY311)?;

// (2) Remove the first parameter's annotation
let mut other_edits = vec![Edit::deletion(
Expand Down Expand Up @@ -367,24 +367,6 @@ fn replace_custom_typevar_with_self(
))
}

/// Attempt to create an [`Edit`] that imports `Self`.
///
/// On Python <3.11, `Self` is imported from `typing_extensions`;
/// on Python >=3.11, it is imported from `typing`.
/// This is because it was added to the `typing` module on Python 3.11,
/// but is available from the backport package `typing_extensions` on all versions.
fn import_self(checker: &Checker, position: TextSize) -> Result<(Edit, String), ResolutionError> {
let source_module = if checker.target_version() >= PythonVersion::PY311 {
"typing"
} else {
"typing_extensions"
};
let request = ImportRequest::import_from(source_module, "Self");
checker
.importer()
.get_or_import_symbol(&request, position, checker.semantic())
}

/// Returns a series of [`Edit`]s that modify all references to the given `typevar`.
///
/// Only references within `editable_range` will be modified.
Expand Down
14 changes: 6 additions & 8 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/duplicate_union_member.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@ use std::collections::HashSet;

use anyhow::Result;

use ruff_python_ast::name::Name;
use rustc_hash::FxHashSet;

use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::{Expr, ExprBinOp, ExprContext, ExprName, ExprSubscript, ExprTuple, Operator};
use ruff_python_ast::name::Name;
use ruff_python_ast::{
Expr, ExprBinOp, ExprContext, ExprName, ExprSubscript, ExprTuple, Operator, PythonVersion,
};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;

/// ## What it does
/// Checks for duplicate union members.
Expand Down Expand Up @@ -181,11 +182,8 @@ fn generate_union_fix(
debug_assert!(nodes.len() >= 2, "At least two nodes required");

// Request `typing.Union`
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import_from("typing", "Union"),
annotation.start(),
checker.semantic(),
)?;
let (import_edit, binding) =
checker.import_from_typing("Union", annotation.start(), PythonVersion::lowest())?;

// Construct the expression as `Subscript[typing.Union, Tuple[expr, [expr, ...]]]`
let new_expr = Expr::Subscript(ExprSubscript {
Expand Down
14 changes: 2 additions & 12 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast as ast;
Expand Down Expand Up @@ -214,17 +213,8 @@ fn replace_with_self_fix(
) -> anyhow::Result<Fix> {
let semantic = checker.semantic();

let (self_import, self_binding) = {
let source_module = if checker.target_version() >= PythonVersion::PY311 {
"typing"
} else {
"typing_extensions"
};

let (importer, semantic) = (checker.importer(), checker.semantic());
let request = ImportRequest::import_from(source_module, "Self");
importer.get_or_import_symbol(&request, returns.start(), semantic)?
};
let (self_import, self_binding) =
checker.import_from_typing("Self", returns.start(), PythonVersion::PY311)?;

let mut others = Vec::with_capacity(2);

Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use ruff_text_size::{Ranged, TextRange};

use smallvec::SmallVec;

use crate::{checkers::ast::Checker, importer::ImportRequest};
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for redundant `Literal[None]` annotations.
Expand Down Expand Up @@ -225,10 +225,10 @@ fn create_fix(

let fix = match union_kind {
UnionKind::TypingOptional => {
let (import_edit, bound_name) = checker.importer().get_or_import_symbol(
&ImportRequest::import_from("typing", "Optional"),
let (import_edit, bound_name) = checker.import_from_typing(
"Optional",
literal_expr.start(),
checker.semantic(),
PythonVersion::lowest(),
)?;
let optional_expr = typing_optional(new_literal_expr, Name::from(bound_name));
let content = checker.generator().expr(&optional_expr);
Expand Down
11 changes: 4 additions & 7 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Vi
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{
name::Name, AnyParameterRef, Expr, ExprBinOp, ExprContext, ExprName, ExprSubscript, ExprTuple,
Operator, Parameters,
Operator, Parameters, PythonVersion,
};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::{Ranged, TextRange};

use crate::{checkers::ast::Checker, importer::ImportRequest};
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for parameter annotations that contain redundant unions between
Expand Down Expand Up @@ -268,11 +268,8 @@ fn generate_union_fix(
debug_assert!(nodes.len() >= 2, "At least two nodes required");

// Request `typing.Union`
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import_from("typing", "Union"),
annotation.start(),
checker.semantic(),
)?;
let (import_edit, binding) =
checker.import_from_typing("Optional", annotation.start(), PythonVersion::lowest())?;

// Construct the expression as `Subscript[typing.Union, Tuple[expr, [expr, ...]]]`
let new_expr = Expr::Subscript(ExprSubscript {
Expand Down
8 changes: 2 additions & 6 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use ruff_python_semantic::{analyze::class::is_enumeration, ScopeKind, SemanticMo
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;
use crate::rules::flake8_pyi::rules::TypingModule;
use crate::Locator;
use ruff_python_ast::PythonVersion;
Expand Down Expand Up @@ -682,11 +681,8 @@ pub(crate) fn type_alias_without_annotation(checker: &Checker, value: &Expr, tar
target.range(),
);
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import(module.as_str(), "TypeAlias"),
target.start(),
checker.semantic(),
)?;
let (import_edit, binding) =
checker.import_from_typing("TypeAlias", target.start(), PythonVersion::PY310)?;
Ok(Fix::safe_edits(
Edit::range_replacement(format!("{id}: {binding}"), target.range()),
[import_edit],
Expand Down
Loading
Loading