Skip to content

Commit 1ac6f4e

Browse files
borsbors
committed
Auto merge of #3772 - flip1995:ice-3719, r=Manishearth
Fix ICE #3719+#3718 in lint match_ref_pats Fixes #3719 This conveniently also fixes #3718 The ICE occurs when the match expression was a macro call, where the macro was defined in another file. Since we don't have the ability to reproduce this behavior with our UI tests (AFAIK), I couldn't add a test reproducing this ICE.. However, I added a test which is related to the ICE, to show the new behavior of the lint. I tested it with the mscheme repo locally and the ICE didn't happen anymore. r? @matthiaskrgr
2 parents cd29740 + 75f3988 commit 1ac6f4e

File tree

4 files changed

+91
-37
lines changed

4 files changed

+91
-37
lines changed

‎clippy_lints/src/matches.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -570,13 +570,15 @@ fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr:
570570
if has_only_ref_pats(arms) {
571571
let mut suggs = Vec::new();
572572
let (title, msg) = if let ExprKind::AddrOf(Mutability::MutImmutable, ref inner) = ex.node {
573-
suggs.push((ex.span, Sugg::hir(cx, inner, "..").to_string()));
573+
let span = ex.span.source_callsite();
574+
suggs.push((span, Sugg::hir_with_macro_callsite(cx, inner, "..").to_string()));
574575
(
575576
"you don't need to add `&` to both the expression and the patterns",
576577
"try",
577578
)
578579
} else {
579-
suggs.push((ex.span, Sugg::hir(cx, ex, "..").deref().to_string()));
580+
let span = ex.span.source_callsite();
581+
suggs.push((span, Sugg::hir_with_macro_callsite(cx, ex, "..").deref().to_string()));
580582
(
581583
"you don't need to add `&` to all patterns",
582584
"instead of prefixing all patterns with `&`, you can dereference the expression",

‎clippy_lints/src/utils/sugg.rs

Lines changed: 46 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
//! Contains utility functions to generate suggestions.
22
#![deny(clippy::missing_docs_in_private_items)]
33

4-
use crate::utils::{higher, in_macro, snippet, snippet_opt};
4+
use crate::utils::{higher, in_macro, snippet, snippet_opt, snippet_with_macro_callsite};
55
use matches::matches;
66
use rustc::hir;
77
use rustc::lint::{EarlyContext, LateContext, LintContext};
@@ -46,38 +46,7 @@ impl<'a> Sugg<'a> {
4646
pub fn hir_opt(cx: &LateContext<'_, '_>, expr: &hir::Expr) -> Option<Self> {
4747
snippet_opt(cx, expr.span).map(|snippet| {
4848
let snippet = Cow::Owned(snippet);
49-
match expr.node {
50-
hir::ExprKind::AddrOf(..)
51-
| hir::ExprKind::Box(..)
52-
| hir::ExprKind::Closure(.., _)
53-
| hir::ExprKind::If(..)
54-
| hir::ExprKind::Unary(..)
55-
| hir::ExprKind::Match(..) => Sugg::MaybeParen(snippet),
56-
hir::ExprKind::Continue(..)
57-
| hir::ExprKind::Yield(..)
58-
| hir::ExprKind::Array(..)
59-
| hir::ExprKind::Block(..)
60-
| hir::ExprKind::Break(..)
61-
| hir::ExprKind::Call(..)
62-
| hir::ExprKind::Field(..)
63-
| hir::ExprKind::Index(..)
64-
| hir::ExprKind::InlineAsm(..)
65-
| hir::ExprKind::Lit(..)
66-
| hir::ExprKind::Loop(..)
67-
| hir::ExprKind::MethodCall(..)
68-
| hir::ExprKind::Path(..)
69-
| hir::ExprKind::Repeat(..)
70-
| hir::ExprKind::Ret(..)
71-
| hir::ExprKind::Struct(..)
72-
| hir::ExprKind::Tup(..)
73-
| hir::ExprKind::While(..)
74-
| hir::ExprKind::Err => Sugg::NonParen(snippet),
75-
hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
76-
hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet),
77-
hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(higher::binop(op.node)), snippet),
78-
hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet),
79-
hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
80-
}
49+
Self::hir_from_snippet(expr, snippet)
8150
})
8251
}
8352

@@ -111,6 +80,50 @@ impl<'a> Sugg<'a> {
11180
})
11281
}
11382

83+
/// Same as `hir`, but will use the pre expansion span if the `expr` was in a macro.
84+
pub fn hir_with_macro_callsite(cx: &LateContext<'_, '_>, expr: &hir::Expr, default: &'a str) -> Self {
85+
let snippet = snippet_with_macro_callsite(cx, expr.span, default);
86+
87+
Self::hir_from_snippet(expr, snippet)
88+
}
89+
90+
/// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*`
91+
/// function variants of `Sugg`, since these use different snippet functions.
92+
fn hir_from_snippet(expr: &hir::Expr, snippet: Cow<'a, str>) -> Self {
93+
match expr.node {
94+
hir::ExprKind::AddrOf(..)
95+
| hir::ExprKind::Box(..)
96+
| hir::ExprKind::Closure(.., _)
97+
| hir::ExprKind::If(..)
98+
| hir::ExprKind::Unary(..)
99+
| hir::ExprKind::Match(..) => Sugg::MaybeParen(snippet),
100+
hir::ExprKind::Continue(..)
101+
| hir::ExprKind::Yield(..)
102+
| hir::ExprKind::Array(..)
103+
| hir::ExprKind::Block(..)
104+
| hir::ExprKind::Break(..)
105+
| hir::ExprKind::Call(..)
106+
| hir::ExprKind::Field(..)
107+
| hir::ExprKind::Index(..)
108+
| hir::ExprKind::InlineAsm(..)
109+
| hir::ExprKind::Lit(..)
110+
| hir::ExprKind::Loop(..)
111+
| hir::ExprKind::MethodCall(..)
112+
| hir::ExprKind::Path(..)
113+
| hir::ExprKind::Repeat(..)
114+
| hir::ExprKind::Ret(..)
115+
| hir::ExprKind::Struct(..)
116+
| hir::ExprKind::Tup(..)
117+
| hir::ExprKind::While(..)
118+
| hir::ExprKind::Err => Sugg::NonParen(snippet),
119+
hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
120+
hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet),
121+
hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(higher::binop(op.node)), snippet),
122+
hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet),
123+
hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
124+
}
125+
}
126+
114127
/// Prepare a suggestion from an expression.
115128
pub fn ast(cx: &EarlyContext<'_>, expr: &ast::Expr, default: &'a str) -> Self {
116129
use syntax::ast::RangeLimits;

‎tests/ui/matches.rs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,4 +150,29 @@ fn match_as_ref() {
150150
};
151151
}
152152

153-
fn main() {}
153+
macro_rules! foo_variant(
154+
($idx:expr) => (Foo::get($idx).unwrap())
155+
);
156+
157+
enum Foo {
158+
A,
159+
B,
160+
}
161+
162+
impl Foo {
163+
fn get(idx: u8) -> Option<&'static Self> {
164+
match idx {
165+
0 => Some(&Foo::A),
166+
1 => Some(&Foo::B),
167+
_ => None,
168+
}
169+
}
170+
}
171+
172+
fn main() {
173+
// ICE #3719
174+
match foo_variant!(0) {
175+
&Foo::A => println!("A"),
176+
_ => println!("Wild"),
177+
}
178+
}

‎tests/ui/matches.stderr

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,5 +278,19 @@ LL | | Some(ref mut v) => Some(v),
278278
LL | | };
279279
| |_____^ help: try this: `mut_owned.as_mut()`
280280

281-
error: aborting due to 19 previous errors
281+
error: you don't need to add `&` to all patterns
282+
--> $DIR/matches.rs:174:5
283+
|
284+
LL | / match foo_variant!(0) {
285+
LL | | &Foo::A => println!("A"),
286+
LL | | _ => println!("Wild"),
287+
LL | | }
288+
| |_____^
289+
help: instead of prefixing all patterns with `&`, you can dereference the expression
290+
|
291+
LL | match *foo_variant!(0) {
292+
LL | Foo::A => println!("A"),
293+
|
294+
295+
error: aborting due to 20 previous errors
282296

0 commit comments

Comments
 (0)