flag `deprecated` exports in autoimport and move to bottom #1694

DanielNoord · 2025-11-26T21:49:36Z

Summary

References #1685

This implements one of the improvements discussed in the issue: taking into account deprecations when checking exports for autoimport.

I have added two tests. One tests whether we suggest imports from the stdlib. It doesn't seem we test this yet and I was figuring out how the tests worked and came up with this. I can remove it, but since I wrote it I thought I might as well keep it? It also shows an improvement we should make (typing over ast).

The second test covers the change I make in the PR. It is pretty simple really.

Test Plan

See above. Added two tests.

Also tried to run the CI on my local fork in DanielNoord#1 but it seems the setup is quite different from the checks I see running on other PRs in this repository.

I did. There are quite a lot of warnings about unused lint ignores and unused exports, but my changes seem okay? Let me know if I missed anything.

meta-cla · 2025-11-26T21:49:42Z

Hi @DanielNoord!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

DanielNoord · 2025-11-26T22:02:15Z

For the reviewer, I also found:

pyrefly/pyrefly/lib/state/lsp.rs

Lines 2226 to 2230 in c0ebd8c

    
           tags: if export.deprecation.is_some() { 
        
               Some(vec![CompletionItemTag::DEPRECATED]) 
        
           } else { 
        
               None 
        
           },

There is a difference between completions and code actions, I guess? I'm not quite sure what the difference between them is, but I think the code on main already shows there is a potential for drift. In completions deprecation is taken into account while for code actions it isn't.
I have now excluded deprecated exports from code actions (causing another type of drift with completions), mainly because search_exports_exact doesn't really provide a nice API to "downrank" handles and just either returns them or not.

Do we need to address this drift? And if so, how?
And does this PR actually address the error I'm trying to fix? Or does the code action not really matter in practice?

meta-cla · 2025-11-27T00:09:30Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

kinto0

thanks for the change! I like the small change with lots of tests.

Do we need to address this drift? And if so, how?

I'm not especially concerned about the drift right now. from what I see, completion autoimports use a fuzzy search and quick fixes don't. that makes sense to me: I don't want a quick fix changing the identifier - I only want it adding imports.

if I were to work on this, I'd likely change search_exports_exact's API to return some notion of deprecated as well and use that metadata to format the quick fixes with some type of warning similar to imports (along with downranking them if you want)

but then the bigger question of "how do we sort these correctly" is still unanswered.... I'll comment on the original task for that

pyrefly/lib/test/lsp/code_actions.rs

kinto0 · 2025-12-01T17:27:50Z

pyrefly/lib/state/lsp.rs

+                        // Ignore deprecated exports in autoimport suggestions
+                        if export.deprecation.is_some() {
+                            return Vec::new();
+                        }


I wonder if we can add metadata making it clear that these are deprecated instead of ignoring these (lots of people might still want to use them).

completion items are the most likely to be accidentally imported, but they have the tag called deprecated that formats it as strikethrough. maybe the next step in improving this is deranking them with sortText?

quick fix code actions are more of an opt-in action, so I'm not too concerned about people accidentally importing deprecated functionality. it might make sense to instead format it with a strike-through or write (deprecated) next to it. what do you think?

Do we know if IDEs take the deprecated into consideration when sorting completions? To me it feels more of a concern of the IDE (or whatever is taking in the LSP output) than of the LSP itself. We already provide all "necessary" data for the sorting algorithm to decide (by indicating it as being deprecated).

For quick fix I think adding (deprecated) makes the most sense?

However, perhaps that should be tackled in a follow up (related to the linked issue). It seems we want more things to be taken into consideration when sorting, which might influence the type of "metadata"/context we show for the action?

Do we know if IDEs take the deprecated into consideration when sorting completions? To me it feels more of a concern of the IDE (or whatever is taking in the LSP output) than of the LSP itself. We already provide all "necessary" data for the sorting algorithm to decide (by indicating it as being deprecated).

Since we provide sortText, I doubt they'd push these results to the bottom out of sortText order. It seems like language servers should decide this.

For quick fix I think adding (deprecated) makes the most sense?

agreed

@kinto0 Could you provide some pointers on the datastructure to return and where to define it?

I'm not too experienced with larger rust codebases like these. I thought of maybe combining Handle and ExportLocation in the a `struct? But perhaps that is not efficient enough.
Very open to any kind of feedback/tips :)

If I were you I'd scope this as follows:

this PR: add deprecated warning on autoimport

another PR: sort deprecated to the bottom

in order to do these, I might start by changing search_exports_exact to return a Vec<(Handle, bool (representing is_deprecated)>. then you can reuse is_deprecated upstream in sorting / text.

DanielNoord · 2025-12-01T21:15:19Z

but then the bigger question of "how do we sort these correctly" is still unanswered.... I'll comment on the original task for that

This is also what I struggle with. I'm scared of overscoping this PR but am not sure what you're preferred course of action is. If this already provides value in your opinion I would merge it, but can also understand if you first want to answer the bigger question :)

kinto0 · 2025-12-01T21:24:49Z

but then the bigger question of "how do we sort these correctly" is still unanswered.... I'll comment on the original task for that

This is also what I struggle with. I'm scared of overscoping this PR but am not sure what you're preferred course of action is. If this already provides value in your opinion I would merge it, but can also understand if you first want to answer the bigger question :)

I think this provides value if it adds the (deprecated) field. But I think removing quick fixes might not be what some users want. Let me look into how pyright sorts / filters these completion items....

pyrefly/lib/state/lsp.rs

DanielNoord · 2025-12-06T13:30:32Z

pyrefly/lib/state/lsp.rs

+        code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| {
+            match (title1.contains("deprecated"), title2.contains("deprecated")) {
+                (true, false) => Ordering::Greater,
+                (false, true) => Ordering::Less,
+                _ => title1.cmp(title2),
+            }
+        });


This also feels very non-idiomatic...

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.

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?

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)

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.

DanielNoord · 2025-12-06T13:31:25Z

@kinto0 I have changed the test and code. It now shows we prefer the import from b.py over a.py due to deprecation and that deprecation is being shown in the action as well. Let me know what you think!

kinto0

thanks for the clean code and comments! a few small suggestions but otherwise looks great

pyrefly/lib/state/lsp.rs

kinto0 · 2025-12-08T18:07:47Z

pyrefly/lib/state/lsp.rs

+        code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| {
+            match (title1.contains("deprecated"), title2.contains("deprecated")) {
+                (true, false) => Ordering::Greater,
+                (false, true) => Ordering::Less,
+                _ => title1.cmp(title2),
+            }
+        });


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.

DanielNoord · 2025-12-12T08:23:49Z

@kinto0 Is there anything left to do here? If possible I'd like to avoid future merge conflicts 😄

meta-codesync · 2025-12-12T16:12:45Z

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D89058677.

kinto0 · 2025-12-12T16:15:46Z

@kinto0 Is there anything left to do here? If possible I'd like to avoid future merge conflicts 😄

sorry!!! not sure how I missed this. I swear I imported it when I renamed it. oh well, thanks for fixing the conflicts.

yangdanny97

Review automatically exported from Phabricator review in Meta.

meta-codesync · 2025-12-12T18:11:52Z

@kinto0 merged this pull request in 2b19816.

DanielNoord mentioned this pull request Nov 26, 2025

Auto import suggestions for collections.abc should take precedence over typing #1685

Open

meta-cla bot added the cla signed label Nov 27, 2025

kinto0 self-assigned this Dec 1, 2025

kinto0 requested changes Dec 1, 2025

View reviewed changes

DanielNoord added 2 commits December 6, 2025 14:15

Skip deprecated exports in autoimport

856a4ec

Don't remove, but sort deprecated imports lower

d2d5ba9

DanielNoord force-pushed the deprecated-imports branch from 751f8f2 to d2d5ba9 Compare December 6, 2025 13:29

DanielNoord commented Dec 6, 2025

View reviewed changes

pyrefly/lib/state/lsp.rs Outdated Show resolved Hide resolved

DanielNoord commented Dec 6, 2025

View reviewed changes

DanielNoord requested a review from kinto0 December 6, 2025 13:30

kinto0 requested changes Dec 8, 2025

View reviewed changes

DanielNoord added 2 commits December 8, 2025 20:44

Use map_or instead of making title mut

f799c78

Rename to local_quickfix_code_actions_sorted

187e0d7

DanielNoord requested a review from kinto0 December 8, 2025 19:48

kinto0 changed the title ~~Skip deprecated exports in autoimport~~ flag deprecated exports in autoimport and move to bottom Dec 8, 2025

Merge branch 'main' into deprecated-imports

7470c82

kinto0 approved these changes Dec 12, 2025

View reviewed changes

yangdanny97 approved these changes Dec 12, 2025

View reviewed changes

meta-codesync bot closed this in 2b19816 Dec 12, 2025

facebook-github-bot added the Merged label Dec 12, 2025

DanielNoord deleted the deprecated-imports branch December 12, 2025 19:17

flag deprecated exports in autoimport and move to bottom #1694

flag deprecated exports in autoimport and move to bottom #1694

Conversation

DanielNoord commented Nov 26, 2025

Summary

Test Plan

Uh oh!

meta-cla bot commented Nov 26, 2025

Action Required

Process

Uh oh!

DanielNoord commented Nov 26, 2025

Uh oh!

meta-cla bot commented Nov 27, 2025

Uh oh!

kinto0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinto0 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Dec 1, 2025

Uh oh!

kinto0 commented Dec 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Dec 6, 2025

Uh oh!

kinto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Dec 12, 2025

Uh oh!

meta-codesync bot commented Dec 12, 2025

Uh oh!

kinto0 commented Dec 12, 2025

Uh oh!

yangdanny97 left a comment

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flag `deprecated` exports in autoimport and move to bottom #1694

flag `deprecated` exports in autoimport and move to bottom #1694

kinto0 left a comment •

edited

Loading

kinto0 Dec 4, 2025 •

edited

Loading