From 4650361fb627f7dd6b8d5c1cee8a7a12a050ba80 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 23 Jun 2018 16:46:41 -0700 Subject: [PATCH 1/3] use structured suggestion for pattern-named-the-same-as-variant warning --- src/librustc_mir/hair/pattern/check_match.rs | 12 +++--- src/test/ui/issue-19100.fixed | 40 ++++++++++++++++++++ src/test/ui/issue-19100.rs | 1 + src/test/ui/issue-19100.stderr | 12 ++---- src/test/ui/issue-30302.stderr | 4 +- 5 files changed, 53 insertions(+), 16 deletions(-) create mode 100644 src/test/ui/issue-19100.fixed diff --git a/src/librustc_mir/hair/pattern/check_match.rs b/src/librustc_mir/hair/pattern/check_match.rs index 2c58bd8e79b08..b96cc352bdb60 100644 --- a/src/librustc_mir/hair/pattern/check_match.rs +++ b/src/librustc_mir/hair/pattern/check_match.rs @@ -23,7 +23,7 @@ use rustc::session::Session; use rustc::ty::{self, Ty, TyCtxt}; use rustc::ty::subst::Substs; use rustc::lint; -use rustc_errors::DiagnosticBuilder; +use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc::util::common::ErrorReported; use rustc::hir::def::*; @@ -328,10 +328,12 @@ fn check_for_bindings_named_the_same_as_variants(cx: &MatchVisitor, pat: &Pat) { "pattern binding `{}` is named the same as one \ of the variants of the type `{}`", name.node, ty_path); - help!(err, - "if you meant to match on a variant, \ - consider making the path in the pattern qualified: `{}::{}`", - ty_path, name.node); + err.span_suggestion_with_applicability( + p.span, + "to match on the variant, qualify the path", + format!("{}::{}", ty_path, name.node), + Applicability::MachineApplicable + ); err.emit(); } } diff --git a/src/test/ui/issue-19100.fixed b/src/test/ui/issue-19100.fixed new file mode 100644 index 0000000000000..3ced913ae11c2 --- /dev/null +++ b/src/test/ui/issue-19100.fixed @@ -0,0 +1,40 @@ +// Copyright 2014 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. + +// run-pass +// run-rustfix + +#![allow(non_snake_case)] +#![allow(dead_code)] +#![allow(unused_variables)] + +#[derive(Copy, Clone)] +enum Foo { + Bar, + Baz +} + +impl Foo { + fn foo(&self) { + match self { + & +Foo::Bar if true +//~^ WARN pattern binding `Bar` is named the same as one of the variants of the type `Foo` +=> println!("bar"), + & +Foo::Baz if false +//~^ WARN pattern binding `Baz` is named the same as one of the variants of the type `Foo` +=> println!("baz"), +_ => () + } + } +} + +fn main() {} diff --git a/src/test/ui/issue-19100.rs b/src/test/ui/issue-19100.rs index 2032f23e6a30e..e073bf9076160 100644 --- a/src/test/ui/issue-19100.rs +++ b/src/test/ui/issue-19100.rs @@ -9,6 +9,7 @@ // except according to those terms. // run-pass +// run-rustfix #![allow(non_snake_case)] #![allow(dead_code)] diff --git a/src/test/ui/issue-19100.stderr b/src/test/ui/issue-19100.stderr index 96835f69b78c7..34dc29c63df72 100644 --- a/src/test/ui/issue-19100.stderr +++ b/src/test/ui/issue-19100.stderr @@ -1,16 +1,12 @@ warning[E0170]: pattern binding `Bar` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-19100.rs:27:1 + --> $DIR/issue-19100.rs:28:1 | LL | Bar if true - | ^^^ - | - = help: if you meant to match on a variant, consider making the path in the pattern qualified: `Foo::Bar` + | ^^^ help: to match on the variant, qualify the path: `Foo::Bar` warning[E0170]: pattern binding `Baz` is named the same as one of the variants of the type `Foo` - --> $DIR/issue-19100.rs:31:1 + --> $DIR/issue-19100.rs:32:1 | LL | Baz if false - | ^^^ - | - = help: if you meant to match on a variant, consider making the path in the pattern qualified: `Foo::Baz` + | ^^^ help: to match on the variant, qualify the path: `Foo::Baz` diff --git a/src/test/ui/issue-30302.stderr b/src/test/ui/issue-30302.stderr index 42dfdadf9c46f..fa3cb92b180e0 100644 --- a/src/test/ui/issue-30302.stderr +++ b/src/test/ui/issue-30302.stderr @@ -2,9 +2,7 @@ warning[E0170]: pattern binding `Nil` is named the same as one of the variants o --> $DIR/issue-30302.rs:23:9 | LL | Nil => true, - | ^^^ - | - = help: if you meant to match on a variant, consider making the path in the pattern qualified: `Stack::Nil` + | ^^^ help: to match on the variant, qualify the path: `Stack::Nil` error: unreachable pattern --> $DIR/issue-30302.rs:25:9 From a417518173bae739d1aef50c6cf1a1e3bd4c4319 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 23 Jun 2018 16:49:09 -0700 Subject: [PATCH 2/3] structured suggestion and rewording for `...` expression syntax error Now that `..=` inclusive ranges are stabilized, people probably shouldn't be using `...` even in patterns, even if it's still legal there (see #51043). To avoid drawing attention to `...` being a real thing, let's reword this message to just say "unexpected token" rather "cannot be used in expressions". --- src/libsyntax/parse/parser.rs | 14 ++++++----- .../parse-fail/range_inclusive_dotdotdot.rs | 25 +++++++++---------- src/test/ui/suggestions/dotdotdot-expr.rs | 14 +++++++++++ src/test/ui/suggestions/dotdotdot-expr.stderr | 16 ++++++++++++ 4 files changed, 50 insertions(+), 19 deletions(-) create mode 100644 src/test/ui/suggestions/dotdotdot-expr.rs create mode 100644 src/test/ui/suggestions/dotdotdot-expr.stderr diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 5970f94b97d52..955bdbdcf9177 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -4800,12 +4800,14 @@ impl<'a> Parser<'a> { fn err_dotdotdot_syntax(&self, span: Span) { self.diagnostic().struct_span_err(span, { - "`...` syntax cannot be used in expressions" - }).help({ - "Use `..` if you need an exclusive range (a < b)" - }).help({ - "or `..=` if you need an inclusive range (a <= b)" - }).emit(); + "unexpected token: `...`" + }).span_suggestion_with_applicability( + span, "use `..` for an exclusive range", "..".to_owned(), + Applicability::MaybeIncorrect + ).span_suggestion_with_applicability( + span, "or `..=` for an inclusive range", "..=".to_owned(), + Applicability::MaybeIncorrect + ).emit(); } // Parse bounds of a type parameter `BOUND + BOUND + BOUND`, possibly with trailing `+`. diff --git a/src/test/parse-fail/range_inclusive_dotdotdot.rs b/src/test/parse-fail/range_inclusive_dotdotdot.rs index fa6474717d3f0..34b96a59c2dde 100644 --- a/src/test/parse-fail/range_inclusive_dotdotdot.rs +++ b/src/test/parse-fail/range_inclusive_dotdotdot.rs @@ -15,22 +15,21 @@ use std::ops::RangeToInclusive; fn return_range_to() -> RangeToInclusive { - return ...1; //~ERROR `...` syntax cannot be used in expressions - //~^HELP Use `..` if you need an exclusive range (a < b) - //~^^HELP or `..=` if you need an inclusive range (a <= b) + return ...1; //~ERROR unexpected token: `...` + //~^HELP use `..` for an exclusive range + //~^^HELP or `..=` for an inclusive range } pub fn main() { - let x = ...0; //~ERROR `...` syntax cannot be used in expressions - //~^HELP Use `..` if you need an exclusive range (a < b) - //~^^HELP or `..=` if you need an inclusive range (a <= b) + let x = ...0; //~ERROR unexpected token: `...` + //~^HELP use `..` for an exclusive range + //~^^HELP or `..=` for an inclusive range - let x = 5...5; //~ERROR `...` syntax cannot be used in expressions - //~^HELP Use `..` if you need an exclusive range (a < b) - //~^^HELP or `..=` if you need an inclusive range (a <= b) + let x = 5...5; //~ERROR unexpected token: `...` + //~^HELP use `..` for an exclusive range + //~^^HELP or `..=` for an inclusive range - for _ in 0...1 {} //~ERROR `...` syntax cannot be used in expressions - //~^HELP Use `..` if you need an exclusive range (a < b) - //~^^HELP or `..=` if you need an inclusive range (a <= b) + for _ in 0...1 {} //~ERROR unexpected token: `...` + //~^HELP use `..` for an exclusive range + //~^^HELP or `..=` for an inclusive range } - diff --git a/src/test/ui/suggestions/dotdotdot-expr.rs b/src/test/ui/suggestions/dotdotdot-expr.rs new file mode 100644 index 0000000000000..afb73a526a8fe --- /dev/null +++ b/src/test/ui/suggestions/dotdotdot-expr.rs @@ -0,0 +1,14 @@ +// Copyright 2018 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 main() { + let _redemptive = 1...21; + //~^ ERROR unexpected token +} diff --git a/src/test/ui/suggestions/dotdotdot-expr.stderr b/src/test/ui/suggestions/dotdotdot-expr.stderr new file mode 100644 index 0000000000000..3315538f2f759 --- /dev/null +++ b/src/test/ui/suggestions/dotdotdot-expr.stderr @@ -0,0 +1,16 @@ +error: unexpected token: `...` + --> $DIR/dotdotdot-expr.rs:12:24 + | +LL | let _redemptive = 1...21; + | ^^^ +help: use `..` for an exclusive range + | +LL | let _redemptive = 1..21; + | ^^ +help: or `..=` for an inclusive range + | +LL | let _redemptive = 1..=21; + | ^^^ + +error: aborting due to previous error + From 0b39a82cf49bc376e568560e4f97360f477255d4 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 23 Jun 2018 16:57:23 -0700 Subject: [PATCH 3/3] in which the trivial-casts word to the wise is tucked into a help note The top level message shouldn't be too long; the replaced-by-coercion/temporary-variable advice can live in a note. Also, don't mention type ascription when it's not actually available as a real thing. (The current state of discussion on the type ascription tracking issue #23416 makes one rather suspect it will never be a stable thing in its current form, but that's not for us to adjudicate in this commit.) While we're here, yank out the differentiating parts of the numeric/other conditional and only have one codepath emitting the diagnostic. --- src/librustc_typeck/check/cast.rs | 41 +++++++++---------- ...trivial-casts-featuring-type-ascription.rs | 20 +++++++++ ...ial-casts-featuring-type-ascription.stderr | 28 +++++++++++++ src/test/ui/lint/trivial-casts.rs | 19 +++++++++ src/test/ui/lint/trivial-casts.stderr | 28 +++++++++++++ 5 files changed, 115 insertions(+), 21 deletions(-) create mode 100644 src/test/ui/lint/trivial-casts-featuring-type-ascription.rs create mode 100644 src/test/ui/lint/trivial-casts-featuring-type-ascription.stderr create mode 100644 src/test/ui/lint/trivial-casts.rs create mode 100644 src/test/ui/lint/trivial-casts.stderr diff --git a/src/librustc_typeck/check/cast.rs b/src/librustc_typeck/check/cast.rs index f0d7ca8ebf14f..07e19c84a9507 100644 --- a/src/librustc_typeck/check/cast.rs +++ b/src/librustc_typeck/check/cast.rs @@ -365,28 +365,27 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> { fn trivial_cast_lint(&self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) { let t_cast = self.cast_ty; let t_expr = self.expr_ty; - if t_cast.is_numeric() && t_expr.is_numeric() { - fcx.tcx.lint_node( - lint::builtin::TRIVIAL_NUMERIC_CASTS, - self.expr.id, - self.span, - &format!("trivial numeric cast: `{}` as `{}`. Cast can be \ - replaced by coercion, this might require type \ - ascription or a temporary variable", - fcx.ty_to_string(t_expr), - fcx.ty_to_string(t_cast))); + let type_asc_or = if fcx.tcx.features().type_ascription { + "type ascription or " } else { - fcx.tcx.lint_node( - lint::builtin::TRIVIAL_CASTS, - self.expr.id, - self.span, - &format!("trivial cast: `{}` as `{}`. Cast can be \ - replaced by coercion, this might require type \ - ascription or a temporary variable", - fcx.ty_to_string(t_expr), - fcx.ty_to_string(t_cast))); - } - + "" + }; + let (adjective, lint) = if t_cast.is_numeric() && t_expr.is_numeric() { + ("numeric ", lint::builtin::TRIVIAL_NUMERIC_CASTS) + } else { + ("", lint::builtin::TRIVIAL_CASTS) + }; + let mut err = fcx.tcx.struct_span_lint_node( + lint, + self.expr.id, + self.span, + &format!("trivial {}cast: `{}` as `{}`", + adjective, + fcx.ty_to_string(t_expr), + fcx.ty_to_string(t_cast))); + err.help(&format!("cast can be replaced by coercion; this might \ + require {}a temporary variable", type_asc_or)); + err.emit(); } pub fn check(mut self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) { diff --git a/src/test/ui/lint/trivial-casts-featuring-type-ascription.rs b/src/test/ui/lint/trivial-casts-featuring-type-ascription.rs new file mode 100644 index 0000000000000..fba3724ae4907 --- /dev/null +++ b/src/test/ui/lint/trivial-casts-featuring-type-ascription.rs @@ -0,0 +1,20 @@ +// Copyright 2018 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. + +#![deny(trivial_casts, trivial_numeric_casts)] +#![feature(type_ascription)] + +fn main() { + let lugubrious = 12i32 as i32; + //~^ ERROR trivial numeric cast + let haunted: &u32 = &99; + let _ = haunted as *const u32; + //~^ ERROR trivial cast +} diff --git a/src/test/ui/lint/trivial-casts-featuring-type-ascription.stderr b/src/test/ui/lint/trivial-casts-featuring-type-ascription.stderr new file mode 100644 index 0000000000000..a77135c875d79 --- /dev/null +++ b/src/test/ui/lint/trivial-casts-featuring-type-ascription.stderr @@ -0,0 +1,28 @@ +error: trivial numeric cast: `i32` as `i32` + --> $DIR/trivial-casts-featuring-type-ascription.rs:15:22 + | +LL | let lugubrious = 12i32 as i32; + | ^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/trivial-casts-featuring-type-ascription.rs:11:24 + | +LL | #![deny(trivial_casts, trivial_numeric_casts)] + | ^^^^^^^^^^^^^^^^^^^^^ + = help: cast can be replaced by coercion; this might require type ascription or a temporary variable + +error: trivial cast: `&u32` as `*const u32` + --> $DIR/trivial-casts-featuring-type-ascription.rs:18:13 + | +LL | let _ = haunted as *const u32; + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/trivial-casts-featuring-type-ascription.rs:11:9 + | +LL | #![deny(trivial_casts, trivial_numeric_casts)] + | ^^^^^^^^^^^^^ + = help: cast can be replaced by coercion; this might require type ascription or a temporary variable + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/lint/trivial-casts.rs b/src/test/ui/lint/trivial-casts.rs new file mode 100644 index 0000000000000..759b282c0da92 --- /dev/null +++ b/src/test/ui/lint/trivial-casts.rs @@ -0,0 +1,19 @@ +// Copyright 2018 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. + +#![deny(trivial_casts, trivial_numeric_casts)] + +fn main() { + let lugubrious = 12i32 as i32; + //~^ ERROR trivial numeric cast + let haunted: &u32 = &99; + let _ = haunted as *const u32; + //~^ ERROR trivial cast +} diff --git a/src/test/ui/lint/trivial-casts.stderr b/src/test/ui/lint/trivial-casts.stderr new file mode 100644 index 0000000000000..d52869f4bed61 --- /dev/null +++ b/src/test/ui/lint/trivial-casts.stderr @@ -0,0 +1,28 @@ +error: trivial numeric cast: `i32` as `i32` + --> $DIR/trivial-casts.rs:14:22 + | +LL | let lugubrious = 12i32 as i32; + | ^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/trivial-casts.rs:11:24 + | +LL | #![deny(trivial_casts, trivial_numeric_casts)] + | ^^^^^^^^^^^^^^^^^^^^^ + = help: cast can be replaced by coercion; this might require a temporary variable + +error: trivial cast: `&u32` as `*const u32` + --> $DIR/trivial-casts.rs:17:13 + | +LL | let _ = haunted as *const u32; + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/trivial-casts.rs:11:9 + | +LL | #![deny(trivial_casts, trivial_numeric_casts)] + | ^^^^^^^^^^^^^ + = help: cast can be replaced by coercion; this might require a temporary variable + +error: aborting due to 2 previous errors +