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

[red-knot] Resolve and check dependencies #11161

Merged
merged 2 commits into from Apr 27, 2024
Merged

Conversation

MichaReiser
Copy link
Member

Summary

Discover a modules dependencies and schedule them for checking.

This does not yet support native dependencies of which we don't have access to the source.

Test Plan

I created a test.py and ran red_knot test.py. The test.py imports match.py.

I can see from the logs that match.py is correctly scheduled for checking. Cyclic dependencies work too.
Red knot won't emit any diagnostics for match.py because it isn't an open file.

@MichaReiser MichaReiser changed the base branch from main to red-knot-program-check April 26, 2024 08:58
@MichaReiser MichaReiser requested a review from carljm April 26, 2024 08:58
Comment on lines -240 to -242
// WARNING!: It's important that this method takes `&mut self`. Without, the implementation is prone to race conditions.
// Note: This won't work with salsa because `Path` is not an ingredient.
pub fn path_to_module<Db>(db: &mut Db, path: &Path) -> Option<Module>
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 think that's actually fine. There's no risk for deadlocks because the function only ever holds on to a single lock (and not multiple ones). Any mutations happen through resolve_module which must be idempotent. The only overhead here is that we might end up calling resolve_module unnecessary but that's fine.

we could even consider not making this method a query. Searching the root paths is cheap. I think the only place where we safe some performance is that we avoid the canoncialize call. So that's why I think it's still worth keeping the cache for now.

Copy link

github-actions bot commented Apr 26, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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.

Some comments in here (the __init__.py thing for relative imports, and the level = Some(0) thing) that should definitely at least get TODO comments, if not fixed before merge.

crates/red_knot/src/module.rs Outdated Show resolved Hide resolved
crates/red_knot/src/program/check.rs Show resolved Hide resolved
crates/red_knot/src/program/check.rs Outdated Show resolved Hide resolved
crates/red_knot/src/program/check.rs Show resolved Hide resolved
crates/red_knot/src/symbols.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the red-knot-program-check branch 2 times, most recently from aca0d3d to 636ca68 Compare April 27, 2024 08:54
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 27, 2024
Base automatically changed from red-knot-program-check to main April 27, 2024 09:01
@MichaReiser MichaReiser enabled auto-merge (squash) April 27, 2024 15:41
@MichaReiser MichaReiser merged commit 983a06c into main Apr 27, 2024
18 checks passed
@MichaReiser MichaReiser deleted the red-knot-dependencies branch April 27, 2024 15:49
Comment on lines +999 to +1003
// `from baz import bar` in `foo/__init__.py` should resolve to `foo/baz.py`
assert_eq!(
Some(ModuleName::new("foo.baz")),
foo_module.relative_name(&db, 0, Some("baz"))
);
Copy link
Contributor

@carljm carljm Apr 27, 2024

Choose a reason for hiding this comment

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

This one isn't right, from baz import bar is not a relative import (no dots), it's an absolute import, so this should resolve to just baz.

Either relative_import should error on level = 0 (and we should never call it that way), or it should just resolve it as an absolute import.

Comment on lines +1020 to +1024
// `from baz import test` in `foo/bar.py` resolves to `foo.bar.baz`
assert_eq!(
Some(ModuleName::new("foo.bar.baz")),
bar_module.relative_name(&db, 0, Some("baz"))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is also wrong for the same reason mentioned above

crates/red_knot/src/module.rs Show resolved Hide resolved
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.

None yet

2 participants