Skip to content

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

Merged
merged 2 commits into from
May 7, 2025

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Apr 11, 2025

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

@som-snytt som-snytt force-pushed the issue/22971-import-implicit-object branch from 5e5d2c9 to 44976dc Compare April 11, 2025 22:55
@som-snytt
Copy link
Contributor Author

Flaky cats test.

@som-snytt som-snytt force-pushed the issue/22971-import-implicit-object branch 2 times, most recently from 9802dd6 to dfc67f5 Compare April 17, 2025 17:26
@som-snytt som-snytt marked this pull request as ready for review April 25, 2025 20:22
@som-snytt
Copy link
Contributor Author

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.

@som-snytt som-snytt marked this pull request as draft April 28, 2025 21:22
@som-snytt som-snytt force-pushed the issue/22971-import-implicit-object branch from dfc67f5 to 8cbba06 Compare April 30, 2025 03:56
@som-snytt som-snytt changed the title Cache only when the context corresponds to prefix Remove premature caching of lookups for unused lint Apr 30, 2025
@som-snytt
Copy link
Contributor Author

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.

@som-snytt som-snytt force-pushed the issue/22971-import-implicit-object branch from 8cbba06 to 004e06d Compare April 30, 2025 14:56
@som-snytt
Copy link
Contributor Author

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.

@som-snytt som-snytt marked this pull request as ready for review April 30, 2025 23:36
@Gedochao Gedochao requested a review from noti0na1 May 6, 2025 09:56
Copy link
Member

@noti0na1 noti0na1 left a 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

@som-snytt som-snytt force-pushed the issue/22971-import-implicit-object branch from 004e06d to 876cc95 Compare May 6, 2025 15:43
@som-snytt som-snytt merged commit c5d5214 into scala:main May 7, 2025
29 checks passed
@som-snytt som-snytt deleted the issue/22971-import-implicit-object branch May 7, 2025 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive warning: unused import with certain combination of generics
2 participants