-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] per-definition inference, use-def maps #12269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand all of it yet, but I like what I'm seeing.
let Some(first) = def_types.next() else { | ||
return Type::Unbound; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let Some(first) = def_types.next() else { | |
return Type::Unbound; | |
}; | |
let first = def_types.next().unwrap_or(Type::Unbound); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better even though it means we'll make an entirely unnecessary second call to def_types.next()
that we already know will return None
? It seems like we may as well short-circuit here and just return Unbound directly.
524f359
to
3cbbd50
Compare
|
CodSpeed Performance ReportMerging #12269 will improve performances by 6.95%Comparing Summary
Benchmarks breakdown
|
This is ready for review. I've updated the PR description with a fuller narrative of what's in here and why. Benchmarks seem to say this PR has reclaimed most of the perf that was lost in the Salsa-type-interning PR, but I'm not sure why. It may be due to the removal of the ancestor-scope-walking name lookup code in favor of the use-def map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what I'm seeing here.
My only concern is that I don't understand how the different type inference scopes are merged. See my inline comment.
It will be interesting to see what the cost of storing the definition -> Ty
and expression -> Ty
hash maps on every level. That's a lot of hash maps and a lot of redundant information that we need to keep around.
Yeah :/ That's the part of this approach that I'm least happy about. We could reduce the number of hashmaps, and the redundancy, by using some kind of chaining (where an outer I'll think more about this. |
Thanks for the great review! |
Here's an idea that's not in the salsa spirit but upholds the important constraints that matter to salsa.
This approach would mitigate most concerns. The only extra cost is that we need to test for every expression if it has already been inferred. I'm unclear on how it would work with any fixpoint iteration. How would we know which expression types need to be invalidated? Or would we know if we run a fixpoint and could then forcefully re-infer all expression types? Regarding the review: Please re-request review when you think I should have another look |
2ab2e96
to
f589b4b
Compare
Ok, I think this is ready for review again. The use-def map building is now better (uses a single vec of definitions and ranges, instead of vecs of vecs) but there are probably ways it could be still better; open to comments. The main complexity is that avoiding vecs-of-vecs means sticking to a single range for the "active" defs of a given symbol, which means we always have to keep all the active defs for a given symbol adjacent. Sometimes at control-flow merge points this requires copying the same definition IDs from earlier to later in the all-definitions vector. I ended up adding basic control flow (just for if statements and expressions), because it makes a big difference in how the range-based use-def map building works.
That's a neat idea. I would like to hold off on it though; it's purely a performance optimization that shouldn't make a difference to the API for type inference. So I would rather leave it until we are at a point where we can evaluate the performance impact on real code. For now I think we can move forward with some duplicated stored types and keep things more salsa-native. It's just extra id -> id mappings; not a ton of wasted memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type inference part looks good to me.
I started (for 1.5h) reviewing the use-def implementation but I must admit that I don't have enough context information to efficiently review the changes and the data structures are non-trivial. I would have preferred if adding use-def and then refactoring to a more efficient data structure were its own PR with a summary explaining how the data structures work together and what considerations have been made. But it's a bit too late for that. So I'm just gonna skip this part. We can come back to it later or set up a session where you walk me through the code.
Planning to merge this as-is once tests pass. All unresolved comments above will be addressed in follow-up PRs. |
Per comments in #12269, "module global" is kind of long, and arguably redundant. I tried just using "module" but there were too many cases where I felt this was ambiguous. I like the way "global" works out better, though it does require an understanding that in Python "global" generally means "module global" not "globally global" (though in a sense module globals are also globally global since modules are singletons).
Implements definition-level type inference, with basic control flow (only if statements and if expressions so far) in Salsa.
There are a couple key ideas here:
We can do type inference queries at any of three region granularities: an entire scope, a single definition, or a single expression. These are represented by the
InferenceRegion
enum, and the entry points are the salsa queriesinfer_scope_types
,infer_definition_types
, andinfer_expression_types
. Generally per-scope will be used for scopes that we are directly checking and per-definition will be used anytime we are looking up symbol types from another module/scope. Per-expression should be uncommon: used only for the RHS of an unpacking or multi-target assignment (to avoid re-inferring the RHS once per symbol defined in the assignment) and for test nodes in type narrowing (e.g. thetest
of anIf
node). All three queries return aTypeInference
with a map of types for all definitions and expressions within their region. If you do e.g. scope-level inference, when it hits a definition, or an independently-inferable expression, it should use the relevant query (which may already be cached) to get all types within the smaller region. This avoids double-inferring smaller regions, even though larger regions encompass smaller ones.Instead of building a control-flow graph and lazily traversing it to find definitions which reach a use of a name (which is O(n^2) in the worst case), instead semantic indexing builds a use-def map, where every use of a name knows which definitions can reach that use. We also no longer track all definitions of a symbol in the symbol itself; instead the use-def map also records which defs remain visible at the end of the scope, and considers these the publicly-visible definitions of the symbol (see below).
Major items left as TODOs in this PR, to be done in follow-up PRs:
Free/global references aren't supported yet (only lookup based on definitions in current scope), which means the override-check example doesn't currently work. This is the first thing I'll fix as follow-up to this PR.
Control flow outside of if statements and expressions.
Type narrowing.
There are also some smaller relevant changes here:
Eliminate
Option
in the return type of member lookups; instead always returnType::Unbound
for a name we can't find. Also useType::Unbound
for modules we can't resolve (not 100% sure about this one yet.)Eliminate the use of the terms "public" and "root" to refer to module-global scope or symbols. Instead consistently use the term "module-global". It's longer, but it's the clearest, and the most consistent with typical Python terminology. In particular I don't like "public" for this use because it has other implications around author intent (is an underscore-prefixed module-global symbol "public"?). And "root" is just not commonly used for this in Python.
Eliminate the
PublicSymbol
Salsa ingredient. Many non-module-global symbols can also be seen from other scopes (e.g. by a free var in a nested scope, or by class attribute access), and thus need to have a "public type" (that is, the type not as seen from a particular use in the control flow of the same scope, but the type as seen from some other scope.) So all symbols need to have a "public type" (here I want to keep the use of the term "public", unless someone has a better term to suggest -- since it's "public type of a symbol" and not "public symbol" the confusion with e.g. initial underscores is less of an issue.) At least initially, I would like to try not having special handling for module-global symbols vs other symbols.Switch to using "definitions that reach end of scope" rather than "all definitions" in determining the public type of a symbol. I'm convinced that in general this is the right way to go. We may want to refine this further in future for some free-variable cases, but it can be changed purely by making changes to the building of the use-def map (the
public_definitions
index in it), without affecting any other code. One consequence of combining this with no control-flow support (just last-definition-wins) is that some inference tests now give more wrong-looking results; I left TODO comments on these tests to fix them when control flow is added.And some potential areas for consideration in the future:
symbol_ty
be a Salsa query? This would require making all symbols a Salsa ingredient, and tracking even more dependencies. But it would save some repeated reconstruction of unions, for symbols with multiple public definitions. For now I'm not making it a query, but open to changing this in future with actual perf evidence that it's better.