selectors: fix for edge case around recursion clauses with an immediate edge. #334
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Seealso matching specs PR: ipld/ipld#172 . Should fix ipni/index-provider#144 . |
mvdan
reviewed
Jan 19, 2022
…te edge. See also the diff in the ipld/ipld repo for the new fixture, which includes some explanation. This would be a somewhat silly selector to write, but, it doesn't seem to be something we should reject at compile time, either; so, we must handle it gracefully. Along the way, I documented some code that was sparse in comments. I also added a shortcut to traversals so that a selector that explicitly states it's not interested in anything (by returning a non-nil but empty slice for its interests) is treated differently than a nil return for interests (which means "I don't know; hit me with everything you've got and lemme see"). This means that our edge case here with edges (...heh) doesn't cause the traversal to create an iterator that it doesn't really need, etc. We turn out to need both that shortcut, *and* less panicky methods on ExploreRecursiveEdge, *and* the edge case branch in ExploreRecursive... because traversals are pre-order. Decide is called first, then Interests, and then Explore. Therefore informing the Explore method alone about this situation is not sufficient. Previously, a recursion clause with an edge as its immediate and only child would get a passing grade by the compiler (same as now), but would panic when actually used (oh dear).
warpfork
force-pushed
the
selectors-fix-recursion-with-immediate-edge
branch
from
3dfd9e6
to
9e5da0e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See also the diff in the ipld/ipld repo for the new fixture,
which includes some explanation.
This would be a somewhat silly selector to write, but,
it doesn't seem to be something we should reject at compile time, either;
so, we must handle it gracefully.
Along the way, I documented some code that was sparse in comments.
I also added a shortcut to traversals so that a selector that
explicitly states it's not interested in anything (by returning a
non-nil but empty slice for its interests) is treated differently
than a nil return for interests (which means "I don't know; hit me
with everything you've got and lemme see"). This means that our
edge case here with edges (...heh) doesn't cause the traversal
to create an iterator that it doesn't really need, etc.
We turn out to need both that shortcut, and less panicky methods on
ExploreRecursiveEdge, and the edge case branch in ExploreRecursive...
because traversals are pre-order. Decide is called first, then
Interests, and then Explore. Therefore informing the Explore method
alone about this situation is not sufficient.
Previously, a recursion clause with an edge as its immediate and only
child would get a passing grade by the compiler (same as now),
but would panic when actually used (oh dear).