Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jul 1, 2025

Summary

I hoped this might fix the latest stack overflows on #18659... it doesn't look like it does, but these changes seem like they're probably correct anyway...?

Test Plan

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Jul 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2025

mypy_primer results

Changes were detected when running on open source projects
jinja (https://github.com/pallets/jinja)
- TOTAL MEMORY USAGE: ~106MB
+ TOTAL MEMORY USAGE: ~97MB

black (https://github.com/psf/black)
- TOTAL MEMORY USAGE: ~129MB
+ TOTAL MEMORY USAGE: ~142MB

alerta (https://github.com/alerta/alerta)
- TOTAL MEMORY USAGE: ~106MB
+ TOTAL MEMORY USAGE: ~117MB

nox (https://github.com/wntrblm/nox)
-     memo fields = ~72MB
+     memo fields = ~66MB

mkosi (https://github.com/systemd/mkosi)
- TOTAL MEMORY USAGE: ~117MB
+ TOTAL MEMORY USAGE: ~129MB
-     memo fields = ~97MB
+     memo fields = ~106MB

tornado (https://github.com/tornadoweb/tornado)
-     memo fields = ~142MB
+     memo fields = ~129MB

paasta (https://github.com/yelp/paasta)
-     memo fields = ~171MB
+     memo fields = ~156MB

altair (https://github.com/vega/altair)
-     memo fields = ~251MB
+     memo fields = ~228MB

rotki (https://github.com/rotki/rotki)
-     memo fields = ~445MB
+     memo fields = ~490MB

scikit-learn (https://github.com/scikit-learn/scikit-learn)
- TOTAL MEMORY USAGE: ~717MB
+ TOTAL MEMORY USAGE: ~652MB

@AlexWaygood AlexWaygood marked this pull request as ready for review July 1, 2025 16:21
@AlexWaygood AlexWaygood added the internal An internal refactor or improvement label Jul 1, 2025
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

.prenormalized_suffix_elements(db, None)
.map(|ty| ty.normalized_impl(db, visitor))
.collect::<Vec<_>>();
Self::mixed(prefix, self.variable.normalized(db), suffix)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the thing I'm least happy with in the current approach, is that it's so easy to just call .normalized(db) on a nested type when it should be .normalized_impl(db, visitor). But I haven't come up with any good ideas for preventing that mistake, because Type::normalized does need to exist, for "external" use -- it just shouldn't be used inside another normalized_impl method.

@AlexWaygood AlexWaygood merged commit 316c1b2 into main Jul 1, 2025
38 of 39 checks passed
@AlexWaygood AlexWaygood deleted the alex/normalized-impl branch July 1, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants