From 7c2526a9d7ff0d141681d4b836e71cd877e4f56e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 12 Nov 2017 14:18:47 -0800 Subject: [PATCH 1/3] Be more obvious when suggesting dereference Include enclosing span when suggesting dereference on a span that is already a reference: ``` error: non-reference pattern used to match a reference (see issue #42640) --> dont-suggest-dereference-on-arg.rs:16:19 | 16 | .filter(|&(ref a, _)| foo(a)) | ^^^^^^^^^^^ help: consider using: `&&(ref k, _)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable ``` --- src/librustc_typeck/check/_match.rs | 35 ++++++++++++++----- src/libsyntax_pos/lib.rs | 12 +++++++ .../dont-suggest-dereference-on-arg.rs | 18 ++++++++++ .../dont-suggest-dereference-on-arg.stderr | 10 ++++++ 4 files changed, 66 insertions(+), 9 deletions(-) create mode 100644 src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs create mode 100644 src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index ea0fa945c3783..fbfc97d33c5c8 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -23,7 +23,9 @@ use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::cmp; use syntax::ast; use syntax::codemap::Spanned; +use syntax::errors::DiagnosticBuilder; use syntax::feature_gate; +use syntax::parse::ParseSess; use syntax::ptr::P; use syntax_pos::Span; @@ -120,17 +122,32 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { .pat_adjustments_mut() .insert(pat.hir_id, pat_adjustments); } else { - let mut err = feature_gate::feature_err( - &tcx.sess.parse_sess, - "match_default_bindings", - pat.span, - feature_gate::GateIssue::Language, - "non-reference pattern used to match a reference", - ); + fn feature_err<'a>(sp: Span, sess: &'a ParseSess) -> DiagnosticBuilder<'a> { + feature_gate::feature_err( + sess, + "match_default_bindings", + sp, + feature_gate::GateIssue::Language, + "non-reference pattern used to match a reference", + ) + } if let Ok(snippet) = tcx.sess.codemap().span_to_snippet(pat.span) { - err.span_suggestion(pat.span, "consider using", format!("&{}", &snippet)); + // The following is a bit of a hack. We probably should check the AST for + // this instead, but this should be good enough for the expected cases. + let prev_span = pat.span.prev_point(); + let (sp, sugg) = match tcx.sess.codemap().span_to_snippet(prev_span) { + // Make the suggestion more obvious when having `&(_, _)` + Ok(ref prev) if &*prev == "&" => { + (prev_span.to(pat.span), format!("&&{}", &snippet)), + } + _ => (pat.span, format!("&{}", &snippet)), + }; + let mut err = feature_err(sp, &tcx.sess.parse_sess); + err.span_suggestion(sp, "consider using a reference", sugg); + err.emit(); + } else { + feature_err(pat.span, &tcx.sess.parse_sess).emit(); } - err.emit(); } } } diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index 47755dc1d5468..40a7917613f1e 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -159,6 +159,18 @@ impl Span { Span::new(BytePos(lo), BytePos(lo), span.ctxt) } + /// Returns a new span representing the previous character after the start-point of this span + pub fn prev_point(self) -> Span { + let span = self.data(); + let span_lo = span.lo.0; + let lo = if span_lo == 0 { + 0 + } else { + span_lo - 1 + }; + Span::new(BytePos(lo), BytePos(span_lo), span.ctxt) + } + /// Returns `self` if `self` is not the dummy span, and `other` otherwise. pub fn substitute_dummy(self, other: Span) -> Span { if self.source_equal(&DUMMY_SP) { other } else { self } diff --git a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs new file mode 100644 index 0000000000000..2a9ea464416da --- /dev/null +++ b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs @@ -0,0 +1,18 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn foo(s: &str) -> bool { true } + +fn main() { + let x = vec![(String::new(), String::new())]; + x.iter() + .filter(|&(ref a, _)| foo(a)) + .collect(); +} diff --git a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr new file mode 100644 index 0000000000000..fc1eb335d0586 --- /dev/null +++ b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr @@ -0,0 +1,10 @@ +error: non-reference pattern used to match a reference (see issue #42640) + --> dont-suggest-dereference-on-arg.rs:16:19 + | +16 | .filter(|&(ref a, _)| foo(a)) + | ^^^^^^^^^^^ help: consider using: `&&(ref k, _)` + | + = help: add #![feature(match_default_bindings)] to the crate attributes to enable + +error: aborting due to previous error + From 2461b7a264676fbe24800beb0bcb39ab9e853562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 23 Nov 2017 09:58:51 -0800 Subject: [PATCH 2/3] Use `get_parent_node` instead of using spans --- src/librustc_typeck/check/_match.rs | 53 ++++++++++--------- src/libsyntax_pos/lib.rs | 12 ----- .../suggestion.stderr | 2 +- .../dont-suggest-dereference-on-arg.stderr | 4 +- 4 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs index fbfc97d33c5c8..f03f782ebb452 100644 --- a/src/librustc_typeck/check/_match.rs +++ b/src/librustc_typeck/check/_match.rs @@ -23,9 +23,7 @@ use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::cmp; use syntax::ast; use syntax::codemap::Spanned; -use syntax::errors::DiagnosticBuilder; use syntax::feature_gate; -use syntax::parse::ParseSess; use syntax::ptr::P; use syntax_pos::Span; @@ -122,32 +120,35 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { .pat_adjustments_mut() .insert(pat.hir_id, pat_adjustments); } else { - fn feature_err<'a>(sp: Span, sess: &'a ParseSess) -> DiagnosticBuilder<'a> { - feature_gate::feature_err( - sess, - "match_default_bindings", - sp, - feature_gate::GateIssue::Language, - "non-reference pattern used to match a reference", - ) - } - if let Ok(snippet) = tcx.sess.codemap().span_to_snippet(pat.span) { - // The following is a bit of a hack. We probably should check the AST for - // this instead, but this should be good enough for the expected cases. - let prev_span = pat.span.prev_point(); - let (sp, sugg) = match tcx.sess.codemap().span_to_snippet(prev_span) { - // Make the suggestion more obvious when having `&(_, _)` - Ok(ref prev) if &*prev == "&" => { - (prev_span.to(pat.span), format!("&&{}", &snippet)), + let mut ref_sp = pat.span; + let mut id = pat.id; + loop { // make span include all enclosing `&` to avoid confusing diag output + id = tcx.hir.get_parent_node(id); + let node = tcx.hir.find(id); + if let Some(hir::map::NodePat(pat)) = node { + if let hir::PatKind::Ref(..) = pat.node { + ref_sp = pat.span; + } else { + break; } - _ => (pat.span, format!("&{}", &snippet)), - }; - let mut err = feature_err(sp, &tcx.sess.parse_sess); - err.span_suggestion(sp, "consider using a reference", sugg); - err.emit(); - } else { - feature_err(pat.span, &tcx.sess.parse_sess).emit(); + } else { + break; + } + } + let sp = ref_sp.to(pat.span); + let mut err = feature_gate::feature_err( + &tcx.sess.parse_sess, + "match_default_bindings", + sp, + feature_gate::GateIssue::Language, + "non-reference pattern used to match a reference", + ); + if let Ok(snippet) = tcx.sess.codemap().span_to_snippet(sp) { + err.span_suggestion(sp, + "consider using a reference", + format!("&{}", &snippet)); } + err.emit(); } } } diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index 40a7917613f1e..47755dc1d5468 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -159,18 +159,6 @@ impl Span { Span::new(BytePos(lo), BytePos(lo), span.ctxt) } - /// Returns a new span representing the previous character after the start-point of this span - pub fn prev_point(self) -> Span { - let span = self.data(); - let span_lo = span.lo.0; - let lo = if span_lo == 0 { - 0 - } else { - span_lo - 1 - }; - Span::new(BytePos(lo), BytePos(span_lo), span.ctxt) - } - /// Returns `self` if `self` is not the dummy span, and `other` otherwise. pub fn substitute_dummy(self, other: Span) -> Span { if self.source_equal(&DUMMY_SP) { other } else { self } diff --git a/src/test/ui/rfc-2005-default-binding-mode/suggestion.stderr b/src/test/ui/rfc-2005-default-binding-mode/suggestion.stderr index b10980d6bd605..ebf9e498ffd9e 100644 --- a/src/test/ui/rfc-2005-default-binding-mode/suggestion.stderr +++ b/src/test/ui/rfc-2005-default-binding-mode/suggestion.stderr @@ -2,7 +2,7 @@ error: non-reference pattern used to match a reference (see issue #42640) --> $DIR/suggestion.rs:12:12 | 12 | if let Some(y) = &Some(22) { //~ ERROR non-reference pattern - | ^^^^^^^ help: consider using: `&Some(y)` + | ^^^^^^^ help: consider using a reference: `&Some(y)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable diff --git a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr index fc1eb335d0586..56303146d5d2f 100644 --- a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr +++ b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr @@ -1,8 +1,8 @@ error: non-reference pattern used to match a reference (see issue #42640) - --> dont-suggest-dereference-on-arg.rs:16:19 + --> $DIR/dont-suggest-dereference-on-arg.rs:16:19 | 16 | .filter(|&(ref a, _)| foo(a)) - | ^^^^^^^^^^^ help: consider using: `&&(ref k, _)` + | ^^^^^^^^^^^ help: consider using a reference: `&&(ref k, _)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable From 15dfd7eb61a63fff6293617552306c6a4fac862b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 24 Nov 2017 08:27:33 -0800 Subject: [PATCH 3/3] Fix test --- src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs | 3 +++ .../ui/suggestions/dont-suggest-dereference-on-arg.stderr | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs index 2a9ea464416da..72269768e0f5c 100644 --- a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs +++ b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs @@ -14,5 +14,8 @@ fn main() { let x = vec![(String::new(), String::new())]; x.iter() .filter(|&(ref a, _)| foo(a)) + //~^ ERROR non-reference pattern used to match a reference + //~| HELP consider using a reference + //~| HELP add .collect(); } diff --git a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr index 56303146d5d2f..799d9080b9d18 100644 --- a/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr +++ b/src/test/ui/suggestions/dont-suggest-dereference-on-arg.stderr @@ -1,8 +1,8 @@ error: non-reference pattern used to match a reference (see issue #42640) - --> $DIR/dont-suggest-dereference-on-arg.rs:16:19 + --> $DIR/dont-suggest-dereference-on-arg.rs:16:18 | 16 | .filter(|&(ref a, _)| foo(a)) - | ^^^^^^^^^^^ help: consider using a reference: `&&(ref k, _)` + | ^^^^^^^^^^^ help: consider using a reference: `&&(ref a, _)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable