From 8f530a7ab0643028dbbb44b7a5a1e42ffe67679d Mon Sep 17 00:00:00 2001 From: Aria Desires Date: Mon, 15 Dec 2025 10:14:36 -0500 Subject: [PATCH] [ty] Add "qualify ..." code fix for undefined references (#21968) ## Summary If `import warnings` exists in the file, we will suggest an edit of `deprecated -> warnings.deprecated` as "qualify warnings.deprecated" ## Test Plan Should test more cases... --- crates/ty_ide/src/code_action.rs | 462 +++++++++++++----- crates/ty_ide/src/completion.rs | 48 +- ...n_existing_import_undefined_decorator.snap | 45 ++ 3 files changed, 428 insertions(+), 127 deletions(-) diff --git a/crates/ty_ide/src/code_action.rs b/crates/ty_ide/src/code_action.rs index 2b6703afac..5c4a769dd7 100644 --- a/crates/ty_ide/src/code_action.rs +++ b/crates/ty_ide/src/code_action.rs @@ -29,12 +29,11 @@ pub fn code_actions( let mut actions = Vec::new(); - // Suggest imports for unresolved references (often ideal) - // TODO: suggest qualifying with an already imported symbol + // Suggest imports/qualifications for unresolved references (often ideal) let is_unresolved_reference = lint_id == LintId::of(&UNRESOLVED_REFERENCE) || lint_id == LintId::of(&UNDEFINED_REVEAL); if is_unresolved_reference - && let Some(import_quick_fix) = create_import_symbol_quick_fix(db, file, diagnostic_range) + && let Some(import_quick_fix) = unresolved_fixes(db, file, diagnostic_range) { actions.extend(import_quick_fix); } @@ -49,7 +48,7 @@ pub fn code_actions( actions } -fn create_import_symbol_quick_fix( +fn unresolved_fixes( db: &dyn Db, file: File, diagnostic_range: TextRange, @@ -59,7 +58,7 @@ fn create_import_symbol_quick_fix( let symbol = &node.expr_name()?.id; Some( - completion::missing_imports(db, file, &parsed, symbol, node) + completion::unresolved_fixes(db, file, &parsed, symbol, node) .into_iter() .map(|import| QuickFix { title: import.label, @@ -84,6 +83,7 @@ mod tests { system::{DbWithWritableSystem, SystemPathBuf}, }; use ruff_diagnostics::Fix; + use ruff_python_trivia::textwrap::dedent; use ruff_text_size::{TextRange, TextSize}; use ty_project::ProjectMetadata; use ty_python_semantic::{ @@ -149,15 +149,14 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a / 0 # ty:ignore[division-by-zero] - | ^ + 2 | b = a / 0 # ty:ignore[division-by-zero] + | ^ | 1 | - - b = a / 0 # ty:ignore[division-by-zero] - 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference] - 3 | + - b = a / 0 # ty:ignore[division-by-zero] + 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference] "); } @@ -171,15 +170,14 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a / 0 # ty:ignore[division-by-zero,] - | ^ + 2 | b = a / 0 # ty:ignore[division-by-zero,] + | ^ | 1 | - - b = a / 0 # ty:ignore[division-by-zero,] - 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference] - 3 | + - b = a / 0 # ty:ignore[division-by-zero,] + 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference] "); } @@ -193,15 +191,14 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a / 0 # ty:ignore[division-by-zero ] - | ^ + 2 | b = a / 0 # ty:ignore[division-by-zero ] + | ^ | 1 | - - b = a / 0 # ty:ignore[division-by-zero ] - 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference ] - 3 | + - b = a / 0 # ty:ignore[division-by-zero ] + 2 + b = a / 0 # ty:ignore[division-by-zero, unresolved-reference ] "); } @@ -215,15 +212,14 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a / 0 # ty:ignore[division-by-zero] some explanation - | ^ + 2 | b = a / 0 # ty:ignore[division-by-zero] some explanation + | ^ | 1 | - - b = a / 0 # ty:ignore[division-by-zero] some explanation - 2 + b = a / 0 # ty:ignore[division-by-zero] some explanation # ty:ignore[unresolved-reference] - 3 | + - b = a / 0 # ty:ignore[division-by-zero] some explanation + 2 + b = a / 0 # ty:ignore[division-by-zero] some explanation # ty:ignore[unresolved-reference] "); } @@ -241,22 +237,22 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:3:21 + --> main.py:3:9 | - 2 | b = ( - 3 | / a # ty:ignore[division-by-zero] - 4 | | / - 5 | | 0 - | |_____________________^ - 6 | ) + 2 | b = ( + 3 | / a # ty:ignore[division-by-zero] + 4 | | / + 5 | | 0 + | |_________^ + 6 | ) | 1 | - 2 | b = ( - - a # ty:ignore[division-by-zero] - 3 + a # ty:ignore[division-by-zero, unresolved-reference] - 4 | / - 5 | 0 - 6 | ) + 2 | b = ( + - a # ty:ignore[division-by-zero] + 3 + a # ty:ignore[division-by-zero, unresolved-reference] + 4 | / + 5 | 0 + 6 | ) "); } @@ -274,22 +270,21 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:3:21 + --> main.py:3:9 | - 2 | b = ( - 3 | / a - 4 | | / - 5 | | 0 # ty:ignore[division-by-zero] - | |_____________________^ - 6 | ) + 2 | b = ( + 3 | / a + 4 | | / + 5 | | 0 # ty:ignore[division-by-zero] + | |_________^ + 6 | ) | - 2 | b = ( - 3 | a - 4 | / - - 0 # ty:ignore[division-by-zero] - 5 + 0 # ty:ignore[division-by-zero, unresolved-reference] - 6 | ) - 7 | + 2 | b = ( + 3 | a + 4 | / + - 0 # ty:ignore[division-by-zero] + 5 + 0 # ty:ignore[division-by-zero, unresolved-reference] + 6 | ) "); } @@ -307,22 +302,22 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:3:21 + --> main.py:3:9 | - 2 | b = ( - 3 | / a # ty:ignore[division-by-zero] - 4 | | / - 5 | | 0 # ty:ignore[division-by-zero] - | |_____________________^ - 6 | ) + 2 | b = ( + 3 | / a # ty:ignore[division-by-zero] + 4 | | / + 5 | | 0 # ty:ignore[division-by-zero] + | |_________^ + 6 | ) | 1 | - 2 | b = ( - - a # ty:ignore[division-by-zero] - 3 + a # ty:ignore[division-by-zero, unresolved-reference] - 4 | / - 5 | 0 # ty:ignore[division-by-zero] - 6 | ) + 2 | b = ( + - a # ty:ignore[division-by-zero] + 3 + a # ty:ignore[division-by-zero, unresolved-reference] + 4 | / + 5 | 0 # ty:ignore[division-by-zero] + 6 | ) "); } @@ -339,20 +334,19 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:3:18 + --> main.py:3:6 | - 2 | b = f""" - 3 | {a} - | ^ - 4 | more text - 5 | """ + 2 | b = f""" + 3 | {a} + | ^ + 4 | more text + 5 | """ | - 2 | b = f""" - 3 | {a} - 4 | more text - - """ - 5 + """ # ty:ignore[unresolved-reference] - 6 | + 2 | b = f""" + 3 | {a} + 4 | more text + - """ + 5 + """ # ty:ignore[unresolved-reference] "#); } @@ -371,23 +365,23 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:4:17 + --> main.py:4:5 | - 2 | b = f""" - 3 | { - 4 | a - | ^ - 5 | } - 6 | more text + 2 | b = f""" + 3 | { + 4 | a + | ^ + 5 | } + 6 | more text | 1 | - 2 | b = f""" - 3 | { - - a - 4 + a # ty:ignore[unresolved-reference] - 5 | } - 6 | more text - 7 | """ + 2 | b = f""" + 3 | { + - a + 4 + a # ty:ignore[unresolved-reference] + 5 | } + 6 | more text + 7 | """ "#); } @@ -403,19 +397,18 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a + """ - | ^ - 3 | more text - 4 | """ + 2 | b = a + """ + | ^ + 3 | more text + 4 | """ | 1 | - 2 | b = a + """ - 3 | more text - - """ - 4 + """ # ty:ignore[unresolved-reference] - 5 | + 2 | b = a + """ + 3 | more text + - """ + 4 + """ # ty:ignore[unresolved-reference] "#); } @@ -430,17 +423,16 @@ mod tests { assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" info[code-action]: Ignore 'unresolved-reference' for this line - --> main.py:2:17 + --> main.py:2:5 | - 2 | b = a \ - | ^ - 3 | + "test" + 2 | b = a \ + | ^ + 3 | + "test" | 1 | - 2 | b = a \ - - + "test" - 3 + + "test" # ty:ignore[unresolved-reference] - 4 | + 2 | b = a \ + - + "test" + 3 + + "test" # ty:ignore[unresolved-reference] "#); } @@ -454,27 +446,249 @@ mod tests { assert_snapshot!(test.code_actions(&UNDEFINED_REVEAL), @r" info[code-action]: import typing.reveal_type - --> main.py:2:13 + --> main.py:2:1 | - 2 | reveal_type(1) - | ^^^^^^^^^^^ + 2 | reveal_type(1) + | ^^^^^^^^^^^ | help: This is a preferred code action 1 + from typing import reveal_type 2 | - 3 | reveal_type(1) - 4 | + 3 | reveal_type(1) info[code-action]: Ignore 'undefined-reveal' for this line - --> main.py:2:13 + --> main.py:2:1 | - 2 | reveal_type(1) - | ^^^^^^^^^^^ + 2 | reveal_type(1) + | ^^^^^^^^^^^ | 1 | - - reveal_type(1) - 2 + reveal_type(1) # ty:ignore[undefined-reveal] + - reveal_type(1) + 2 + reveal_type(1) # ty:ignore[undefined-reveal] + "); + } + + #[test] + fn unresolved_deprecated() { + let test = CodeActionTest::with_source( + r#" + @deprecated("do not use") + def my_func(): ... + "#, + ); + + assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" + info[code-action]: import warnings.deprecated + --> main.py:2:2 + | + 2 | @deprecated("do not use") + | ^^^^^^^^^^ + 3 | def my_func(): ... + | + help: This is a preferred code action + 1 + from warnings import deprecated + 2 | + 3 | @deprecated("do not use") + 4 | def my_func(): ... + + info[code-action]: Ignore 'unresolved-reference' for this line + --> main.py:2:2 + | + 2 | @deprecated("do not use") + | ^^^^^^^^^^ + 3 | def my_func(): ... + | + 1 | + - @deprecated("do not use") + 2 + @deprecated("do not use") # ty:ignore[unresolved-reference] + 3 | def my_func(): ... + "#); + } + + #[test] + fn unresolved_deprecated_warnings_imported() { + let test = CodeActionTest::with_source( + r#" + import warnings + + @deprecated("do not use") + def my_func(): ... + "#, + ); + + assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r#" + info[code-action]: import warnings.deprecated + --> main.py:4:2 + | + 2 | import warnings 3 | + 4 | @deprecated("do not use") + | ^^^^^^^^^^ + 5 | def my_func(): ... + | + help: This is a preferred code action + 1 + from warnings import deprecated + 2 | + 3 | import warnings + 4 | + + info[code-action]: qualify warnings.deprecated + --> main.py:4:2 + | + 2 | import warnings + 3 | + 4 | @deprecated("do not use") + | ^^^^^^^^^^ + 5 | def my_func(): ... + | + help: This is a preferred code action + 1 | + 2 | import warnings + 3 | + - @deprecated("do not use") + 4 + @warnings.deprecated("do not use") + 5 | def my_func(): ... + + info[code-action]: Ignore 'unresolved-reference' for this line + --> main.py:4:2 + | + 2 | import warnings + 3 | + 4 | @deprecated("do not use") + | ^^^^^^^^^^ + 5 | def my_func(): ... + | + 1 | + 2 | import warnings + 3 | + - @deprecated("do not use") + 4 + @deprecated("do not use") # ty:ignore[unresolved-reference] + 5 | def my_func(): ... + "#); + } + + // using `importlib.abc.ExecutionLoader` when no imports are in scope + #[test] + fn unresolved_loader() { + let test = CodeActionTest::with_source( + r#" + ExecutionLoader + "#, + ); + + assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" + info[code-action]: import importlib.abc.ExecutionLoader + --> main.py:2:1 + | + 2 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + help: This is a preferred code action + 1 + from importlib.abc import ExecutionLoader + 2 | + 3 | ExecutionLoader + + info[code-action]: Ignore 'unresolved-reference' for this line + --> main.py:2:1 + | + 2 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + 1 | + - ExecutionLoader + 2 + ExecutionLoader # ty:ignore[unresolved-reference] + "); + } + + // using `importlib.abc.ExecutionLoader` when `import importlib` is in scope + // + // TODO: `importlib.abc` is available whenever `importlib` is, so qualifying + // `importlib.abc.ExecutionLoader` without adding imports is actually legal here! + #[test] + fn unresolved_loader_importlib_imported() { + let test = CodeActionTest::with_source( + r#" + import importlib + ExecutionLoader + "#, + ); + + assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" + info[code-action]: import importlib.abc.ExecutionLoader + --> main.py:3:1 + | + 2 | import importlib + 3 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + help: This is a preferred code action + 1 + from importlib.abc import ExecutionLoader + 2 | + 3 | import importlib + 4 | ExecutionLoader + + info[code-action]: Ignore 'unresolved-reference' for this line + --> main.py:3:1 + | + 2 | import importlib + 3 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + 1 | + 2 | import importlib + - ExecutionLoader + 3 + ExecutionLoader # ty:ignore[unresolved-reference] + "); + } + + // Using `importlib.abc.ExecutionLoader` when `import importlib.abc` is in scope + #[test] + fn unresolved_loader_abc_imported() { + let test = CodeActionTest::with_source( + r#" + import importlib.abc + ExecutionLoader + "#, + ); + + assert_snapshot!(test.code_actions(&UNRESOLVED_REFERENCE), @r" + info[code-action]: import importlib.abc.ExecutionLoader + --> main.py:3:1 + | + 2 | import importlib.abc + 3 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + help: This is a preferred code action + 1 + from importlib.abc import ExecutionLoader + 2 | + 3 | import importlib.abc + 4 | ExecutionLoader + + info[code-action]: qualify importlib.abc.ExecutionLoader + --> main.py:3:1 + | + 2 | import importlib.abc + 3 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + help: This is a preferred code action + 1 | + 2 | import importlib.abc + - ExecutionLoader + 3 + importlib.abc.ExecutionLoader + + info[code-action]: Ignore 'unresolved-reference' for this line + --> main.py:3:1 + | + 2 | import importlib.abc + 3 | ExecutionLoader + | ^^^^^^^^^^^^^^^ + | + 1 | + 2 | import importlib.abc + - ExecutionLoader + 3 + ExecutionLoader # ty:ignore[unresolved-reference] "); } @@ -493,7 +707,7 @@ mod tests { db.init_program().unwrap(); - let mut cleansed = source.to_string(); + let mut cleansed = dedent(source).to_string(); let start = cleansed .find("") diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs index 4e5051ddcc..a33f7600eb 100644 --- a/crates/ty_ide/src/completion.rs +++ b/crates/ty_ide/src/completion.rs @@ -67,6 +67,7 @@ impl<'db> Completions<'db> { self.items } + // Convert this collection into a list of "import..." fixes fn into_imports(mut self) -> Vec { self.items.sort_by(compare_suggestions); self.items @@ -82,6 +83,28 @@ impl<'db> Completions<'db> { .collect() } + // Convert this collection into a list of "qualify..." fixes + fn into_qualifications(mut self, range: TextRange) -> Vec { + self.items.sort_by(compare_suggestions); + self.items + .dedup_by(|c1, c2| (&c1.name, c1.module_name) == (&c2.name, c2.module_name)); + self.items + .into_iter() + .filter_map(|item| { + // If we would have to actually import something, don't suggest the qualification + // (we could, maybe we should, but for now, we don't) + if item.import.is_some() { + return None; + } + + Some(ImportEdit { + label: format!("qualify {}", item.insert.as_ref()?), + edit: Edit::replacement(item.insert?.into_string(), range.start(), range.end()), + }) + }) + .collect() + } + /// Attempts to adds the given completion to this collection. /// /// When added, `true` is returned. @@ -555,15 +578,19 @@ pub(crate) struct ImportEdit { pub edit: Edit, } -pub(crate) fn missing_imports( +/// Get fixes that would resolve an unresolved reference +pub(crate) fn unresolved_fixes( db: &dyn Db, file: File, parsed: &ParsedModuleRef, symbol: &str, node: AnyNodeRef, ) -> Vec { - let mut completions = Completions::exactly(db, symbol); + let mut results = Vec::new(); let scoped = ScopedTarget { node }; + + // Request imports we could add to put the symbol in scope + let mut completions = Completions::exactly(db, symbol); add_unimported_completions( db, file, @@ -574,8 +601,23 @@ pub(crate) fn missing_imports( }, &mut completions, ); + results.extend(completions.into_imports()); - completions.into_imports() + // Request qualifications we could apply to the symbol to make it resolve + let mut completions = Completions::exactly(db, symbol); + add_unimported_completions( + db, + file, + parsed, + scoped, + |module_name: &ModuleName, symbol: &str| { + ImportRequest::import(module_name.as_str(), symbol).force() + }, + &mut completions, + ); + results.extend(completions.into_qualifications(node.range())); + + results } /// Adds completions derived from keywords. diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action_existing_import_undefined_decorator.snap b/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action_existing_import_undefined_decorator.snap index f5e5a4f565..ceaf6ad94a 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action_existing_import_undefined_decorator.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__code_actions__code_action_existing_import_undefined_decorator.snap @@ -48,6 +48,51 @@ expression: code_actions }, "isPreferred": true }, + { + "title": "qualify warnings.deprecated", + "kind": "quickfix", + "diagnostics": [ + { + "range": { + "start": { + "line": 3, + "character": 1 + }, + "end": { + "line": 3, + "character": 11 + } + }, + "severity": 1, + "code": "unresolved-reference", + "codeDescription": { + "href": "https://ty.dev/rules#unresolved-reference" + }, + "source": "ty", + "message": "Name `deprecated` used when not defined" + } + ], + "edit": { + "changes": { + "file:///src/foo.py": [ + { + "range": { + "start": { + "line": 3, + "character": 1 + }, + "end": { + "line": 3, + "character": 11 + } + }, + "newText": "warnings.deprecated" + } + ] + } + }, + "isPreferred": true + }, { "title": "Ignore 'unresolved-reference' for this line", "kind": "quickfix",