Accumulator misses accumulated values when behind tracked fn

[NickNick]

In 3c230f8 I modified the existing test 'accumulate-from-tracked-fn.rs' to demonstrate. After setting l1 to 11, the value accumulated by compute_single on l0 is missing.

[MichaReiser]

I think that's expected, sort of. The idea of accumulated values is that you call them outside of queries. and Salsa will automatically identify all the accumulated values relevant in that query tree.

But there might still be something wrong here. But that's what I remember from when I used accumulators the last time (Neither we nor r-a are using them)

[MichaReiser]

Never mind. I read your example wrong. Let me take another look.

[MichaReiser]

This looks like a bug in maybe_changed_after. We probably don't set InputAccumulatedValues correctly

[MichaReiser]

Yes that's it.

You can comment out

https://github.com/MichaReiser/salsa/blob/87a730fccf07add73aabc6f2c842d92fd2bfe466/src/function/accumulated.rs#L62-64

(and change ingredient.origin to return None by default) and this fixes the bug.

What that means is, that we aren't correctly aggregating whether some query subtree has any accumulated values. I currently don't have the time to debug this. But you want to debug where we return InputAccumulatedValues and we'll return Empty somewhere where it should be Any instead.

[MichaReiser]

I think the fix here is to add the following here https://github.com/MichaReiser/salsa/blob/87a730fccf07add73aabc6f2c842d92fd2bfe466/src/function/maybe_changed_after.rs#L520

                if old_memo
                    .revisions
                    .accumulated()
                    .is_some()
                {
                    inputs = InputAccumulatedValues::Any;
                }

Because inputs must be Any if any query dependency OR the query itself has any accumulated values.

@NickNick do you want to submit a PR with a test for the fix

[NickNick]

Sure! Thanks for debugging this so quickly. I've opened #924 with the modified test. I think it also still tests what the old test did, but I can also add a test instead of modifying the existing one.

[MichaReiser]

I think it's best to have a new test for it because it tests a different usage pattern

[NickNick]

Of course, done!