Skip to content

Commit

Permalink
Auto merge of #3561 - fuerstenau:master, r=oli-obk
Browse files Browse the repository at this point in the history 
Suggest `.as_ref()?` instead of `?` in certain circumstances.
  • Loading branch information
@bors
bors committed Dec 28, 2018
2 parents f7bdf50 + 8be7050 commit 3c4abb5
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 39 deletions.
60 changes: 29 additions & 31 deletions clippy_lints/src/question_mark.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,52 +72,50 @@ impl Pass {
if Self::is_option(cx, subject);

then {
let receiver_str = &Sugg::hir(cx, subject, "..");
let mut replacement: Option<String> = None;
if let Some(else_) = else_ {
if_chain! {
if let ExprKind::Block(block, None) = &else_.node;
if block.stmts.len() == 0;
if let Some(block_expr) = &block.expr;
if SpanlessEq::new(cx).ignore_fn().eq_expr(subject, block_expr);
then {
span_lint_and_then(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
|db| {
db.span_suggestion_with_applicability(
expr.span,
"replace_it_with",
format!("Some({}?)", Sugg::hir(cx, subject, "..")),
Applicability::MaybeIncorrect, // snippet
);
}
)
replacement = Some(format!("Some({}?)", receiver_str));
}
}
return;
} else if Self::moves_by_default(cx, subject) {
replacement = Some(format!("{}.as_ref()?;", receiver_str));
} else {
replacement = Some(format!("{}?;", receiver_str));
}

span_lint_and_then(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
|db| {
let receiver_str = &Sugg::hir(cx, subject, "..");

db.span_suggestion_with_applicability(
expr.span,
"replace_it_with",
format!("{}?;", receiver_str),
Applicability::MaybeIncorrect, // snippet
);
}
)
if let Some(replacement_str) = replacement {
span_lint_and_then(
cx,
QUESTION_MARK,
expr.span,
"this block may be rewritten with the `?` operator",
|db| {
db.span_suggestion_with_applicability(
expr.span,
"replace_it_with",
replacement_str,
Applicability::MaybeIncorrect, // snippet
);
}
)
}
}
}
}

fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr) -> bool {
let expr_ty = cx.tables.expr_ty(expression);

expr_ty.moves_by_default(cx.tcx, cx.param_env, expression.span)
}

fn is_option(cx: &LateContext<'_, '_>, expression: &Expr) -> bool {
let expr_ty = cx.tables.expr_ty(expression);

Expand Down
54 changes: 50 additions & 4 deletions tests/ui/question_mark.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ fn some_func(a: Option<u32>) -> Option<u32> {
a
}

fn some_other_func(a: Option<u32>) -> Option<u32> {
if a.is_none() {
return None;
} else {
return Some(0);
}
unreachable!()
}

pub enum SeemsOption<T> {
Some(T),
None,
Expand All @@ -37,11 +46,11 @@ fn returns_something_similar_to_option(a: SeemsOption<u32>) -> SeemsOption<u32>
a
}

pub struct SomeStruct {
pub struct CopyStruct {
pub opt: Option<u32>,
}

impl SomeStruct {
impl CopyStruct {
#[rustfmt::skip]
pub fn func(&self) -> Option<u32> {
if (self.opt).is_none() {
Expand All @@ -62,12 +71,49 @@ impl SomeStruct {
}
}

#[derive(Clone)]
pub struct MoveStruct {
pub opt: Option<Vec<u32>>,
}

impl MoveStruct {
pub fn ref_func(&self) -> Option<Vec<u32>> {
if self.opt.is_none() {
return None;
}

self.opt.clone()
}

pub fn mov_func_reuse(self) -> Option<Vec<u32>> {
if self.opt.is_none() {
return None;
}

self.opt
}

pub fn mov_func_no_use(self) -> Option<Vec<u32>> {
if self.opt.is_none() {
return None;
}
Some(Vec::new())
}
}

fn main() {
some_func(Some(42));
some_func(None);

let some_struct = SomeStruct { opt: Some(54) };
some_struct.func();
let copy_struct = CopyStruct { opt: Some(54) };
copy_struct.func();

let move_struct = MoveStruct {
opt: Some(vec![42, 1337]),
};
move_struct.ref_func();
move_struct.clone().mov_func_reuse();
move_struct.clone().mov_func_no_use();

let so = SeemsOption::Some(45);
returns_something_similar_to_option(so);
Expand Down
32 changes: 28 additions & 4 deletions tests/ui/question_mark.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,23 @@ LL | | }
= note: `-D clippy::question-mark` implied by `-D warnings`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:47:9
--> $DIR/question_mark.rs:56:9
|
LL | / if (self.opt).is_none() {
LL | | return None;
LL | | }
| |_________^ help: replace_it_with: `(self.opt)?;`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:51:9
--> $DIR/question_mark.rs:60:9
|
LL | / if self.opt.is_none() {
LL | | return None
LL | | }
| |_________^ help: replace_it_with: `self.opt?;`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:55:17
--> $DIR/question_mark.rs:64:17
|
LL | let _ = if self.opt.is_none() {
| _________________^
Expand All @@ -35,5 +35,29 @@ LL | | self.opt
LL | | };
| |_________^ help: replace_it_with: `Some(self.opt?)`

error: aborting due to 4 previous errors
error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:81:9
|
LL | / if self.opt.is_none() {
LL | | return None;
LL | | }
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:89:9
|
LL | / if self.opt.is_none() {
LL | | return None;
LL | | }
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:97:9
|
LL | / if self.opt.is_none() {
LL | | return None;
LL | | }
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`

error: aborting due to 7 previous errors

0 comments on commit 3c4abb5

Please sign in to comment.