Fix accumulator only accumulating direct children

[PhoebeSzmucer]

I noticed that some of the accumulated values were getting dropped, and I traced it to the accumulated_by function only accumulating from direct children.

Let me know if this fix is any good - I'm quite new to Rust and this codebase, and I wasn't sure how to best implement this recursion.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	a85ac26
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/669eaf3fc07f2c00085609da

[nikomatsakis]

Good catch! Silly me. This looks like a good fix, but I have a suggested rewrite.

[nikomatsakis]

Rather than this recursive function let's just inline a loop here...

    let db_key = self.database_key_index(key);
    let mut visited: FxHashSet<DatabaseKeyIndex> = std::iter::once(db_key).collect();
    let mut stack = vec![db_key];

    while let Some(k) = stack.pop() {
        accumulator.produced_by(runtime, k, &mut output);
        for each successor s {
            if visited.insert(s) {
                stack.push(s);
            }
        }
    }
}  

Reasons:

• Overall simpler, less generics, etc
• Adding public methods, particularly those that can be called from outside the crate, is something to do with caution
• Using FxHashSet is better than just plain old HashSet if you don't need DOS protection -- more efficient
• Using a vector to store the stack avoids risk of stack overflow

[PhoebeSzmucer]

Ah the pub was a mistake, I didn't mean to expose it outside of the crate. I'll rewrite this using your suggestion - it's much cleaner :)

One question though - I think stack will change the accumulated diagnostic order. Do we care? I could also use a queue.

[PhoebeSzmucer]

Ok I pushed a refactor. This did change the order of accumulated values in some tests - let me know if I should use a queue instead.

[nikomatsakis]

You're right. Let's use a queue.

[PhoebeSzmucer]

Updated. Let me know if there is another preferred queue struct.

[nikomatsakis]

Or not, see my comment below.

[PhoebeSzmucer]

Ok one more try - thanks for being patient with me.

I pushed your suggestion of pushing children in reverse order onto a stack. This required me to change Iterator to DoubleEndedIterator in a few places - let me know if that's ok.

I can also go back to a recursion if you prefer that.

[nikomatsakis]

Hmm. Ideally we would accumulate in execution order, I think. A queue won't really do it, that will give us a breadth-first search. I think what we want to do is to push the children onto the stack but in reverse order -- something like this. But you also have to remove the hashset insertion I think and instead do the insertion when you pop off the stack. Otherwise this case would print wrong:

A {
    B {
        D
    }
    C,
    D,
}

it would print like A, B, C, D instead of A, B, D, C.

Or you could go back to a recursive function (I wouldn't bother with the generic method, I'd just pull out a simple helper function). I think that would be fine, especially since the recursion depth is reflecting an execution that already took place. =)

[nikomatsakis]

I like it! Left a suggestion for a comment.

[PhoebeSzmucer]

@nikomatsakis is the failure a flake?

[nikomatsakis]

Yeah, the miri failure is #520

[nikomatsakis]

I was thinking @PhoebeSzmucer that it'd be good to add a test for one of the more complex scenarios -- e.g. to test duplicates. Are you up to add something like that?

[PhoebeSzmucer]

@nikomatsakis yes if be happy to add more tests.

I think there are accumulator tests that indirectly test for the lack of duplicates already (the accumulator-dag one, accumulator-execution-order one that I added here), but I can add some more.

[PhoebeSzmucer]

@nikomatsakis I added one more complex test here a85ac26

[nikomatsakis]

@PhoebeSzmucer nice! Approved.