Skip to content
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] gather type prevalence statistics #15834

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[red-knot] gather type prevalence statistics #15834

wants to merge 2 commits into from

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Jan 30, 2025

Something Alex and I threw together during our 1:1 this morning. Allows us to collect statistics on the prevalence of various types in a file, most usefully TODO types or other dynamic types.

@carljm carljm added the red-knot Multi-file analysis & type inference label Jan 30, 2025
@carljm
Copy link
Contributor Author

carljm commented Jan 30, 2025

Hmm, the test failure here is very odd! Test definitely passes locally. Will have to look into this later.

@AlexWaygood
Copy link
Member

that is really weird. it passes for me locally too (and it's obviously passing on all but one of our CI jobs).

That said, the statistic given by the one job that's failing is a statistic that makes more sense intuitively to me...

@carljm
Copy link
Contributor Author

carljm commented Jan 31, 2025

The issue is that in a release build we (intentionally, for perf reasons) omit tracking of distinct messages / file-and-line information for Todo types. So this results in fewer different kinds of Todo types in a release build. Then this interacts with another bug, which is that we are using FxHashMap::extend wrongly to combine statistics from different scopes (this won't sum the totals when the key exists in both maps). The latter bug only shows up if the same type occurs in more than one scope, and that only occurs in our tests when running in release mode so that all Todo types are the same.

@carljm
Copy link
Contributor Author

carljm commented Jan 31, 2025

Pushed a commit that fixes statistics merging, and fixes the tests.

It remains the case that we can't add any tests specifically testing that the statistics for "different" Todo types are differentiated, or those tests will fail on a release build. (We could switch any such tests off in release build.) We can still build features making use of this differentiation, but they will only work in debug build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants