Skip to content

flag deprecated exports in autoimport and move to bottom #1694

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

Closed
DanielNoord wants to merge 5 commits into from
+112 −11
Closed

flag deprecated exports in autoimport and move to bottom #1694

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
856a4ec
Skip `deprecated` exports in autoimport
DanielNoord Nov 26, 2025
d2d5ba9
Don't remove, but sort deprecated imports lower
DanielNoord Dec 6, 2025
f799c78
Use `map_or` instead of making `title` `mut`
DanielNoord Dec 8, 2025
187e0d7
Rename to `local_quickfix_code_actions_sorted`
DanielNoord Dec 8, 2025
7470c82
Merge branch 'main' into deprecated-imports
DanielNoord Dec 12, 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
2 changes: 1 addition & 1 deletion pyrefly/lib/commands/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ impl InferArgs {
let imports: Vec<(TextSize, String, String)> = transaction
.search_exports_exact(unknown_name)
.into_iter()
.map(|handle_to_import_from| {
.map(|(handle_to_import_from, _)| {
insert_import_edit_with_forced_import_format(
&ast,
handle.dupe(),
Expand Down
2 changes: 1 addition & 1 deletion pyrefly/lib/lsp/non_wasm/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2376,7 +2376,7 @@ impl Server {
let range = self.from_lsp_range(uri, &module_info, params.range);
let mut actions = Vec::new();
if let Some(quickfixes) =
transaction.local_quickfix_code_actions(&handle, range, import_format)
transaction.local_quickfix_code_actions_sorted(&handle, range, import_format)
{
actions.extend(
quickfixes
Expand Down
33 changes: 25 additions & 8 deletions pyrefly/lib/state/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

use std::cmp::Ordering;
use std::cmp::Reverse;
use std::collections::BTreeMap;

Expand Down Expand Up @@ -1679,7 +1680,7 @@ impl<'a> Transaction<'a> {
}

/// Produce code actions that makes edits local to the file.
pub fn local_quickfix_code_actions(
pub fn local_quickfix_code_actions_sorted(
&self,
handle: &Handle,
range: TextRange,
Expand All @@ -1695,7 +1696,9 @@ impl<'a> Transaction<'a> {
let error_range = error.range();
if error_range.contains_range(range) {
let unknown_name = module_info.code_at(error_range);
for handle_to_import_from in self.search_exports_exact(unknown_name) {
for (handle_to_import_from, export) in
self.search_exports_exact(unknown_name)
{
let (position, insert_text, _) = insert_import_edit(
&ast,
self.config_finder(),
Expand All @@ -1705,7 +1708,12 @@ impl<'a> Transaction<'a> {
import_format,
);
let range = TextRange::at(position, TextSize::new(0));
let title = format!("Insert import: `{}`", insert_text.trim());
let title = format!(
"Insert import: `{}`{}",
insert_text.trim(),
export.deprecation.map_or("", |_| " (deprecated)")
);

code_actions.push((title, module_info.dupe(), range, insert_text));
}

Expand All @@ -1728,7 +1736,16 @@ impl<'a> Transaction<'a> {
_ => {}
}
}
code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| title1.cmp(title2));

// Sort code actions: non-deprecated first, then alphabetically
code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| {
match (title1.contains("deprecated"), title2.contains("deprecated")) {
(true, false) => Ordering::Greater,
(false, true) => Ordering::Less,
_ => title1.cmp(title2),
}
});
Comment on lines +1741 to +1747
Copy link
Contributor Author

@DanielNoord DanielNoord Dec 6, 2025

Choose a reason for hiding this comment

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

This also feels very non-idiomatic...

Copy link
Contributor

@kinto0 kinto0 Dec 8, 2025

Choose a reason for hiding this comment

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

if we sort within this function, we should rename it to local_quickfix_code_actions_sorted so callers know not to re-sort (I think most other state functions return unsorted items). keeping it in here is probably the simplest thing to do.

maybe instead of this sort, we add a sort_key to code_actions. when we notice deprecated and add it, we put a sort-key that will put it at the bottom. then in this function, we can just sort by key once at the end. this will let us add other things to sort by in the future. we don't need to return the sort_key, so we can map it out after/during sorting.

Copy link
Contributor Author

@DanielNoord DanielNoord Dec 8, 2025

Choose a reason for hiding this comment

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

I did the rename!

I'm not quite sure how to implement the sort_key. I grepped the code base but could only find examples in Python code. Do you have an example of an idiomatic rust implementation of such a property/attribute/whatever?

Copy link
Contributor

@kinto0 kinto0 Dec 8, 2025

Choose a reason for hiding this comment

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

in the code_actions.sort_by call, we would do something like this:

code_actions.sort_by_key(|(_, _, _, _, sort_key)| sort_key)

// remove sort key
code_actions.map(|(a, b, c, d, _)| (a, b, c, d));

this means all the logic of sorting can be confined to when we add the code action:

    if (export.deprecation.is_some()) {
        let title = format!(
            "Insert import: `{}`",
            insert_text.trim(),
        );
        code_actions.push((title, module_info.dupe(), range, insert_text, ""));
    } else {
        let title = format!(
            "Insert import: `{}` (deprecated)",
            insert_text.trim(),
        );
        code_actions.push((title, module_info.dupe(), range, insert_text, "zzzz"));
    }

(sorry this removes the other suggestion of insert import deduplication unless you can figure out a clean way to keep it)

Copy link
Contributor Author

@DanielNoord DanielNoord Dec 8, 2025

Choose a reason for hiding this comment

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

Hmm, I'm not sure how to do this cleanly. Because you'll also still want to sort by title itself so you'd get "zzzz - {}", title as the sort_key. It also doesn't feel very extensible to other sorting considerations?

If you think it is better to use the sort_key I can change it of course, but I'd probably keep as is for now so and come up with a better datastructure for all of our sorting needs in the rest of the linked issue.


Some(code_actions)
}

Expand Down Expand Up @@ -2963,11 +2980,11 @@ impl<'a> Transaction<'a> {
(result, is_incomplete)
}

pub fn search_exports_exact(&self, name: &str) -> Vec<Handle> {
pub fn search_exports_exact(&self, name: &str) -> Vec<(Handle, Export)> {
self.search_exports(|handle, exports| {
if let Some(export) = exports.get(&Name::new(name)) {
match export {
ExportLocation::ThisModule(_) => vec![handle.dupe()],
if let Some(export_location) = exports.get(&Name::new(name)) {
match export_location {
ExportLocation::ThisModule(export) => vec![(handle.dupe(), export.clone())],
// Re-exported modules like `foo` in `from from_module import foo`
// should likely be ignored in autoimport suggestions
// because the original export in from_module will show it.
Expand Down
86 changes: 85 additions & 1 deletion pyrefly/lib/test/lsp/code_actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn get_test_report(state: &State, handle: &Handle, position: TextSize) -> String
let mut report = "Code Actions Results:\n".to_owned();
let transaction = state.transaction();
for (title, info, range, patch) in transaction
.local_quickfix_code_actions(
.local_quickfix_code_actions_sorted(
handle,
TextRange::new(position, position),
ImportFormat::Absolute,
Expand Down Expand Up @@ -320,6 +320,90 @@ my_export
);
}

#[test]
fn test_import_from_stdlib() {
let report = get_batched_lsp_operations_report_allow_error(
&[("a", "TypeVar('T')\n# ^")],
get_test_report,
);
// TODO: Ideally `typing` would be preferred over `ast`.
assert_eq!(
r#"
# a.py
1 | TypeVar('T')
^
Code Actions Results:
# Title: Insert import: `from ast import TypeVar`

## Before:
TypeVar('T')
# ^
## After:
from ast import TypeVar
TypeVar('T')
# ^
# Title: Insert import: `from typing import TypeVar`

## Before:
TypeVar('T')
# ^
## After:
from typing import TypeVar
TypeVar('T')
# ^
"#
.trim(),
report.trim()
);
}

#[test]
fn test_take_deprecation_into_account_in_sorting_of_actions() {
let report = get_batched_lsp_operations_report_allow_error(
&[
(
"a",
"from warnings import deprecated\n@deprecated('')\ndef my_func(): pass",
),
("b", "def my_func(): pass"),
("c", "my_func()\n# ^"),
],
get_test_report,
);
assert_eq!(
r#"
# a.py

# b.py

# c.py
1 | my_func()
^
Code Actions Results:
# Title: Insert import: `from b import my_func`

## Before:
my_func()
# ^
## After:
from b import my_func
my_func()
# ^
# Title: Insert import: `from a import my_func` (deprecated)

## Before:
my_func()
# ^
## After:
from a import my_func
my_func()
# ^
"#
.trim(),
report.trim()
);
}

#[test]
fn extract_function_basic_refactor() {
let code = r#"
Expand Down