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

[DanielNoord]

Describe the Bug

When running pyrefly as LSP I'd expect imports that are available in collections.abc to take precedence of those coming from typing.
Considering they are deprecated, see for example https://docs.python.org/3/library/typing.html#typing.Iterable

For example in def func(x: Iterable[str]) -> None: ... the import from collections.abc import Iterable should come before from typing import Iterable.
However, pyrefly doesn't find it, or suggests to import it from _collections_abc.

I believe the logic for sorting these actions is in:

pyrefly/pyrefly/lib/state/lsp.rs

Line 1676 in 66dfdbf

code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| title1.cmp(title2));

That should probably learn about a certain precedence of certain modules over others.
However, at the moment pyrefly seems unable to even recognize collections.abc as a shim for _collections_abc.

I found the following code which could explain this:

pyrefly/pyrefly/lib/state/lsp.rs

Lines 2822 to 2828 in 66dfdbf

	ExportLocation::ThisModule(_) => vec![handle.dupe()],
	// 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.
	// The current strategy will prevent intended re-exports from showing up in
	// result list, but it's better than showing thousands of likely bad results.
	ExportLocation::OtherModule(..) => Vec::new(),

I'd love to contribute to this project as I'd like to learn a bit more Rust and consider myself to have more than adequate knowledge of Python, but this seems like it might be too involved for a first contribution. If somebody is willing to coach me, I'd be up for trying that though!

Sandbox Link

I don't think the Sandbox supports generating samples of LSP output

(Only applicable for extension issues) IDE Information

No response

[stroxler]

Thanks for the bug report! I think you're right, we shold prefer the collections variants.

We ideally would also downrank anything marked as @deprecated, but that's a separate implementation I think, and probably not as important as the core collection types

[DanielNoord]

@stroxler Do you think this could serve as a good first issue? Or is it too complex?

[yangdanny97]

As steven mentioned, this could be split on 2 parts:

1. downranking deprecated imports
2. fixing _collections_abc to be collections.abc

I'm not sure how to do the second one (cc @kinto0 on the implementation details) but the first item should be easy enough to be a good first issue, if you'd like to tackle it.

[DanielNoord]

I have tried to implement the check for deprecated imports in #1694.

I went with removing deprecated exports from the selection. There are some questions about this behaviour and the code in question that we can discuss there.

However, when looking at the stdlib I noticed that typing.Callable isn't actually marked as deprecated except for in the docstring:
https://github.com/python/cpython/blob/f47e928574187ae97c778e326739b3aa0c7dc765/Lib/typing.py#L2778

Similarly, in the PR I have added you can see that ast.TypeVar takes precedence over typing.TypeVar.

Therefore, we do not only need to do 2. but likely need to add a third step to find some way to indicate that certain modules should (always) take precedence over others. In this case that would be collections.abc > typing > ast.

[kinto0]

Aside from deprecated, I wonder if there are good heuristics for ranking these.

Maybe length would work? That captures the general idea that top-level libraries are more public facing.

from typing import TYPE_CHECKING should take precedence over all of the other ones (report here)

[yangdanny97]

Maybe length would work? That captures the general idea that top-level libraries are more public facing.

As a general case maybe that makes sense, but in this specific case the preference of collections.abc > typing > ast means it's reverse length order lol.

[kinto0]

pyright seems like it uses depth(number of dots) to compare these.

then pyright also uses MRU to put recent auto imports at the top

I bet a combination of these will be good enough for the general case. when someone manually uses collections.abc, the next import (in the same session) will also come from there

[DanielNoord]

Those all seem valid but quite hard to implement considering we currently just have a Vec<Handle>. I also commented in the linked PR, but I think I need some pointers on how to change the data structure to return all data that is needed to execute these strategies when sorting.