Skip deprecated exports in autoimport

[DanielNoord]

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]

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]

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]

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

[kinto0]

this test is great, i'd like to keep it

the one concern would be that different python / typeshed versions change where TypeVar comes from, but since we bundled typeshed it should match on all systems. an integration test of this behavior (where we actually use system python) would be less ideal than what you did.

[kinto0]

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?

[DanielNoord]

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?

[kinto0]

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

[DanielNoord]

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]

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....