Fix nondeterministic macro output

[dtolnay]

The automock attribute currently creates nondeterministic output, which breaks in environments that cache crate compilations, and causes unnecessary rebuilds of downstream code when the code shouldn't have changed.

Repro:

#[mockall::automock]
trait Trait {
    fn f<'a, 'b>(s: &'a &'b str);
}

Running cargo expand | rg "for<'., '.>" this shows mockall sometimes expanding to:

Mut(Box<dyn for<'b, 'a> FnMut(&'a &'b str) -> () + Send>),
MutSt(::mockall::Fragile<Box<dyn for<'b, 'a> FnMut(&'a &'b str) -> ()>>),
Once(Box<dyn for<'b, 'a> FnOnce(&'a &'b str) -> () + Send>),
OnceSt(::mockall::Fragile<Box<dyn for<'b, 'a> FnOnce(&'a &'b str) -> ()>>),
Func(Box<dyn for<'b, 'a> Fn(&&'b str) -> bool + Send>),
FuncSt(::mockall::Fragile<Box<dyn for<'b, 'a> Fn(&&'b str) -> bool>>),
Pred(Box<(Box<dyn for<'b, 'a> ::mockall::Predicate<&'b str> + Send>,)>),
fn with<MockallMatcher0: for<'b, 'a> ::mockall::Predicate<&'b str> + Send + 'static>(

and sometimes to:

Mut(Box<dyn for<'a, 'b> FnMut(&'a &'b str) -> () + Send>),
MutSt(::mockall::Fragile<Box<dyn for<'a, 'b> FnMut(&'a &'b str) -> ()>>),
Once(Box<dyn for<'a, 'b> FnOnce(&'a &'b str) -> () + Send>),
OnceSt(::mockall::Fragile<Box<dyn for<'a, 'b> FnOnce(&'a &'b str) -> ()>>),
Func(Box<dyn for<'a, 'b> Fn(&&'b str) -> bool + Send>),
FuncSt(::mockall::Fragile<Box<dyn for<'a, 'b> Fn(&&'b str) -> bool>>),
Pred(Box<(Box<dyn for<'a, 'b> ::mockall::Predicate<&'b str> + Send>,)>),
fn with<MockallMatcher0: for<'a, 'b> ::mockall::Predicate<&'b str> + Send + 'static>(

The source of the nondeterminism is iterating over a randomized HashSet.

This PR fixes the nondeterminism and uses https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_type to disallow iteration of hash based collections in the future.

[asomers]

Interesting find! I promise to review this by the weekend, but I may not have time until then.

[asomers]

While the problem is real, your solution seems excessively complicated. What's wrong with just using a deterministic HashMap/HashSet? They're only random because they use a random hasher by default. But they can be made deterministic by constructing one like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=420f28eeccedee9cd077fff59f831635 .

Also, this bug needs a regression test. I'll work on that.

[asomers]

If you check the "allow edits by maintainers" button, I'll push the test to the PR.

[dtolnay]

@asomers, I believe the complexity over your proposed alternative is justified because that's what makes this mistake machine catchable. Replacing all current places that create a randomized hashmap with with_hasher is not sufficient because every future code change would need to be inspected for this bug, and at least for myself as a reviewer I don't have confidence that I could keep all the issues requiring vigilance in my head.

[asomers]

That's what the test is for. Also, would your disallowed-types trick work for RandomState, as used indirectly by HashMap::default()?

[dtolnay]

That's what the test is for.

A test can (probabilistically) catch nondeterminism in a handful of automock invocations -- the ones covered by the test. Whereas banning HashMap rules out the entire category of HashMap-related nondeterminism across all possible automock invocations, including ones not well covered by the test suite.

I do agree with you that another test would be good, because a test could catch new sources of nondeterminism outside of HashMap. In practice that's less of a concern to me because I've debugged many of these nondeterministic compilation issues and it's been HashMap iteration every time, with the one exception of https://crates.io/crates/const-random which is blatant and intentional.

Also, would your disallowed-types trick work for RandomState, as used indirectly by HashMap::default()?

It does not catch this as far as I can tell. The mechanism here is the same as what is used for deprecations -- it's catching whether your code has named the thing. If the only place the word RandomState is written is in std code somewhere, disallowed-types would consider that std's fault, and not flag it in the downstream code which does not name RandomState.

[asomers]

Well, HashMap<K, V> is really just shorthand for HashMap<K, V, RandomState>. Is clippy smart enough to detect that? Or can it detect method calls like HashMap::new and HashMap::default? I'm just concerned about the size of your patch. I don't want to maintain a whole new collection type if I don't have to.

[dtolnay]

Well, HashMap<K, V> is really just shorthand for HashMap<K, V, RandomState>. Is clippy smart enough to detect that?

I think this isn't how the linter works. (But you could propose an enhancement to clippy. I don't know enough about the linter infrastructure to know how reasonable this would be to implement.)

HashMap<K, V> does not literally mention the type RandomState. It's the same for deprecation:

use std::marker::PhantomData;

#[deprecated]
pub struct Thing;

#[allow(deprecated)]
pub struct Generic<T = Thing>(PhantomData<T>);

fn main() {
    // does not mention "Thing", no warning
    let _g: Generic = Generic(PhantomData);
}

Or can it detect method calls like HashMap::new and HashMap::default?

There is a disallowed_method lint that you could use to flag HashMap::new. It can't flag HashMap::default unless you want to ban all of std::default::Default::default. But even beyond those, there are infinite ways to obtain a HashMap (e.g. think of deserializing it from JSON. The HashMap construction appears nowhere in your code.) I would have little confidence that a new one wouldn't sneak in over time.

I don't want to maintain a whole new collection type if I don't have to.

I think the reality is there isn't an appropriate collection type in std, so for a solution that involves banning iteration in collection order, you would either need it in this crate or you would need to add a dependency that provides it (which is an option if you are open to it). FWIW it's adding <1% to the Rust code already in this crate.

[asomers]

@dtolnay I ended up going the "HashMap with BuildHasherDefault" route. But I did make one new change: instead of a single test case for nondeterministic output, I added a --cfg option which checks for nondeterminism on every single invocation. Then I added a CI test step that builds the entire test suite with that option. I think that should pretty well cover it. Thanks again for raising this issue.

Superseded by #333 .