Add `Checker::import_from_typing` #17340

ntBre · 2025-04-10T18:18:56Z

Summary

This PR replaces uses of version-dependent imports from typing or typing_extensions with a centralized Checker::import_from_typing method.

The idea here is to make the fix for #9761 (whatever it ends up being) applicable to all of the rules performing similar checks.

I found these with a pretty naive grep for "typing_extensions" in the ruff_linter/src/rules directory, so if there are any rules I missed, please let me know. @AlexWaygood mentioned "lots of rules" here, so I think this is likely.

Test Plan

Existing tests for FAST002, PYI019, and PYI034.

@AlexWaygood

Summary -- This PR replaces uses of version-dependent imports from `typing` or `typing_extensions` with a centralized `Checker::import_from_typing` method. The idea here is to make the fix for #9761 (whatever it ends up being) applicable to all of the rules performing similar checks. I found these with a pretty naive grep for `"typing_extensions"` in the `ruff_linter/src/rules` directory, so if there are any rules I missed, please let me know. @AlexWaygood mentioned "lots of rules" [here](#9761 (comment)), so I think this is likely. Test Plan -- Existing tests for FAST002, PYI019, and PYI034.

github-actions · 2025-04-10T18:25:17Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

dhruvmanila · 2025-04-10T19:00:10Z

This PR replaces uses of version-dependent imports from typing or typing_extensions with a centralized Checker::import_from_typing method.

Is the goal to replace all edit generation from typing module to be using this method? I think that makes sense as it would then be easier to find all usages of it via this method.

I found these with a pretty naive grep for "typing_extensions" in the ruff_linter/src/rules directory, so if there are any rules I missed, please let me know.

You can go through the references of get_or_import_symbol which should have some of the symbols that are being imported from typing.

For example,

ruff/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs

Lines 129 to 142 in 410aa4b

    
           let (no_return_edit, binding) = importer 
        
               .get_or_import_symbol( 
        
                   &ImportRequest::import_from( 
        
                       "typing", 
        
                       if target_version >= PythonVersion::PY311 { 
        
                           "Never" 
        
                       } else { 
        
                           "NoReturn" 
        
                       }, 
        
                   ), 
        
                   at, 
        
                   semantic, 
        
               ) 
        
               .ok()?;

ntBre · 2025-04-10T19:10:20Z

Is the goal to replace all edit generation from typing module to be using this method? I think that makes sense as it would then be easier to find all usages of it via this method.

My goal was just to replace imports that might come from typing or typing_extensions, but that's a good idea.
This could catch uses that should have considered checking the version and using typing_extensions but didn't.

You can go through the references of get_or_import_symbol which should have some of the symbols that are being imported from typing.

For example,

ruff/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs

Lines 129 to 142 in 410aa4b

let (no_return_edit, binding) = importer

.get_or_import_symbol(

&ImportRequest::import_from(

"typing",

if target_version >= PythonVersion::PY311 {

"Never"

} else {

"NoReturn"

},

),

at,

semantic,

)

.ok()?;

Yeah this is a good example, it looks like these are both in typing_extensions, so this could check both Never vs NoReturn and also the module.

The one downside of replacing all typing imports is that it will make the false positive issue from #9761 worse, i.e. affecting more rules, but only briefly, I plan to work on that after this.

ntBre · 2025-04-10T20:02:58Z

I went through all of the references for get_or_import_symbol and think I've handled all of the typing imports.

PYI058 looks a lot like it needs the new method, but it does its own bookkeeping on whether typing, typing_extensions, or collections.abc is already imported and reuses whichever one is already present, so I think it's okay to leave alone.

ruff/crates/ruff_linter/src/rules/flake8_pyi/rules/bad_generator_return_type.rs

Lines 128 to 142 in 8b2727c

    
           match (name, qualified_name.segments()) { 
        
               ("__iter__", ["typing", "Generator"]) => { 
        
                   (Method::Iter, Module::Typing, Generator::Generator) 
        
               } 
        
               ("__aiter__", ["typing", "AsyncGenerator"]) => { 
        
                   (Method::AIter, Module::Typing, Generator::AsyncGenerator) 
        
               } 
        
               ("__iter__", ["typing_extensions", "Generator"]) => { 
        
                   (Method::Iter, Module::TypingExtensions, Generator::Generator) 
        
               } 
        
               ("__aiter__", ["typing_extensions", "AsyncGenerator"]) => ( 
        
                   Method::AIter, 
        
                   Module::TypingExtensions, 
        
                   Generator::AsyncGenerator, 
        
               ),

I believe the same is true for UP006, which ends up in this function:

ruff/crates/ruff_python_stdlib/src/typing.rs

Lines 340 to 355 in 8b2727c

    
           pub fn as_pep_585_generic(module: &str, member: &str) -> Option<ModuleMember> { 
        
               match (module, member) { 
        
                   ("typing", "Dict") => Some(("", "dict")), 
        
                   ("typing", "FrozenSet") => Some(("", "frozenset")), 
        
                   ("typing", "List") => Some(("", "list")), 
        
                   ("typing", "Set") => Some(("", "set")), 
        
                   ("typing", "Tuple") => Some(("", "tuple")), 
        
                   ("typing", "Type") => Some(("", "type")), 
        
                   ("typing_extensions", "Type") => Some(("", "type")), 
        
                   ("typing", "Deque") => Some(("collections", "deque")), 
        
                   ("typing_extensions", "Deque") => Some(("collections", "deque")), 
        
                   ("typing", "DefaultDict") => Some(("collections", "defaultdict")), 
        
                   ("typing_extensions", "DefaultDict") => Some(("collections", "defaultdict")), 
        
                   _ => None, 
        
               } 
        
           }

I actually accepted new snapshot results for PYI026, which was previously using ImportRequest::import instead of ImportRequest::import_from. It seems to be the only one doing that, and the example in the docs also uses ImportFrom, so hopefully this doesn't need to be preview-gated.

crates/ruff_linter/src/rules/flake8_annotations/helpers.rs

dhruvmanila · 2025-04-10T20:28:41Z

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

+    ///
+    /// On Python <`python_version`, `member` is imported from `typing_extensions`, while on Python
+    /// >=`python_version`, it is imported from `typing`.
+    pub(crate) fn import_from_typing(


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

}

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.

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.

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.

MichaReiser

We can more freely change behavior of fixes because they aren't observed as "breaking" for users unlike rules that can trigger new lint violations. The main reason for gating fixes behind preview is to get more feedback on the fixe's correctness.

AlexWaygood

This is great, thank you!

AlexWaygood · 2025-04-11T12:34:23Z

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

+        member: &str,
+        position: TextSize,
+        python_version: PythonVersion,
+    ) -> Result<(Edit, String), ResolutionError> {


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

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.

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

* main: [red-knot] Specialize `str.startswith` for string literals (#17351) [syntax-errors] `yield`, `yield from`, and `await` outside functions (#17298) [red-knot] Refresh diagnostics when changing related files (#17350) Add `Checker::import_from_typing` (#17340) Don't add chaperone space after escaped quote in triple quote (#17216) [red-knot] Silence errors in unreachable type annotations / class bases (#17342)

Skylion007 · 2025-04-13T15:35:24Z

Can this be merged with the logic for the import upgrading rules in PYUP as that should have version info for all these import calls.

ntBre · 2025-04-14T13:21:52Z

Can this be merged with the logic for the import upgrading rules in PYUP as that should have version info for all these import calls.

Do you have any specific pyupgrade rules in mind? I looked through them briefly and didn't see any obvious ones. I thought most of these rules only run at all when the target Python version is high enough, but it's certainly plausible that this logic could be generalized and used elsewhere. I guess the idea here was that typing was a common-enough specific case that it made sense to have a special method for it.

Skylion007 · 2025-04-14T15:26:04Z

ruff/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs

Line 204 in 701aecb

const TYPING_EXTENSIONS_TO_TYPING_37: &[&str] = &[

Skylion007 · 2025-04-14T15:26:50Z

Can this be merged with the logic for the import upgrading rules in PYUP as that should have version info for all these import calls.

Do you have any specific pyupgrade rules in mind? I looked through them briefly and didn't see any obvious ones. I thought most of these rules only run at all when the target Python version is high enough, but it's certainly plausible that this logic could be generalized and used elsewhere. I guess the idea here was that typing was a common-enough specific case that it made sense to have a special method for it.

ruff/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs

Line 204 in 701aecb

const TYPING_EXTENSIONS_TO_TYPING_37: &[&str] = &[

ntBre · 2025-04-14T15:52:58Z

Ah yes, thank you! Those did not come up in my searches but look very relevant.

Summary -- This PR resolves #9761 by adding a linter configuration option to disable `typing_extensions` imports. As mentioned [here], it would be ideal if we could detect whether or not `typing_extensions` is available as a dependency automatically, but this seems like a much easier fix in the meantime. The default for the new option `disable-typing-extensions` is `false`, preserving the current behavior. Setting it to `true` will bail out of the new `Checker::import_from_typing` method (#17340) with a new `TypingImportError::TypingExtensionsDisabled` error, which should log an explanatory message whenever `Diagnostic::try_set_fix` is used. [here]: #9761 (comment) Test Plan -- New linter tests exercising several combinations of Python versions and the new config option for PYI019. We could also test other rules, but any rule using `import_from_typing` should behave the same way.

Summary -- This PR resolves #9761 by adding a linter configuration option to disable `typing_extensions` imports. As mentioned [here], it would be ideal if we could detect whether or not `typing_extensions` is available as a dependency automatically, but this seems like a much easier fix in the meantime. The default for the new option, `typing-extensions`, is `true`, preserving the current behavior. Setting it to `false` will bail out of the new `Checker::typing_importer` method, which has been refactored from the `Checker::import_from_typing` method in #17340), with `None`, which is then handled specially by each rule that calls it. I considered some alternatives to a config option, such as checking if `typing_extensions` has been imported or checking for a `TYPE_CHECKING` block we could use, but I think defaulting to allowing `typing_extensions` imports and allowing the user to disable this with an option is both simple to implement and pretty intuitive. [here]: #9761 (comment) Test Plan -- New linter tests exercising several combinations of Python versions and the new config option for PYI019. I also added tests for the other affected rules, but only in the case where the new config option is enabled. The rules' existing tests also cover the default case.

ntBre added the internal An internal refactor or improvement label Apr 10, 2025

ntBre marked this pull request as ready for review April 10, 2025 18:25

ntBre requested a review from AlexWaygood as a code owner April 10, 2025 18:25

ntBre added 2 commits April 10, 2025 15:36

convert AutoPythonType::into_expression to use import_from_typing

684ae0d

replace other typing imports

f4c0317

dhruvmanila approved these changes Apr 10, 2025

View reviewed changes

add PythonVersion::lowest and use it instead of hard-coding 3.8

1a453ea

MichaReiser approved these changes Apr 11, 2025

View reviewed changes

AlexWaygood approved these changes Apr 11, 2025

View reviewed changes

update python version parameter name and extend docs

3a8aadc

ntBre enabled auto-merge (squash) April 11, 2025 13:36

ntBre merged commit 8e11c53 into main Apr 11, 2025
19 checks passed

ntBre deleted the brent/refactor-typing-imports branch April 11, 2025 13:37

ntBre mentioned this pull request Apr 24, 2025

Add config option to disable typing_extensions imports #17611

Merged

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

Add Checker::import_from_typing #17340

Add Checker::import_from_typing #17340

Uh oh!

Conversation

ntBre commented Apr 10, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

dhruvmanila commented Apr 10, 2025

Uh oh!

ntBre commented Apr 10, 2025

Uh oh!

ntBre commented Apr 10, 2025

Uh oh!

Uh oh!

dhruvmanila Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Skylion007 commented Apr 13, 2025

Uh oh!

ntBre commented Apr 14, 2025

Uh oh!

Skylion007 commented Apr 14, 2025

Uh oh!

Skylion007 commented Apr 14, 2025

Uh oh!

ntBre commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Add `Checker::import_from_typing` #17340

Add `Checker::import_from_typing` #17340

github-actions bot commented Apr 10, 2025 •

edited

Loading

`ruff-ecosystem` results