-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove premature caching of lookups for unused lint #22982
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
Remove premature caching of lookups for unused lint #22982
Conversation
5e5d2c9
to
44976dc
Compare
Flaky cats test. |
9802dd6
to
dfc67f5
Compare
This deals with the second-hardest problem after naming, but also the performance of the lint requires review, since this is an example of premature caching without benchmarking. It might be preferable to ignore caching (for the sake of correctness) and only then do a review of performance. That also entails a review of architecture and whether mini-phase really serves the use case. |
dfc67f5
to
8cbba06
Compare
As coded, the cache of lookups at contexts doesn't pull its weight, either provably or demonstrably. This commit removes it for robustness. (The first swing tweaked a couple of conditions that made the test pass.) The phase still needs a review of performance. |
8cbba06
to
004e06d
Compare
Took me a while to see that the REPL wrapper test needed adjustment because there is no longer a fresh context "injected" to house the cache apparatus. |
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.
LGTM, just need to rebase to main
004e06d
to
876cc95
Compare
Caching a lookup of c in a.b.c is ok at a.b.
A lookup from a.p should not see the cached value.
Fixes #22971