fix: nondeterministic crash in grind congruence closure (#13027)
This PR fixes a nondeterministic crash in `grind` caused by a `BEq`/`Hashable` invariant violation in the congruence table. `congrHash` uses each expression's own `funCC` flag to compute its hash (one-level decomposition for `funCC = true`, full recursive decomposition for `funCC = false`), but `isCongruent` only checked the stored expression's flag. When two expressions with mismatched `funCC` flags accidentally hash-collided (via pointer-based `ptrAddrUnsafe` hashing), `isCongruent` could declare them congruent despite different argument counts, leading to an assertion failure in `mkCongrProof`. The fix requires matching `funCC` flags in `isCongruent`. The PR also fixes the debug invariant checker (`checkParents`) to skip `funCC` parents and adds a regression test for funCC congruence. Observed as a nondeterministic crash in Mathlib at `Analysis/ODE/PicardLindelof.lean`. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
1cf030c5d4
commit
9fa1a252f2
3 changed files with 66 additions and 26 deletions
|
|
@ -63,32 +63,34 @@ def checkMatchCondParent (e : Expr) (parent : Expr) : GoalM Bool := do
|
|||
return false
|
||||
|
||||
def checkParents (e : Expr) : GoalM Unit := do
|
||||
-- **Note**: We currently do not check the `funCC` case
|
||||
unless (← useFunCC e) do
|
||||
if (← isRoot e) then
|
||||
for parent in (← getParents e).elems do
|
||||
if isMatchCond parent then
|
||||
unless (← checkMatchCondParent e parent) do
|
||||
-- **Note**: We skip `funCC` parents because the parent-child relationship uses
|
||||
-- one-level decomposition (`appFn!`/`appArg!`) rather than full `getAppArgs`/`getAppFn`.
|
||||
if (← isRoot e) then
|
||||
for parent in (← getParents e).elems do
|
||||
if parent.isApp && (← useFunCC parent) then
|
||||
pure ()
|
||||
else if isMatchCond parent then
|
||||
unless (← checkMatchCondParent e parent) do
|
||||
throwError "e: {e}, parent: {parent}"
|
||||
assert! (← checkMatchCondParent e parent)
|
||||
else
|
||||
let mut found := false
|
||||
-- There is an argument `arg` s.t. root of `arg` is `e`.
|
||||
for arg in parent.getAppArgs do
|
||||
if (← checkChild e arg) then
|
||||
found := true
|
||||
break
|
||||
-- Recall that we have support for `Expr.forallE` propagation. See `ForallProp.lean`.
|
||||
if let .forallE _ d b _ := parent then
|
||||
if (← checkChild e d) then
|
||||
found := true
|
||||
unless b.hasLooseBVars do
|
||||
if (← checkChild e b) then
|
||||
found := true
|
||||
unless found do
|
||||
unless (← checkChild e parent.getAppFn) do
|
||||
throwError "e: {e}, parent: {parent}"
|
||||
assert! (← checkMatchCondParent e parent)
|
||||
else
|
||||
let mut found := false
|
||||
-- There is an argument `arg` s.t. root of `arg` is `e`.
|
||||
for arg in parent.getAppArgs do
|
||||
if (← checkChild e arg) then
|
||||
found := true
|
||||
break
|
||||
-- Recall that we have support for `Expr.forallE` propagation. See `ForallProp.lean`.
|
||||
if let .forallE _ d b _ := parent then
|
||||
if (← checkChild e d) then
|
||||
found := true
|
||||
unless b.hasLooseBVars do
|
||||
if (← checkChild e b) then
|
||||
found := true
|
||||
unless found do
|
||||
unless (← checkChild e parent.getAppFn) do
|
||||
throwError "e: {e}, parent: {parent}"
|
||||
assert! (← checkChild e parent.getAppFn)
|
||||
assert! (← checkChild e parent.getAppFn)
|
||||
else
|
||||
-- All the parents are stored in the root of the equivalence class.
|
||||
assert! (← getParents e).isEmpty
|
||||
|
|
|
|||
|
|
@ -575,6 +575,31 @@ where
|
|||
| .app f a => go f (mixHash r (hashRoot enodes a))
|
||||
| _ => mixHash r (hashRoot enodes e)
|
||||
|
||||
/-!
|
||||
**Note**: `congrHash` and `isCongruent` must satisfy the `BEq`/`Hashable` invariant for
|
||||
`PHashSet`: if `isCongruent e₁ e₂` returns `true`, then `congrHash e₁ == congrHash e₂`.
|
||||
|
||||
When `funCC = true`, `congrHash` hashes only the immediate function and argument:
|
||||
`mixHash (hashRoot f) (hashRoot a)`. When `funCC = false`, it recursively decomposes all
|
||||
application layers. These produce fundamentally different hash values, so `isCongruent` must
|
||||
require matching `funCC` flags. Here is the scenario that leads to a nondeterministic crash
|
||||
if mismatched flags are allowed:
|
||||
|
||||
1. `e₁` (with `funCC = true`) is inserted into the congruence table.
|
||||
2. `e₂` (with `funCC = false`) is inserted. Its hash is computed using the non-`funCC` path.
|
||||
3. Because `hashRoot` uses pointer addresses (`ptrAddrUnsafe`), the two different hash
|
||||
computations can accidentally collide, placing `e₂` in the same bucket as `e₁`.
|
||||
4. `isCongruent e₁ e₂` is called. If it used only `e₁`'s `funCC` flag (the old behavior),
|
||||
the `funCC` comparison path could declare them congruent even when they have different
|
||||
numbers of top-level arguments.
|
||||
5. `addCongrTable` calls `pushEqHEq e₂ e₁ congrPlaceholderProof`, where `e₂` is the new
|
||||
node with `funCC = false`.
|
||||
6. During proof reconstruction, `mkCongrProof e₂ e₁` checks `useFunCC e₂ = false`, enters
|
||||
the standard (non-`funCC`) proof path, and hits `assert! rhs.getAppNumArgs == numArgs`
|
||||
because `e₁` and `e₂` have different argument counts.
|
||||
|
||||
This was observed as a nondeterministic crash in Mathlib (e.g., at `Analysis/ODE/PicardLindelof.lean`).
|
||||
-/
|
||||
/-- Returns `true` if `e₁` and `e₂` are congruent modulo the equivalence classes in `enodes`. -/
|
||||
private partial def isCongruent (enodes : ENodeMap) (e₁ e₂ : Expr) : Bool :=
|
||||
if let .forallE _ d₁ b₁ _ := e₁ then
|
||||
|
|
@ -600,7 +625,14 @@ private partial def isCongruent (enodes : ENodeMap) (e₁ e₂ : Expr) : Bool :=
|
|||
**Note**: We are not in `MetaM` here. Thus, we cannot check whether `f` and `g` have the same type.
|
||||
So, we approximate and try to handle this issue when generating the proof term.
|
||||
-/
|
||||
hasSameRoot enodes a b && hasSameRoot enodes f g
|
||||
useFunCC' enodes e₂ && hasSameRoot enodes a b && hasSameRoot enodes f g
|
||||
else if useFunCC' enodes e₂ then
|
||||
/-
|
||||
Mismatched `funCC` flags: `e₁` uses first-order congruence, `e₂` uses higher-order.
|
||||
They hash differently (via `congrHash`), so declaring them congruent here would violate
|
||||
the `BEq`/`Hashable` consistency invariant required by `PHashSet`.
|
||||
-/
|
||||
false
|
||||
else
|
||||
hasSameRoot enodes a b && go f g
|
||||
where
|
||||
|
|
|
|||
|
|
@ -24,3 +24,9 @@ example
|
|||
a = b →
|
||||
x = y := by
|
||||
grind
|
||||
|
||||
-- funCC congruence: `h a` and `k` are in the same equivalence class as functions,
|
||||
-- so `h a b c = k b c` should follow.
|
||||
example (h : Nat → Nat → Nat → Nat) (k : Nat → Nat → Nat)
|
||||
: h a = k → h a b c = k b c := by
|
||||
grind
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue