-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[red-knot] Improve handling of visibility constraints in external modules when resolving *
imports
#17286
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
Conversation
|
7b02daf
to
06e8dc2
Compare
crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
Outdated
Show resolved
Hide resolved
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.
This is looking pretty good! Didn't do a full review, just commented on the main open issues I saw.
crates/red_knot_python_semantic/resources/mdtest/import/star.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/star.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/star.md
Outdated
Show resolved
Hide resolved
06e8dc2
to
4c6b6d1
Compare
Quite apart from any of the big things I was stuck on (thank you all for your review comments!), I realised after sleeping on it that I could significantly simplify things by reviving the idea I had previously in #16923 (comment). There's now no need for moving any logic from |
eeb09bf
to
3706dca
Compare
CodSpeed Performance ReportMerging #17286 will degrade performances by 12.49%Comparing Summary
Benchmarks breakdown
|
I tried out @sharkdp's suggested alternative implementation, and it appears to work beautifully... except that this PR is now triggering(?) a Salsa panic in some unrelated tests in a file I didn't touch:
which looks like the same Salsa panic that @dcreager was running into in #17023 before he applied ea12548 to that PR... |
534964f
to
93bc320
Compare
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.
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.
This is so nice and clean now! Love it.
I think given our upcoming deadlines I would be inclined to merge this even with a significant regression (especially if that regression gets smaller percentage-wise when we check a larger codebase), and prefer to have someone spend dedicated time later looking for performance wins. But it wouldn't hurt to at least look at some profiles (the CodSpeed ones are sadly useless, so they have to be generated locally) to understand which queries we spend more time in now.
crates/red_knot_python_semantic/src/semantic_index/predicate.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/semantic_index/predicate.rs
Outdated
Show resolved
Hide resolved
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.
Very nice — Thank you!
…ules when resolving `*` imports
… same visibility constraint
93bc320
to
6ec2191
Compare
crates/red_knot_python_semantic/src/semantic_index/predicate.rs
Outdated
Show resolved
Hide resolved
We discussed this internally. It's obviously far from ideal, but for now we're going to eat the performance regression. We have some ideas for how to address it, but they may or may not work out, and we want to move onto other tasks for now. I'll writeup a separate issue with our findings from looking at the tracing logs and various profiles. |
Co-authored-by: Carl Meyer <[email protected]>
* main: [red-knot] Allow explicit specialization of generic classes (#17023) [`airflow`] Refactor `AIR301` logic and fix typos (`AIR301`) (#17293) [`airflow`] Extract `AIR312` from `AIR302` rules (`AIR302`, `AIR312`) (#17152) [red-knot] Improve handling of visibility constraints in external modules when resolving `*` imports (#17286) [red-knot] Add more tests for `*` imports (#17315)
…ules when resolving `*` imports (astral-sh#17286)
Summary
Further work towards #14169.
This adjusts our handling of
*
imports so that we pay attention to visibility constraints in the external module (the module symbols are being imported from) when we infer the boundness of symbols defined via a*
import. I.e., for something like this:We will now correctly emit an
[unresolved-reference]
diagnostic on the use ofX
inb.py
: the visibility constraints ina.py
resolve toTruthiness::AlwaysFalse
, so no symbols are actually bound inb.py
as a result of the*
import.Implementation
When adding each
*
-import definition to theUseDefMap
, we snapshot theSymbolState
as it was prior to the*
import definition (possibly) overriding any (possible) previous definitions. Then, insymbol::symbol_from_bindings_impl
, we evaluate*
import definitions to determine the boundness of the symbol in the external module. If the symbol is in fact unbound in the external module (due to visibility constraints resolving toTruthiness::AlwaysFalse
), we determine that the*
import did not in fact provide an additional binding for this symbol, and in the importing module we fall back to theSymbolState
for that symbol prior to the*
-import definition.In order to be able to resolve
*
imports fromsymbol.rs
as well as fromtypes/infer.rs
, several functions are pulled out ofinfer.rs
into a newimport_resolution
module where they can be accessed from bothtypes/infer.rs
andsymbol.rs
.Problems with this approach
This approach works well for symbols in the external module that are statically known to either be definitely bound or definitely unbound. But the logic in
symbol.rs
is still somewhat naive, and doesn't work for possibly-unbound definitions in the external module. I've added a failing test in this PR to illustrate the issue: I don't know how to fix it properly, and would welcome ideas. In addition to the new failing test, the PR does not address any of these TODOs, and ideally it would:ruff/crates/red_knot_python_semantic/resources/mdtest/import/star.md
Lines 217 to 227 in 761749c
Test Plan
Existing assertions in mdtests updated; new ones added.