Skip to content

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 25, 2025

Summary

We now have a working module resolver in ty_python_semantic. ruff_python_resolver is unused, has very little test coverage, and gives the wrong impression that this could be used in ruff.

We didn't delete this in the past because we wanted it as a reference for ty's module resolver. I think we're now at a point where this is no longer necessary (ty's module resolver is further along). We can also always take a look at pyright's module resolver, from which this implementation was inspired from.

Test Plan

cargo build

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Jun 25, 2025
@MichaReiser MichaReiser force-pushed the micha/delete-ruff-resolver branch from 1d121a1 to 8409b52 Compare June 25, 2025 06:29
@MichaReiser MichaReiser requested a review from AlexWaygood June 25, 2025 06:29
@github-actions
Copy link
Contributor

mypy_primer results

No ecosystem changes detected ✅

@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood changed the title Delete the ruff_python_resovler crate Delete the ruff_python_resolver crate Jun 25, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 25, 2025

I still feel like this will be a useful reference for some of the more advanced features pyright's resolver has that ty's doesn't yet have, but which we want to add in the future. For example, pyright's resolver has the ability to resolve a module to a C extension, and therefore records the fact that the module does actually exist at runtime, pyright just has no types for it. That allows pyright to issue different diagnostics for "the module isn't installed" vs "the module is installed but you'll need to install some stubs before I can do type inference properly".

I guess this crate will always be in Ruff's git history if you really want to remove it now, though 😛

@MichaReiser
Copy link
Member Author

For example, pyright's resolver has the ability to resolve a module to a C extension, and therefore records the fact that the module does actually exist at runtime, pyright just has no types for it. That allows pyright to issue different diagnostics for "the module isn't installed" vs "the module is installed but you'll need to install some stubs before I can do type inference properly"

If that's the only use, then I suggest that we look at Pyright's module resolver implementation (which was the inspiration for our implementation). https://github.com/microsoft/pyright/blob/e5ecd9e32f54db97222a982db26fb182c34125d8/packages/pyright-internal/src/analyzer/importResolver.ts#L716

@AlexWaygood
Copy link
Member

Right, but I'm terrible at typescript and prefer reading Charlie's rust port 😆

@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 25, 2025

I mean, the port is type script written in Rust (and it's also not much TypeScript, it's mostly c-like code). Anyway, I don't think these are strong enough reasons to keep the code lingering, especially considering that it has confused contributors because they thought they could build on top of it.

@AlexWaygood
Copy link
Member

It sounds like you feel more strongly than I do — don't let me block you!

it has confused contributors because they thought they could build on top of it.

This is a great motivation, and one I haven't got much insight into, because I haven't been engaging with Ruff contributors to nearly the same extent you have over the past few months

@MichaReiser
Copy link
Member Author

Thank you.

It doesn't come up often but every now and then. There was another instance today (#9103 (comment)) which is why I opened this PR, very well knowing that we decided not to merge similar PRs for the reasons you mentioned before.

@MichaReiser MichaReiser merged commit c1fed55 into main Jun 25, 2025
36 checks passed
@MichaReiser MichaReiser deleted the micha/delete-ruff-resolver branch June 25, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants