0

I'm using AsRef<T> and AsMut<T> to expose an wrapped value in an enum. Can anyone tell me if this is an anti-pattern? I came across Is it considered a bad practice to implement Deref for newtypes?, and it convinced me that Deref would be a bad idea, but I'm not sure about the approach below.

pub enum Node {
    Stmt(Statement),
    Expr(Expression),
}

impl AsMut<Expression> for Node {
    fn as_mut(&mut self) -> &mut Expression {
        match self {
            Node::Stmt(_) => panic!("fatal: expected Expression"),
            Node::Expr(e) => e,
        }
    }
}

impl AsMut<Expression> for Box<Node> {
    fn as_mut(&mut self) -> &mut Expression {
        (**self).as_mut()
    }
}

impl AsMut<Expression> for Expression {
    fn as_mut(&mut self) -> &mut Expression {
        self
    }
}

fn check_binop<T: AsMut<Expression>>(
    &mut self,
    sym: Symbol,
    lhs: &mut T,
    rhs: &mut T,
    ty: &mut Option<Type>,
) -> Result<Type, String> {
    let lhs = lhs.as_mut();
    let rhs = rhs.as_mut();
    ...
}

I'm considering just making my own traits (AsExpr<T> and AsExprMut<T>) that are just re-implementations of AsRef<T> and AsMut<T>. Functionally no different, but I think it would be clearer.

marcantonio
  • 958
  • 10
  • 24

1 Answers1

4

I strongly suggest you do not do this, especially considering the docs for AsRef and AsMut say in bold:

Note: This trait must not fail.

and I would definitely consider panicking to be failure. This isn't an anti-pattern so much as it is breaking the contract you opt-into by implementing those traits. You should consider returning an Option<&mut Expression> instead of going through AsRef/AsMut, or making your own traits like you mentioned.

Ian S.
  • 1,831
  • 9
  • 17
  • Yep, this is definitely not fine. Too tired to answer a question, probably... – Chayim Friedman May 22 '22 at 03:31
  • Thanks for validating. and very well put re: contract breaking. My original way didn't _feel_ right. I ended up going with my own `AsExpr`/`AsExprMut` which just mirror `AsRef`/`AsMut`. I tried `Option` and `Result`, but the ergonomics were really poor. Also, in my case, a compiler frontend, `as_expr` should never be called on a Node that might not contain one. That would be an unrecoverable bug in the compiler, rather than user error. – marcantonio May 22 '22 at 17:50