Enable Clippy's iter_over_hash_type lint#25732
Conversation
charliermarsh
changed the title
iter_over_hash_type lint
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 92.23%. The percentage of expected errors that received a diagnostic held steady at 87.42%. The number of fully passing files held steady at 92/134. |
Memory usage reportMemory usage unchanged ✅ |
charliermarsh
requested review from
AlexWaygood,
BurntSushi,
Gankra,
MichaReiser,
carljm,
dcreager,
dhruvmanila,
ibraheemdev and
sharkdp
as code owners
|
|
|
IIUC, I think there was a previous attempt at something similar using custom types in #21686 |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I'm fine with this, but I suggest we add a reason to every suppression explaining why it is okay, because it will now even be harder to find non determinism by hash map iteration because we simply assume; Oh, this is okay. A reason at least allows us to day: No, this reason is no longer true!
#[expect(clippy::iter_over_hash_type, reason = "we want to avoid these idents to be future compatible")]| let existing = existing_tests(&tests_dir)?; | ||
|
|
||
| let mut updated_files = vec![]; | ||
| let mut sorted_tests = tests.iter().collect::<Vec<_>>(); |
There was a problem hiding this comment.
Can you say more why it's important to sort here? Same below
| } | ||
|
|
||
| let mut normalized_aliases: FxHashMap<String, String> = FxHashMap::default(); | ||
| let mut aliases = aliases.into_iter().collect::<Vec<_>>(); |
There was a problem hiding this comment.
I'm not sure sorting here is required? It may mean that ruff's error message changes from run to run, but it should still always be an error? I don't mind it, but it feels a bit pedantic ;)
| @@ -1614,6 +1620,9 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { | |||
| &mut self, | |||
| nested_bindings: NestedGlobalOrNonlocalDeclarations, | |||
There was a problem hiding this comment.
Should this be a BTreeMap instead?
| let mut interned_ids_by_definition = | ||
| FxHashMap::with_capacity_and_hasher(definitions_by_definition.len(), FxBuildHasher); | ||
|
|
||
| let mut definitions_by_definition = |
There was a problem hiding this comment.
Could this be a BTreeMap instead?
|
Lol I literally backed out all the |
|
(I will re-add them.) |
charliermarsh
force-pushed
the
charlie/enable-iter-over-hash-type
branch
from
64b9213 to
ec1c351
Compare
charliermarsh
force-pushed
the
charlie/enable-iter-over-hash-type
branch
from
ec1c351 to
bcf904d
Compare
4 participants
Summary
Stacks on #25726 and enables Clippy's
iter_over_hash_typerestriction lint across the workspace, so new hash-map and hash-set iterations must make their ordering assumptions explicit.Most existing loops are order-independent and use a site-local
#[expect(clippy::iter_over_hash_type)]. At the order-sensitive sites, this sorts before iteration. (In practice, that should stabilize some parser fixtures, configuration diagnostics, union normalization, etc.).This lint is kind of annoying, but given how much we want to crack down on non-determinism in ty, it seems worthwhile to enable IMO.