Skip to content
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.

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

vendored_typeshed_versions should use db.vendored #13434

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Sep 21, 2024

Summary

This fixes a bug in the module resolver where it always defaulted to the versions file
bundled with Red Knot rather than using the vendored file system instance returned by db.vendored.

Not using db.vendored can lead to inconsistencies, as seen in the ruff module graph resolver where
the module resolver loads the versions file from the static vendored file system but resolves the modules from the empty vendored file system set on the ModuleGraphDb.

Now, this also shows that Ruff needs the vendored stubs. At least, the VERSIONS file because Red Knot verifies if the stub directory is valid when constructing the ProgramSettings to prevent the situation where a user runs red_knot fix with a broken custom typeshed dir.

Test Plan

cargo test

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Sep 21, 2024
@MichaReiser MichaReiser force-pushed the micha/use-vendored-fs-from-db branch 2 times, most recently from 1d025e8 to 5b15b53 Compare September 21, 2024 09:22
Copy link
Contributor

github-actions bot commented Sep 21, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

} else {
tracing::debug!("Using vendored stdlib");
(
ResolvedTypeshedVersions::Vendored(vendored_typeshed_versions()),
Copy link
Member Author

@MichaReiser MichaReiser Sep 21, 2024

Choose a reason for hiding this comment

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

I think it's fine to defer validating the vendored typeshed versions. They should be valid and whether we panic here or when trying to resolve the first module doesn't make a real difference. It results in a panic.

The only downside is that getting the versions is now a salsa lookup rather than a static field access. Let's see if this hurts performance in a meaningful way

Copy link
Member Author

@MichaReiser MichaReiser Sep 21, 2024

Choose a reason for hiding this comment

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

Okay, there's a 1% performance drop. That doesn't seem worth it.

I'm just gonna make vendored_typeshed_versions always parse the file without caching. This is an overall simplification and shouldn't hurt performance except in the case when we reconstruct the SearchPathSettings because the project configuration has changed.... but that's a very slow path anyway and parsing the VERSIONS file should be rather fast

Comment on lines -283 to -286
enum ResolvedTypeshedVersions {
Vendored(&'static TypeshedVersions),
Custom(TypeshedVersions),
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why I didn't use Cow for this... 🫤

@MichaReiser
Copy link
Member Author

I tried to make the typeshed versions usage as lazy as possible but there are some module graph tests that still access typeshed for imports that fail to resolve. We would have to add support for no typeshed to the module resolver to make the vendored stubs truly optional.

@MichaReiser MichaReiser merged commit 8921fbb into main Sep 21, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/use-vendored-fs-from-db branch September 21, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant