Re-work indirect dependencies #19798
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
OK, I updated the PR to handle other issues mentioned in #933 as well. FWIW it actually looks more elegant/simple now. I will now try to minimize the performance impact if possible. |
|
I made one more change, now I am adding only actual module as a dependency, not all its parents. Not sure why this was needed, as no tests failed when I removed them. IMO this is conceptually correct: indirect dependency, is a dependency on the place where an actual symbol is defined. Also it terms of performance I measure ~0.6% slow down, which is IMO totally acceptable. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Nice, the original logic always seemed a little sketchy. I agree that the performance regression is worth it, and I can't think of a way to do this more efficiently while retaining correctness. It's important to get this right or we risk random false positives and false negatives once we support parallel type checking.
This makes self-check almost 2% faster on my desktop (Python 3.12, compiled -O2). Inspired by a slight regression in #19798 I decided to re-think how we detect/label the recursive types. The new algorithm is not 100% equivalent to old one, but should be much faster. The main semantic difference is this: ```python A = list[B1] B1 = list[B2] B2 = list[B1] ``` previously all three aliases where labeled as recursive, now only last two are. Which is kind of correct if you think about it for some time, there is nothing genuinely recursive in `A` by itself. As a result: * We have somewhat more verbose `reveal_type()` for recursive types after fine-grained increments. Excessive use of `get_proper_type()` in the daemon code is a known issue. I will take a look at it when I will have a chance. * I cleaned up some of relevant visitors to be more consistent with recursive aliases. * I also do couple cleanups/speedups in the type queries while I am at it. If there are no comments/objections, I will merge it later today. Then I will merge #19798, and then _maybe_ an equivalent optimization for recursive instances like `class str(Sequence[str]): ...`
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
SCC construction should only consider import dependencies, since indirect dependencies are not available during non-incremental runs. I think otherwise SCCs can be different in non-incremental vs incremtal runs. Attempt to fix an issue with excessively large SCCs computed (sometimes) after #19798 when doing incremental runs. I haven't been able to create a self-contained test case that reproduces the issue, but this appears to fix this issue with an internal codebase.
2 participants
Wow, this was quite a ride. Indirect dependencies were always supported kind of on best effort. This PR puts them on some principled foundation. It fixes three crashes and three stale types reported. All tests are quite weird/obscure, they are designed to expose the flaws in current logic (plus one test that passes on master, but it covers important corner case, so I add it just in case ). A short summary of various fixes (in arbitrary order):
module_refslogic in type checker with more principled one during semantic analysisqualified_tvarsas a concept, they are not needed since long agoTypeInfos frombuild.pyIn general the logic should be more simple/straightforward now:
get_proper_type()calls).Note since this makes the algorithm correct, it may also make it slower (most notably because we must visit generic bases). I tried to offset this by couple optimizations, hopefully performance impact will be minimal. On my machine slow down is ~0.6%