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

Eagerly validate typeshed versions #12786

Merged
merged 6 commits into from
Aug 21, 2024
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 9, 2024

Summary

This should be the last PR in the module resolver validation stack.

It makes the typeshed version file parsing eagerly by moving it into SearchPaths::from_settings. This allows us to exit Red Knot early if the VERSIONS file is missing or incorrect. I added custom logic to the file watching to handle changes to the VERSIONS file (and added a test for it).

Handling changes to the VERSIONS file introduces some complexity because Program::update_search_paths takes a SearchPathSettings struct but we no longer know the SearchPathSettings in apply_changes.

A possible solution to this problem is to expose a method Program::reload_versions_file instead, which would do the trick just fine. However, I disliked that it is very low-level. We may have other cases where we need to reload the search-path settings, and I want to avoid introducing special case methods for each of those.

That's why I started looking into how we want to support configurations. What we need in apply_changes is access to what the user configured. The approach follows the same as Ruff's:

  • *Configuration: The configuration that uses Option in most places to know whether the user provided a value or not. Configurations from different sources can be merged (e.g. CLI overrides the pyproject.toml configuration)
  • *Settings: A resolved configuration in the sense that Red Knot fills in default values and e.g. merges values from different configurations (resolves the target-python version).

This PR does not add support for loading or deserializing configurations. It just builds out some of the infrastructure.

Test Plan

cargo test

cargo run -q --bin red_knot -- --current-directory=../test -v --custom-typeshed-dir ./blaaa
INFO Target version: 3.8
Red Knot failed
  Cause: Invalid search path settings
  Cause: Failed to read the custom typeshed versions file '/home/micha/astral/test/blaaa/stdlib/VERSIONS': No such file or directory (os error 2)

@MichaReiser MichaReiser changed the base branch from main to eagerly-validate-search-paths August 9, 2024 13:39
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Aug 9, 2024
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from b6f2f3d to fedd0b8 Compare August 9, 2024 13:52
@MichaReiser MichaReiser force-pushed the eagerly-validate-search-paths branch from 60568d5 to 099162a Compare August 9, 2024 15:18
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from fedd0b8 to 533ec59 Compare August 9, 2024 15:21
@@ -133,6 +133,8 @@ pub(crate) struct SearchPaths {
/// in terms of module-resolution priority until we've discovered the editable installs
/// for the first `site-packages` path
site_packages: Vec<SearchPath>,

typeshed_versions: Cow<'static, 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.

An alternative to storing typeshed_versions on SearchPaths would be to store the TypeshedVersions alongside the SearchPath (for the Custom or vendored stdlib variants). I don't really have an opinion on what's better. Let me know if you have ;)

@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from 533ec59 to c4f163f Compare August 9, 2024 15:25
@MichaReiser MichaReiser marked this pull request as ready for review August 9, 2024 15:26
Copy link
Contributor

github-actions bot commented Aug 9, 2024

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
Copy link
Member

If I understand correctly, this PR appears to treat the VERSIONS file as something that we can parse once upfront, and never parse again. That's true if we're using a vendored typeshed, but not if we're using a custom typeshed. The user might edit the VERSIONS file in between two queries; we need to be able to react to that and re-parse the VERSIONS file if our cached version is stale. That's why it's a Salsa query on main.

@MichaReiser
Copy link
Member Author

If I understand correctly, this PR appears to treat the VERSIONS file as something that we can parse once upfront, and never parse again. That's true if we're using a vendored typeshed, but not if we're using a custom typeshed. The user might edit the VERSIONS file in between two queries; we need to be able to react to that and re-parse the VERSIONS file if our cached version is stale. That's why it's a Salsa query on main.

Hmm I missed this point. I have to add some manual file-watching to trigger a reload in this case. It also forces us to explicitly think about what should happen if parsing the VERSION files fails when we were able to parse it successfully before (in watch mode)

@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from c4f163f to 6dab77a Compare August 12, 2024 07:39
Base automatically changed from eagerly-validate-search-paths to main August 12, 2024 07:47
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from 6dab77a to 4e2b2d7 Compare August 14, 2024 11:24
@MichaReiser MichaReiser marked this pull request as draft August 14, 2024 11:24
Copy link

codspeed-hq bot commented Aug 14, 2024

CodSpeed Performance Report

Merging #12786 will degrade performances by 6.56%

Comparing eagerly-resolve-typeshed-versions (ba1a840) with main (f873d2a)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main eagerly-resolve-typeshed-versions Change
linter/all-rules[numpy/globals.py] 726.4 µs 777.3 µs -6.56%

Comment on lines 85 to 87
/// The unresolved search path configuration.
#[return_ref]
pub search_path_configuration: SearchPathConfiguration,
Copy link
Member Author

@MichaReiser MichaReiser Aug 14, 2024

Choose a reason for hiding this comment

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

We may want to store the entire WorkspaceSettings on Workspace long term but it isn't clear to me if we actually want to do this. That's why I went with storing the absolute minimum

@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from d70ce0c to f2385fa Compare August 14, 2024 17:23
@MichaReiser MichaReiser marked this pull request as ready for review August 14, 2024 17:31
@MichaReiser MichaReiser marked this pull request as draft August 14, 2024 17:47
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch 2 times, most recently from 32ac8e8 to a4c9afe Compare August 15, 2024 09:30
@MichaReiser MichaReiser marked this pull request as ready for review August 15, 2024 09:32
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from a4c9afe to ecd8af3 Compare August 15, 2024 12:07
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

I skimmed this and it looks reasonable to me, but I'd rather have @AlexWaygood take a look.

@MichaReiser
Copy link
Member Author

I plan to merge this tomorrow afternoon.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks fantastic. Thanks for working on this, and sorry for the slow review!

crates/red_knot/tests/file_watching.rs Outdated Show resolved Hide resolved
crates/red_knot_workspace/src/db/changes.rs Outdated Show resolved Hide resolved
crates/red_knot_workspace/src/workspace.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Member Author

Uff, rebasing this PR after so long is painful

@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from ecd8af3 to d11c49c Compare August 21, 2024 15:44
@MichaReiser MichaReiser force-pushed the eagerly-resolve-typeshed-versions branch from d11c49c to ba1a840 Compare August 21, 2024 15:44
@MichaReiser MichaReiser enabled auto-merge (squash) August 21, 2024 15:47
@MichaReiser MichaReiser merged commit dce87c2 into main Aug 21, 2024
17 checks passed
@MichaReiser MichaReiser deleted the eagerly-resolve-typeshed-versions branch August 21, 2024 15:49
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.

3 participants