Open anandijain opened 9 months ago
i think when you recurse though you pop need to think about (f (bns) (bns) (bs)) so when we enter (f a) | (f (bs)) we dont see [1]->(seq). but then you add [1]-> (seq a) then you see in that final that its good so you go back up. and you want the order to be so that the original is "first"/ applied first" (stack)
so you would have [1]-> [seq a, seq, seq] and pop + rebuild until empty
thinking about it more, the problem also arises with (f a) | (p x (bns)) (bs))
I think the changes required to fix are
so i think that in the pos map if you encounter a Vec that has length >1 then all but the last should be (Sequence) but im not 100% on that. but i cant think of a case where that is violated
the issue arises in that during incremental building we splice empty sequences into nothing, this causes a problem with position recording because our incremental solution reports the insertion [1]-> (Seq a) for (bs). when we pop to top level, we've re-added [1]-> (Seq) for (bns) which is "objectively" correct. meaning correct at top level.
i think that the solution is basically special casing empty sequences to not splice. just so that we avoid overwriting an empty bns matching.
i am not confident in that solution, id have to see how it works if you're actually trying to match sequences. also since the whole thing is that we have an equality check on final, at some point we need to splice empty. we dont really need to rely on equality. we could make our own equality check that ignores empty sequences
one thing to note
so MatchQ is not SequenceHold
walkthrough of proposed soln
(f a) | (f (bns) (bs))
another thought is that pos map can store a vec and instead of removing the key you pop then on final rebuild you apply
so then [1] -> vec![seq, (seq a)] so you would then rebuild (f (bns) (bs)) first to be (f (bs)) then you would see that [1] is not empty so you apply [1]-> (seq a)