vendored_typeshed_versions should use db.vendored#13434
Conversation
1d025e8 to
5b15b53
Compare
|
5b15b53 to
1964a2f
Compare
| } else { | ||
| tracing::debug!("Using vendored stdlib"); | ||
| ( | ||
| ResolvedTypeshedVersions::Vendored(vendored_typeshed_versions()), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
1964a2f to
69ab48f
Compare
| enum ResolvedTypeshedVersions { | ||
| Vendored(&'static TypeshedVersions), | ||
| Custom(TypeshedVersions), | ||
| } |
There was a problem hiding this comment.
Not sure why I didn't use Cow for this... 🫤
69ab48f to
6defd25
Compare
|
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. |
6defd25 to
1fca2c5
Compare
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.vendoredcan lead to inconsistencies, as seen in the ruff module graph resolver wherethe 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
ProgramSettingsto prevent the situation where a user runsred_knot fixwith a broken custom typeshed dir.Test Plan
cargo test