red-knot(Salsa): Types without refinements#11899
Conversation
4f74692 to
cd4c5df
Compare
|
471d853 to
915b793
Compare
cef51c3 to
e9150a3
Compare
carljm
left a comment
There was a problem hiding this comment.
On the whole, this looks really good to me. A lot of things are cleaner and clearer than in the previous version. I am definitely learning to write better Rust from seeing how you rewrite my code :)
I think the most concerning comment here (in the sense of "might have significant implications for needing to revisit this architecture") is the one about cycles and the need to support multiple partially-completed scopes at once.
crates/ruff_python_semantic/src/red_knot/semantic_index/builder.rs
Outdated
Show resolved
Hide resolved
crates/ruff_python_semantic/src/red_knot/semantic_index/symbol.rs
Outdated
Show resolved
Hide resolved
crates/ruff_python_semantic/src/red_knot/semantic_index/symbol.rs
Outdated
Show resolved
Hide resolved
crates/ruff_python_semantic/src/red_knot/semantic_index/symbol.rs
Outdated
Show resolved
Hide resolved
| class Base: | ||
| pass | ||
|
|
||
| class Sub(Base): | ||
| pass"#, |
There was a problem hiding this comment.
Per my follow-up comment on the symbol table PR, I'm not convinced by the text-offsets argument, and I'd still rather use dedent in tests (unless we don't need it? see below) instead of this style of indentation for multi-line code in tests.
There was a problem hiding this comment.
I'm not convinced by the text-offsets argument,
Wait until you have to debug some more byte offset errors, but maybe it's different in the type checker context. Debugging byte offsets was something I did frequently when working on the formatter.
I personally find both about equally ugly because rustfmt can't format them for you. But I'm not sure if I like having dedent calls in all tests.
There was a problem hiding this comment.
I wouldn't want dedent calls in every test. I'd want helpers for writing file contents in tests with less boilerplate, which would also call dedent on the given source.
And yeah, I think caring about byte offsets will be a lot less common in these tests than in parser or formatter tests.
|
I haven't yet found a solution for the cyclic import problem but Carl and I discussed that we don't think that this should block this PR. It seems there are a few different solutions to it, but they all require extending Salsa's functionality. See https://salsa.zulipchat.com/#narrow/stream/145099-general/topic/Handling.20cyclic.20imports Let's solve the |
Summary
This PR ports the type inference code from red-knot to a salsa based implementation.
The biggest difference to the implementation in red-knot is that the type inference is per scope and not per expression, similar to how we build symbol tables.
This PR adds automatic type invalidation when types of dependency changes; this is a functionality that the implementation in the red knot crate doesn't support today.
Not yet ported
This PR does not yet implement type narrowing based on the control flow graph because the salsa version doesn't have a control flow graph yet.
Tests
I ported the existing tests of the red-knot code base that aren't context sensitive. I added a few new tests that validate that local changes don't trigger cross-file invalidation.