Fix tests on 32-bit architectures #19652
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Surprisingly, I don't think the mypy_primer failure is my fault. I tried |
ntBre
added
testing
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you for looking into this!
The goto reference changes look good to me. I'm less sure if this is the right place (it seems unfortunate to pay the overhead of an ordered map to fix a test only issue). I'd appreciate if @AlexWaygood could take a look at that code
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
This makes sense to me. I doubt that this method will be very hot (attribute access should be cached, and we only enter it for __getitem__ attribute access on tuple classes), so I don't think switching to an FxOrderMap (or FxIndexMap) is likely to significantly impact performance here
| let mut element_type_to_indices: FxOrderMap<Type<'db>, Vec<i64>> = | ||
| FxOrderMap::default(); |
There was a problem hiding this comment.
tiny nit: I think an FxIndexMap would suffice here, because we shouldn't need to hash it anywhere
There was a problem hiding this comment.
Ah, makes sense! I had that first and then noticed FxOrderMap was already in scope 😆 I'll switch back
There was a problem hiding this comment.
yeah, I have no idea why FxOrderMap is defined in this submodule rather than in lib.rs with our other Fx* aliases! It should probably be moved
I merged a workaround for this in #19659 for now |
Summary -- Fixes #19640. I'm not sure these are the exact fixes we really want, but I reproduced the issue in a 32-bit Docker container and tracked down the causes, so I figured I'd open a PR. As I commented on the issue, the `goto_references` test depends on the iteration order of the files in an `FxHashSet` in `Indexed`. In this case, we can just sort the output in test code. Similarly, the tuple case depended on the order of overloads inserted in an `FxHashMap`. `FxOrderMap` seemed like a convenient drop-in replacement, but I don't know if that will have other detrimental effects. I did have to change the assertion for the tuple test, but I think it should now be stable across architectures. Test Plan -- Running the tests in the aforementioned Docker container
ntBre
force-pushed
the
brent/fix-32bit-tests
branch
from
b9d987e to
ec52ec5
Compare
|
* main: (39 commits) [ty] Initial test suite for `TypedDict` (#19686) [ty] Improve debuggability of protocol types (#19662) [ty] Simplify lifetime requirements for `PySlice` trait (#19687) [ty] Improve `isinstance()` truthiness analysis for generic types (#19668) [`refurb`] Make example error out-of-the-box (`FURB164`) (#19673) Fix link: unused_import.rs (#19648) [ty] Remove `Specialization::display` (full) (#19682) [ty] Remove `KnownModule::is_enum` (#19681) [ty] Support `__setitem__` and improve `__getitem__` related diagnostics (#19578) [ty] Sync vendored typeshed stubs (#19676) [`flake8-use-pathlib`] Expand `PTH201` to check all `PurePath` subclasses (#19440) [`refurb`] Make example error out-of-the-box (`FURB180`) (#19672) [`pyupgrade`] Prevent infinite loop with `I002` (`UP010`, `UP035`) (#19413) [ty] Improve the `Display` for generic `type[]` types (#19667) [ty] Refactor `TypeInferenceBuilder::infer_subscript_expression_types` (#19658) Fix tests on 32-bit architectures (#19652) [ty] Move `pandas-stubs` to bad.txt (#19659) [ty] Remove special casing for string-literal-in-tuple `__contains__` (#19642) Update pre-commit's `ruff` id (#19654) Update salsa (#19449) ...
Summary
Fixes #19640. I'm not sure these are the exact fixes we really want, but I
reproduced the issue in a 32-bit Docker container and tracked down the causes,
so I figured I'd open a PR.
As I commented on the issue, the
goto_referencestest depends on the iterationorder of the files in an
FxHashSetinIndexed. In this case, we can justsort the output in test code.
Similarly, the tuple case depended on the order of overloads inserted in an
FxHashMap.FxIndexMapseemed like a convenient drop-in replacement, but Idon't know if that will have other detrimental effects. I did have to change the
assertion for the tuple test, but I think it should now be stable across
architectures.
Test Plan
Running the tests in the aforementioned Docker container