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 search paths #12783

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Eagerly validate search paths #12783

merged 4 commits into from
Aug 12, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 9, 2024

Summary

This PR moves from a lazy search path validation and makes it eagerly so that Red Knot fails to start if the settings are incorrect.

The main change is that this PR removes ModuleResolutionSettings and instead stores the "resolved" SearchPaths directly on Program. Program gets a new Program::from_settings(db, settings) function that returns an error if any setting has an invalid value.

Test Plan

cargo test

cargo run -q --bin red_knot -- --current-directory=../test -v --extra-search-path ./blaaa
INFO Target version: py38
Red Knot failed
  Cause: Invalid search path settings
  Cause: ./blaaa does not point to a directory

@MichaReiser MichaReiser changed the base branch from main to warn-about-skipped-pth-files August 9, 2024 12:55
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Aug 9, 2024
@MichaReiser MichaReiser force-pushed the eagerly-validate-search-paths branch from f751f13 to 54b5f0b Compare August 9, 2024 13:02
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.

Base automatically changed from warn-about-skipped-pth-files to main August 9, 2024 14:29
An error occurred while trying to automatically change base from warn-about-skipped-pth-files to main August 9, 2024 14:29
@MichaReiser MichaReiser force-pushed the eagerly-validate-search-paths branch from 60568d5 to 099162a Compare August 9, 2024 15:18
@MichaReiser MichaReiser marked this pull request as ready for review August 9, 2024 15:20
let search_path = SearchPath::extra(system, path)?;
files.try_add_root(
db.upcast(),
search_path.as_system_path().unwrap(),
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't forgotten about the unwrap. I addressed it in the following PR. I think this PR even added a few more as_system_path().unwrap() calls.

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.

Nice!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@MichaReiser MichaReiser enabled auto-merge (squash) August 12, 2024 07:38
@MichaReiser MichaReiser merged commit a99a458 into main Aug 12, 2024
17 checks passed
@MichaReiser MichaReiser deleted the eagerly-validate-search-paths branch August 12, 2024 07:47
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.

2 participants