Calling .pop() on an Attribute node causes BorrowMutError panic

[mickdekkers]

Hey, I noticed that the .pop() method appears to be broken for Attribute nodes. It causes a BorrowMutError panic when called.

Example code:

use std::rc::Rc;

use xrust::item::Node;
use xrust::parser::xml::parse as xmlparse;
use xrust::trees::smite::Node as SmiteNode;

fn main() {
    let input = r"<doc foo='bar'/>";

    let root = xmlparse(Rc::new(SmiteNode::new()), input, None).unwrap();
    let elem = root.first_child().unwrap();
    let mut foo_attr = elem.attribute_iter().next().unwrap();
    // Panics with BorrowMutError
    foo_attr.pop().unwrap();
}

Panics when run with cargo run:

thread 'main' panicked at /home/mick/.cargo/git/checkouts/xrust-bdc2299a4e8ad91c/127ee22/src/trees/smite.rs:849:59:
already borrowed: BorrowMutError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Can run with core/debug_refcell feature (adjust target triple to match your host):

cargo +nightly run --target x86_64-unknown-linux-gnu -Zbuild-std -Zbuild-std-features=core/debug_refcell

To display additional info about the panic:

thread 'main' panicked at /home/mick/.cargo/git/checkouts/xrust-bdc2299a4e8ad91c/127ee22/src/trees/smite.rs:849:59:
already borrowed: BorrowMutError { location: Location { file: "/home/mick/.cargo/git/checkouts/xrust-bdc2299a4e8ad91c/127ee22/src/trees/smite.rs", line: 426, col: 45 } }

The line numbers are a bit shifted for me, but it refers to these two lines:

xrust/src/trees/smite.rs

Line 424 in 2449e4d

match Weak::upgrade(&parent.borrow()) {

xrust/src/trees/smite.rs

Line 847 in 2449e4d

| NodeInner::ProcessingInstruction(p, _, _) => *p.borrow_mut() = Rc::downgrade(&b),

Tested with latest dev branch commit (2449e4d)

[ballsteve]

Thanks for the bug report. I'll look at this ASAP.

[mickdekkers]

Thanks! In the meantime I'm using this workaround to avoid calling .pop() on any attribute nodes:

/// Workaround for faulty attribute node .pop() method.
///
/// Takes an attribute node and returns a new parent element with all children and attributes except
/// the given attribute node. The other attributes are copied, but the children are moved.
fn attribute_pop_workaround(attr_node: RNode) -> Result<RNode, xrust::Error> {
    assert_eq!(attr_node.node_type(), NodeType::Attribute);
    let mut parent = attr_node.parent().unwrap();
    let mut new_parent = parent.owner_document().new_element(parent.name())?;
    let attrs: Vec<_> = parent
        .attribute_iter()
        // We want to keep all attributes except attr_node
        .filter(|node| !node.is_same(&attr_node))
        .map(|node| {
            // .add_attribute() also calls .pop() on the attribute node, which would panic, so we
            // need a copy of the attribute node that isn't attached to any parent. Using
            // .deep_copy() would just clone the attribute Rc<Node>, so instead we create a new
            // attribute node with the same name and value ourselves.
            let new_attr = new_parent
                .owner_document()
                .new_attribute(node.name(), node.value())?;
            Ok(new_attr)
        })
        .collect::<Result<_, xrust::Error>>()?;
    for attr in attrs.into_iter() {
        new_parent.add_attribute(attr)?;
    }
    let children: Vec<_> = parent.child_iter().collect();
    for child in children.into_iter() {
        new_parent.push(child)?;
    }
    // Insert the new parent before the old one
    parent.insert_before(new_parent.clone())?;
    // Remove the old parent
    parent.pop()?;
    // The new parent is now in the same position as the old one
    Ok(new_parent)
}

Used like:

// [...]
let elem = root.first_child().unwrap();
let foo_attr = elem.attribute_iter().next().unwrap();
let new_elem = attribute_pop_workaround(foo_attr).unwrap();