Fix operator precedence in ProvisionalEvaluationCache::on_failure - #80
Draft
xmakro wants to merge 1 commit into
Draft
Fix operator precedence in ProvisionalEvaluationCache::on_failure#80xmakro wants to merge 1 commit into
xmakro wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
on_failureis meant to discard the provisional cache entries created since the given depth-first number.on_completiondocuments the rule with concrete numbers: withdfnof 2 for the C node, the entry for D at DFN 3 is removed while A and B at DFNs 0 and 1 are kept.next_dfnis a plain increasing counter, so entries created while a node is on the stack have the higher numbers, andinsert_provisionaluses the same>= from_dfntest to find the entries it created.on_failureshould therefore removefrom_dfn >= dfn.The condition is currently written
!eval.from_dfn >= dfn, where!is a bitwise negation of a usize rather than a logical one, so it evaluates asusize::MAX - from_dfn >= dfnand holds for any realistic value. Every entry is dropped whenever an obligation fails.The history is worth spelling out, because the obvious reading of the typo gives the wrong fix. Before 59f5045 ("add more debug logs") this was a retain predicate,
retain(|_key, eval| eval.from_dfn >= dfn), which keptfrom_dfn >= dfnand dropped everything older. Expanding a retain predicate into an if/else that returns false to remove requires negating it, so the author wanted!(eval.from_dfn >= dfn)and the parenthesis is what went missing. But restoring that would restore a predicate that was already inverted with respect to both doc comments: it keeps precisely the entries that may depend on the failing node and discards the independent ones. The stray!turned an inverted predicate into an unconditional clear, which is wasteful but conservative, and that is presumably why nothing ever misbehaved.So this is not a restoration of intended behaviour. The documented behaviour appears never to have been implemented. That also explains why correcting it is not a performance win: I measured no significant change on syn, serde, hyper and ripgrep, so nothing has been tuned around the cache working. Taking it as a correctness and clarity fix, it retains more entries than the current code does and wants the usual test coverage on that basis.