Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OR patterns in let chains are formatted without indentation #194

Open
BoxyUwU opened this issue Sep 16, 2024 · 1 comment
Open

OR patterns in let chains are formatted without indentation #194

BoxyUwU opened this issue Sep 16, 2024 · 1 comment

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 16, 2024

        let hir = tcx.hir();
        if let Some(body_id) = tcx.hir_node_by_def_id(def_id).body_id() {
            tcx.arena.alloc_from_iter(hir.body_param_names(body_id))
        } else if let Node::TraitItem(&TraitItem {
            kind: TraitItemKind::Fn(_, TraitFn::Required(idents)),
            ..
        })
        | Node::ForeignItem(&ForeignItem {
            kind: ForeignItemKind::Fn(_, idents, _),
            ..
        }) = tcx.hir_node(tcx.local_def_id_to_hir_id(def_id))
        {
            idents
        } else {
            span_bug!(
                hir.span(tcx.local_def_id_to_hir_id(def_id)),
                "fn_arg_names: unexpected item {:?}",
                def_id
            );
        }

The indentation on this else if let is very confusing as it looks like the if else ends because we are at "normal" indentation for statements in the body, which makes it look like someone has written an assignment with a pattern with a leading | which is then somehow followed up by a block with an else on the end of it (?)

Something like the following would be significantly clearer as the indentation signifies that this is inside of the else condition:

        let hir = tcx.hir();
        if let Some(body_id) = tcx.hir_node_by_def_id(def_id).body_id() {
            tcx.arena.alloc_from_iter(hir.body_param_names(body_id))
        } else if let Node::TraitItem(&TraitItem {
                kind: TraitItemKind::Fn(_, TraitFn::Required(idents)),
                ..
            })
            | Node::ForeignItem(&ForeignItem {
                kind: ForeignItemKind::Fn(_, idents, _),
                ..
            }) = tcx.hir_node(tcx.local_def_id_to_hir_id(def_id))
        {
            idents
        } else {
            span_bug!(
                hir.span(tcx.local_def_id_to_hir_id(def_id)),
                "fn_arg_names: unexpected item {:?}",
                def_id
            );
        }
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Sep 16, 2024

Relatedly consider the following two code examples:

else if let Node::TraitItem(&TraitItem {
    kind: TraitItemKind::Fn(..),
}) | Node::ForeignItem(&ForeignItem {
    kind: ForeignItemKind::Fn(..),
}) = tcx.hir_node()
{
    x
}

and

else if Node::trait_item() {
    TraitItemKind::Fn(..),
} else if let Node::ForeignItem(&ForeignItem {
    kind: ForeignItemKind::Fn(..),
}) = tcx.hir_node()
{
    x
}

The former has incredibly similar structure to the latter despite having incredibly different meaning and the only distinction is | let which is very easy to miss when skimming the code.

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

No branches or pull requests

1 participant