Blind zippers restage

[luketpeterson]

This is a port of the changes in #25 onto master. This also addresses the GOATs in that branch.

We probably don't want to merge this until we're ready with the corresponding changes in MORK.

[luketpeterson]

Performance is roughly unchanged (equal proportion of gains vs. regressions)

[adamv-symbolica]

Looked through the code and mostly looks really neat!
I propose we make one change:
Keep descend_until with the current signature, and have it forward to
descend_until_watched(&mut self, &mut Obs) -> ().
This keeps both the flexibility of the new interface (but without the redundant return value) and the elegance of the old interface where we're in the simple case.

[adamv-symbolica]

Well, the focus_byte contract is ugly, but I don't immediately know how to do better.

[adamv-symbolica]

Maybe not something we need to address now, but descend_until_max_bytes is still a prototype (i.e. ascending back instead of respecting max), and it's pretty silly to let a prototype drive the interface.

[adamv-symbolica]

This may need to hash instead of a vec to keep it usable for debugging at scale. Also, obs could be given to wlog a since we panic on exit anyways. Maybe over-engineering though to do self.a.descend_until_watched(&mut tee(obs, hasher))

[luketpeterson]

Good idea. Added HashObserver type. It might be handy too.

[luketpeterson]

I propose we make one change: Keep descend_until with the current signature, and have it forward to descend_until_watched(&mut self, &mut Obs) -> (). This keeps both the flexibility of the new interface (but without the redundant return value) and the elegance of the old interface where we're in the simple case.

"unobserved" versions of the descend calls only make sense on zippers that have internal path buffers. Which was every zipper, but won't be if we permit this degree of freedom in the API. Without an internal path buffer, there is no way of knowing where descend_until took you to except through the observer.

The observed API needs to be the base case on ZipperMoving (no way around it), it's the unobserved convenience that is special and would need to be on ZipperPath.

If you know the zipper has a path buffer (or you don't care where you end up), you can pass &mut () and the runtime cost vanishes completely.

As to the return value being redundant, it is possible to coax the "did descend" signal out of the observer, but it would be a pain, just to reconstruct the information the function already has handy. Plus, having the return value for control flow lets you use empty PathObservers to good effect.

[luketpeterson]

Maybe not something we need to address now, but descend_until_max_bytes is still a prototype (i.e. ascending back instead of respecting max), and it's pretty silly to let a prototype drive the interface.

Prototype how? The default impl descends and re-ascends. But ReadZipperCore has a non-default impl that save that work of over-descending.

TruncatingObserver was included as a public utility because it is handy.

[imlvts]

This should not be in public interface.
Ok to use for testing, but it seems to me the promise is weird/broken.
We hash while descending, and reset on ascend, so semantically equivalent, but slightly different movements yield different results; while semantically different movements that end the same will yield the same hash. Weird.

[luketpeterson]

To me the bar for a public interface is: 1. Is it useful outside the library and 2. Are we comfortable maintaining it until the next API-breaking release.

To me this passes.

On the useful point, this allows a caller of a descend call to validate the zipper went where it was expected to go. It offers more information than depth counting while not requiring a buffer.

[adamv-symbolica]

returning ()

I see the effort of extracting it out argument.

&mut ()

is ugly, and I don't see why we can't have both: descend_until with current signature + descend_until_watched which is what has to be implemented on the trait.

[luketpeterson]

&mut () is ugly, and I don't see why we can't have both: descend_until with current signature + descend_until_watched which is what has to be implemented on the trait.

My bigger issue is about naming. Once this change is fully in, PathObservers are the common case - they show up in a lot more places than this one method. So giving the special calls the undecorated names and requiring decoration on the universal calls seems wrong.

We could declare a const like const NO_OBS: &'static mut () = &mut (); so you could pass NO_OBS, or something like that.

[Adam-Vandervorst]

In library code, yes, it'll be more common. But the interface convenience should be for the user, and that's -- with the current distribution of users -- going to degrade unless we make the change I'm suggesting.

[luketpeterson]

In library code, yes, it'll be more common. But the interface convenience should be for the user, and that's -- with the current distribution of users -- going to degrade unless we make the change I'm suggesting.

It's just a matter of naming. The observed methods should be named consistently. We need a better name!

candidates: _tracked, _observed, _watched, _obs, any others?

_obs is short but ungrammatical
_observed is a pain to type a lot
_tracked and _watched aren't much better and they imply the PathObserver should be renamed. (Maybe it should)

[Adam-Vandervorst]

PathTracker kind of works, but _observed is completely fine with me also