3

fix: Correctly check for parentheses redundancy in `remove_parentheses` assist b...

 1 year ago
source link: https://github.com/rust-lang/rust-analyzer/pull/13764
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Correctly check for parentheses redundancy in `remove_parentheses` assist by WaffleLapkin · Pull Request #13764 · rust-lang/rust-analyzer · GitHub

Member

WaffleLapkin commented Dec 13, 2022

This is quite a bunch of code and some hacks, but I think this time it's correct.

I've added a lot of tests, most of which fail with the assist impl from #13733 :')

rustbot

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

Dec 13, 2022

ast::StmtList(_) => self.needs_parens_in_stmt(None),

ast::ArgList(_) => false,

ast::MatchArm(_) => false,

_ => unimplemented!()

I feel like this should just be false? unimplmented seems wrong since other nodes don't make sense here right?

Yeah, right. This was left from the time I was testing things out and haven't figured out possible parents.

fixed :)

return contains_exterior_struct_lit_inner(self).is_some();

fn contains_exterior_struct_lit_inner(expr: &Expr) -> Option<()> {

use Expr::*;

match expr {

RecordExpr(..) => Some(()),

// X { y: 1 } + X { y: 2 }

BinExpr(e) => e

.lhs()

.as_ref()

.and_then(contains_exterior_struct_lit_inner)

.or_else(|| e.rhs().as_ref().and_then(contains_exterior_struct_lit_inner)),

// `&X { y: 1 }`, `X { y: 1 }.y`, `X { y: 1 }.bar(...)`, etc

IndexExpr(e) => contains_exterior_struct_lit_inner(&e.base()?),

AwaitExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),

PrefixExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),

CastExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),

FieldExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),

MethodCallExpr(e) => contains_exterior_struct_lit_inner(&e.receiver()?),

_ => None,

}

}

This could use a poor-mans try block aka immediately called closure

Suggested change
return contains_exterior_struct_lit_inner(self).is_some();
fn contains_exterior_struct_lit_inner(expr: &Expr) -> Option<()> {
use Expr::*;
match expr {
RecordExpr(..) => Some(()),
// X { y: 1 } + X { y: 2 }
BinExpr(e) => e
.lhs()
.as_ref()
.and_then(contains_exterior_struct_lit_inner)
.or_else(|| e.rhs().as_ref().and_then(contains_exterior_struct_lit_inner)),
// `&X { y: 1 }`, `X { y: 1 }.y`, `X { y: 1 }.bar(...)`, etc
IndexExpr(e) => contains_exterior_struct_lit_inner(&e.base()?),
AwaitExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
PrefixExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
CastExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
FieldExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
MethodCallExpr(e) => contains_exterior_struct_lit_inner(&e.receiver()?),
_ => None,
}
}
(|| {
use Expr::*;
match expr {
RecordExpr(..) => Some(()),
// X { y: 1 } + X { y: 2 }
BinExpr(e) => e
.lhs()
.as_ref()
.and_then(contains_exterior_struct_lit_inner)
.or_else(|| e.rhs().as_ref().and_then(contains_exterior_struct_lit_inner)),
// `&X { y: 1 }`, `X { y: 1 }.y`, `X { y: 1 }.bar(...)`, etc
IndexExpr(e) => contains_exterior_struct_lit_inner(&e.base()?),
AwaitExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
PrefixExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
CastExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
FieldExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?),
MethodCallExpr(e) => contains_exterior_struct_lit_inner(&e.receiver()?),
_ => None,
}
})().is_some()

It kind of makes the code worse, since the recursive calls return bool that needs to be converted back to Option...

    fn contains_exterior_struct_lit(&self) -> bool {
        (|| {
            use Expr::*;
            (match self {
                RecordExpr(..) => true,
                // X { y: 1 } + X { y: 2 }
                BinExpr(e) => {
                    return e
                        .lhs()
                        .as_ref()
                        .filter(|e| e.contains_exterior_struct_lit())
                        .map(drop)
                        .or_else(|| {
                            e.rhs().as_ref().filter(|e| e.contains_exterior_struct_lit()).map(drop)
                        })
                }
                // `&X { y: 1 }`, `X { y: 1 }.y`, `X { y: 1 }.bar(...)`, etc
                IndexExpr(e) => e.base()?.contains_exterior_struct_lit(),
                AwaitExpr(e) => e.expr()?.contains_exterior_struct_lit(),
                PrefixExpr(e) => e.expr()?.contains_exterior_struct_lit(),
                CastExpr(e) => e.expr()?.contains_exterior_struct_lit(),
                FieldExpr(e) => e.expr()?.contains_exterior_struct_lit(),
                MethodCallExpr(e) => e.receiver()?.contains_exterior_struct_lit(),
                _ => return None,
            })
            .then(|| ())
        })()
        .is_some()
    }

Member

@Veykril Veykril Dec 20, 2022 •

edited

Oh thats a good point, nvm


report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK