Skip to content

fix(mir): make BackLink parent removal precise#531

Open
phrwlk wants to merge 1 commit into0xMiden:nextfrom
phrwlk:fix/mir-backlink-remove-parent
Open

fix(mir): make BackLink parent removal precise#531
phrwlk wants to merge 1 commit into0xMiden:nextfrom
phrwlk:fix/mir-backlink-remove-parent

Conversation

@phrwlk
Copy link
Copy Markdown

@phrwlk phrwlk commented Jan 5, 2026

Describe your changes

BackLink implements PartialEq as always true so that backlink fields are ignored in derived equality and hashing. However,
remove_parent in MIR ops compared BackLink values directly:

self.parents.retain(|p| *p != parent.clone().into());*

Because of the always-true PartialEq, this predicate was always false and effectively cleared the entire parents vector instead of removing only the requested parent.

This change updates all remove_parent implementations for ops that use parents: Vec<BackLink> to compare against the upgraded Link via to_link(), keeping BackLink’s structural Eq/Hash semantics intact while making parent removal semantically correct.

@AlgofootPrint
Copy link
Copy Markdown

Review: Fix is incomplete. Link::PartialEq compares by value, not pointer identity @phrwlk

I tested this PR locally and the included test add_remove_parent_removes_only_target fails:

thread 'ir::nodes::ops::add::tests::add_remove_parent_removes_only_target' panicked
assertion left == right failed
left: 0
right: 1

The root cause diagnosis is correct — BackLink::PartialEq always returning true breaks retain. However, the proposed
fix still has a problem.

Switching to p.to_link() and then link != parent uses Link::PartialEq, which compares by value (self.link ==
other.link → Rc<RefCell> → inner value equality). So if two different owners happen to hold the same value (as in
the test: both are Owner::None(span)), they compare as equal and both get removed, leaving parents.len() == 0.

The fix needs pointer identity, using the already-available get_ptr() method on Link:

fn remove_parent(&mut self, parent: LinkSelf::Parent) {
self.parents.retain(|p| match p.to_link() {
Some(link) => link.get_ptr() != parent.get_ptr(),
None => true,
});
}

This uses Rc::as_ptr under the hood, which correctly distinguishes different allocations regardless of value
equality. The same fix needs to be applied to all 16 op files.

The test itself is well-structured and proves the regression correctly but it just needs the implementation to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants