Stream: t-compiler/wg-rls-2.0

Topic: fill match arms deletes _ case


dubi steinkek (Jun 03 2020 at 10:54, on Zulip):

I'd like to try fixing this issue: https://github.com/rust-analyzer/rust-analyzer/issues/4165.
When using the fill match arms assist on

enum E { A, B, C }
// ...
match e {
    E::A => 1,
    _ => 2,
}

rust-analyzer replaces the _ with E::B => {}, E::C => {}.

As tspiteri mentions, there are 3 ways of handling the instead:

1.    E::B | E::C => 2,
2.    E::B => 2,
      E::C => 2,
3.    _ => 2

Which one should I be implementing?

matklad (Jun 03 2020 at 10:56, on Zulip):

So, I think we should check if the => expr is a trival expression (there's some code for this in the assist already) and, if it is the case, we should remove the arm.

Otherwise, I think we should insert missing arms with {} before the wildcard

matklad (Jun 03 2020 at 10:56, on Zulip):

so,

match e {
  E::A => 1,
  E::B => {}
  E::C => {}
  _ => 2,
}
dubi steinkek (Jun 03 2020 at 10:59, on Zulip):

Why do you think we should remove the arm? That is not what I would be expecting as a user.

matklad (Jun 03 2020 at 11:03, on Zulip):

If the arm is trivial, I would expect it to be removed. What else woudl you do with a trivial arm, if you are going to fill others anyway?

dubi steinkek (Jun 03 2020 at 11:05, on Zulip):

Maybe you know there is only one match arm left, and want to have an explicit match arm instead of a placeholder

dubi steinkek (Jun 03 2020 at 11:06, on Zulip):

i.e. replace

match some_option {
    Some(v) => v,
    _ => panic!(),
}

with

match some_option {
    Some(v) => v,
    None => panic!(),
}
matklad (Jun 03 2020 at 11:07, on Zulip):

I'd say if the has jsut panic, it's fine to remove it. It has something else, its not trivial and won't be remoed

dubi steinkek (Jun 03 2020 at 11:16, on Zulip):

okay, I see. You said there is some code for checking whether an expression is trivial but I can't find it, what exactly should I look for?

dubi steinkek (Jun 03 2020 at 11:17, on Zulip):

or to determine whether the code is trivial should I just check whether the expression is a block or not?

matklad (Jun 03 2020 at 11:18, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/blob/7e2bca4ec356655fe47d50b466c34c34e7cae565/crates/ra_assists/src/handlers/fill_match_arms.rs#L77-L82

matklad (Jun 03 2020 at 11:18, on Zulip):

Looks like I am remembering incorrectly -- it doesn't try to check if the expression to the right of => is "trivial"

dubi steinkek (Jun 03 2020 at 11:25, on Zulip):

It looks like is_trivial was removed in https://github.com/rust-analyzer/rust-analyzer/commit/6087c014608108e2b971608e214a74759743e95e. If I understand correctly an expression is not trivial if it is wrapped in block, right? So that shouln't be too hard to check.

dubi steinkek (Jun 03 2020 at 11:38, on Zulip):
fn expression_is_trivial(expr: &ast::Expr) -> bool {
    match expr {
        ast::Expr::ArrayExpr(_)
        | ast::Expr::ReturnExpr(_)
        | ast::Expr::CallExpr(_)
        | ast::Expr::MethodCallExpr(_)
        | ast::Expr::RangeExpr(_)
        | ast::Expr::MacroCall(_)
        | ast::Expr::Literal(_) => true,
        ast::Expr::TupleExpr(e) => e.exprs().all(|e| expression_is_trivial(&e)),
        ast::Expr::BoxExpr(e) => e.expr().map_or(false, |e| expression_is_trivial(&e)),
        ast::Expr::RefExpr(e) => e.expr().map_or(false, |e| expression_is_trivial(&e)),
        ast::Expr::PrefixExpr(e) => e.expr().map_or(false, |e| expression_is_trivial(&e)),
        ast::Expr::CastExpr(e) => e.expr().map_or(false, |e| expression_is_trivial(&e)),
        _ => false,
    }
}

how about this?

matklad (Jun 03 2020 at 11:39, on Zulip):

I'd maybe just used expr.text() == "{}" || expr.text() == "()" || expr.text() == "todo!()", should be good enoguh

dubi steinkek (Jun 03 2020 at 11:40, on Zulip):

okay :thumbs_up:

Last update: Sep 27 2020 at 14:00UTC