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

Don't expand macros in some suggestions #3366

Merged
merged 3 commits into from Oct 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions clippy_lints/src/identity_conversion.rs
Expand Up @@ -12,7 +12,7 @@ use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use crate::rustc::{declare_tool_lint, lint_array};
use crate::rustc::hir::*;
use crate::syntax::ast::NodeId;
use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, span_lint_and_then};
use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, snippet_with_macro_callsite, span_lint_and_then};
use crate::utils::{opt_def_id, paths, resolve_node};
use crate::rustc_errors::Applicability;

Expand Down Expand Up @@ -72,7 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityConversion {
let a = cx.tables.expr_ty(e);
let b = cx.tables.expr_ty(&args[0]);
if same_tys(cx, a, b) {
let sugg = snippet(cx, args[0].span, "<expr>").into_owned();
let sugg = snippet_with_macro_callsite(cx, args[0].span, "<expr>").to_string();

span_lint_and_then(cx, IDENTITY_CONVERSION, e.span, "identical conversion", |db| {
db.span_suggestion_with_applicability(
e.span,
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/matches.rs
Expand Up @@ -19,7 +19,8 @@ use crate::syntax::ast::LitKind;
use crate::syntax::source_map::Span;
use crate::utils::paths;
use crate::utils::{expr_block, is_allowed, is_expn_of, match_qpath, match_type, multispan_sugg,
remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty};
remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then,
span_note_and_lint, walk_ptrs_ty};
use crate::utils::sugg::Sugg;
use crate::consts::{constant, Constant};
use crate::rustc_errors::Applicability;
Expand Down
8 changes: 4 additions & 4 deletions clippy_lints/src/methods/mod.rs
Expand Up @@ -21,7 +21,7 @@ use crate::utils::sugg;
use crate::utils::{
get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty,
iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type,
match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, span_lint,
match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_macro_callsite, span_lint,
span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
};
use if_chain::if_chain;
Expand Down Expand Up @@ -1062,9 +1062,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
}

let sugg: Cow<'_, _> = match (fn_has_arguments, !or_has_args) {
(true, _) => format!("|_| {}", snippet(cx, arg.span, "..")).into(),
(false, false) => format!("|| {}", snippet(cx, arg.span, "..")).into(),
(false, true) => snippet(cx, fun_span, ".."),
(true, _) => format!("|_| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
(false, false) => format!("|| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
(false, true) => snippet_with_macro_callsite(cx, fun_span, ".."),
};
let span_replace_word = method_span.with_hi(span.hi());
span_lint_and_sugg(
Expand Down
11 changes: 10 additions & 1 deletion clippy_lints/src/utils/mod.rs
Expand Up @@ -362,6 +362,12 @@ pub fn snippet<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str)
snippet_opt(cx, span).map_or_else(|| Cow::Borrowed(default), From::from)
}

/// Same as `snippet`, but should only be used when it's clear that the input span is
/// not a macro argument.
pub fn snippet_with_macro_callsite<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> {
snippet(cx, span.source_callsite(), default)
}

/// Convert a span to a code snippet. Returns `None` if not available.
pub fn snippet_opt<'a, T: LintContext<'a>>(cx: &T, span: Span) -> Option<String> {
cx.sess().source_map().span_to_snippet(span).ok()
Expand Down Expand Up @@ -400,7 +406,10 @@ pub fn expr_block<'a, 'b, T: LintContext<'b>>(
) -> Cow<'a, str> {
let code = snippet_block(cx, expr.span, default);
let string = option.unwrap_or_default();
if let ExprKind::Block(_, _) = expr.node {
if in_macro(expr.span) {
Cow::Owned(format!("{{ {} }}", snippet_with_macro_callsite(cx, expr.span, default)))
}
else if let ExprKind::Block(_, _) = expr.node {
Cow::Owned(format!("{}{}", code, string))
} else if string.is_empty() {
Cow::Owned(format!("{{ {} }}", code))
Expand Down
1 change: 1 addition & 0 deletions tests/ui/identity_conversion.rs
Expand Up @@ -53,4 +53,5 @@ fn main() {
let _ = String::from(format!("A: {:04}", 123));
let _ = "".lines().into_iter();
let _ = vec![1, 2, 3].into_iter().into_iter();
let _: String = format!("Hello {}", "world").into();
}
8 changes: 7 additions & 1 deletion tests/ui/identity_conversion.stderr
Expand Up @@ -58,5 +58,11 @@ error: identical conversion
55 | let _ = vec![1, 2, 3].into_iter().into_iter();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2, 3].into_iter()`

error: aborting due to 9 previous errors
error: identical conversion
--> $DIR/identity_conversion.rs:56:21
|
56 | let _: String = format!("Hello {}", "world").into();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `format!("Hello {}", "world")`

error: aborting due to 10 previous errors

2 changes: 1 addition & 1 deletion tests/ui/matches.stderr
Expand Up @@ -33,7 +33,7 @@ error: you seem to be trying to use match for destructuring a single pattern. Co
51 | | &(v, 1) => println!("{}", v),
52 | | _ => println!("none"),
53 | | }
| |_____^ help: try this: `if let &(v, 1) = tup { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; } else { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; }`
| |_____^ help: try this: `if let &(v, 1) = tup { println!("{}", v) } else { println!("none") }`

error: you don't need to add `&` to all patterns
--> $DIR/matches.rs:50:5
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/methods.stderr
Expand Up @@ -297,7 +297,7 @@ error: use of `unwrap_or` followed by a function call
--> $DIR/methods.rs:339:14
|
339 | with_vec.unwrap_or(vec![]);
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))`
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| vec![])`

error: use of `unwrap_or` followed by a function call
--> $DIR/methods.rs:344:21
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/single_match.rs
Expand Up @@ -23,6 +23,15 @@ fn single_match(){
_ => ()
};

let x = Some(1u8);
match x {
// Note the missing block braces.
// We suggest `if let Some(y) = x { .. }` because the macro
// is expanded before we can do anything.
Some(y) => println!("{:?}", y),
_ => ()
}

let z = (1u8,1u8);
match z {
(2...3, 7...9) => dummy(),
Expand Down
52 changes: 32 additions & 20 deletions tests/ui/single_match.stderr
Expand Up @@ -12,38 +12,50 @@ error: you seem to be trying to use match for destructuring a single pattern. Co
error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
--> $DIR/single_match.rs:27:5
|
27 | / match z {
28 | | (2...3, 7...9) => dummy(),
29 | | _ => {}
30 | | };
27 | / match x {
28 | | // Note the missing block braces.
29 | | // We suggest `if let Some(y) = x { .. }` because the macro
30 | | // is expanded before we can do anything.
31 | | Some(y) => println!("{:?}", y),
32 | | _ => ()
33 | | }
| |_____^ help: try this: `if let Some(y) = x { println!("{:?}", y) }`

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
--> $DIR/single_match.rs:36:5
|
36 | / match z {
37 | | (2...3, 7...9) => dummy(),
38 | | _ => {}
39 | | };
| |_____^ help: try this: `if let (2...3, 7...9) = z { dummy() }`

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
--> $DIR/single_match.rs:53:5
--> $DIR/single_match.rs:62:5
|
53 | / match x {
54 | | Some(y) => dummy(),
55 | | None => ()
56 | | };
62 | / match x {
63 | | Some(y) => dummy(),
64 | | None => ()
65 | | };
| |_____^ help: try this: `if let Some(y) = x { dummy() }`

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
--> $DIR/single_match.rs:58:5
--> $DIR/single_match.rs:67:5
|
58 | / match y {
59 | | Ok(y) => dummy(),
60 | | Err(..) => ()
61 | | };
67 | / match y {
68 | | Ok(y) => dummy(),
69 | | Err(..) => ()
70 | | };
| |_____^ help: try this: `if let Ok(y) = y { dummy() }`

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
--> $DIR/single_match.rs:65:5
--> $DIR/single_match.rs:74:5
|
65 | / match c {
66 | | Cow::Borrowed(..) => dummy(),
67 | | Cow::Owned(..) => (),
68 | | };
74 | / match c {
75 | | Cow::Borrowed(..) => dummy(),
76 | | Cow::Owned(..) => (),
77 | | };
| |_____^ help: try this: `if let Cow::Borrowed(..) = c { dummy() }`

error: aborting due to 5 previous errors
error: aborting due to 6 previous errors