From 28d0312b7d47b8bc8fcb4888ca627231715db3cd Mon Sep 17 00:00:00 2001 From: Kevin Per Date: Tue, 11 Oct 2022 16:17:59 +0000 Subject: [PATCH] Implement assertions and fixes to not emit empty spans without suggestions --- compiler/rustc_errors/src/diagnostic.rs | 25 +++++++++ compiler/rustc_expand/src/base.rs | 5 +- .../rustc_hir_analysis/src/astconv/mod.rs | 25 +++++---- .../rustc_parse/src/parser/diagnostics.rs | 10 +++- .../clippy/clippy_lints/src/manual_assert.rs | 12 ++-- .../clippy_lints/src/needless_late_init.rs | 11 ++-- .../tests/ui/manual_assert.edition2018.stderr | 55 +++---------------- .../tests/ui/manual_assert.edition2021.stderr | 55 +++---------------- 8 files changed, 81 insertions(+), 117 deletions(-) diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs index 518c59dba5366..a63fc0ca285da 100644 --- a/compiler/rustc_errors/src/diagnostic.rs +++ b/compiler/rustc_errors/src/diagnostic.rs @@ -567,6 +567,11 @@ impl Diagnostic { style: SuggestionStyle, ) -> &mut Self { assert!(!suggestion.is_empty()); + debug_assert!( + !(suggestion.iter().any(|(sp, text)| sp.is_empty() && text.is_empty())), + "Span must not be empty and have no suggestion" + ); + self.push_suggestion(CodeSuggestion { substitutions: vec![Substitution { parts: suggestion @@ -644,6 +649,10 @@ impl Diagnostic { applicability: Applicability, style: SuggestionStyle, ) -> &mut Self { + debug_assert!( + !(sp.is_empty() && suggestion.to_string().is_empty()), + "Span must not be empty and have no suggestion" + ); self.push_suggestion(CodeSuggestion { substitutions: vec![Substitution { parts: vec![SubstitutionPart { snippet: suggestion.to_string(), span: sp }], @@ -684,6 +693,12 @@ impl Diagnostic { ) -> &mut Self { let mut suggestions: Vec<_> = suggestions.collect(); suggestions.sort(); + + debug_assert!( + !(sp.is_empty() && suggestions.iter().any(|suggestion| suggestion.is_empty())), + "Span must not be empty and have no suggestion" + ); + let substitutions = suggestions .into_iter() .map(|snippet| Substitution { parts: vec![SubstitutionPart { snippet, span: sp }] }) @@ -705,8 +720,18 @@ impl Diagnostic { suggestions: impl Iterator>, applicability: Applicability, ) -> &mut Self { + let suggestions: Vec<_> = suggestions.collect(); + debug_assert!( + !(suggestions + .iter() + .flat_map(|suggs| suggs) + .any(|(sp, suggestion)| sp.is_empty() && suggestion.is_empty())), + "Span must not be empty and have no suggestion" + ); + self.push_suggestion(CodeSuggestion { substitutions: suggestions + .into_iter() .map(|sugg| Substitution { parts: sugg .into_iter() diff --git a/compiler/rustc_expand/src/base.rs b/compiler/rustc_expand/src/base.rs index cd8a525e062c5..052ca229f0127 100644 --- a/compiler/rustc_expand/src/base.rs +++ b/compiler/rustc_expand/src/base.rs @@ -22,7 +22,7 @@ use rustc_span::edition::Edition; use rustc_span::hygiene::{AstPass, ExpnData, ExpnKind, LocalExpnId}; use rustc_span::source_map::SourceMap; use rustc_span::symbol::{kw, sym, Ident, Symbol}; -use rustc_span::{FileName, Span, DUMMY_SP}; +use rustc_span::{BytePos, FileName, Span, DUMMY_SP}; use smallvec::{smallvec, SmallVec}; use std::default::Default; @@ -1228,8 +1228,9 @@ pub fn expr_to_spanned_string<'a>( ast::LitKind::Str(s, style) => return Ok((s, style, expr.span)), ast::LitKind::ByteStr(_) => { let mut err = cx.struct_span_err(l.span, err_msg); + let span = expr.span.shrink_to_lo(); err.span_suggestion( - expr.span.shrink_to_lo(), + span.with_hi(span.lo() + BytePos(1)), "consider removing the leading `b`", "", Applicability::MaybeIncorrect, diff --git a/compiler/rustc_hir_analysis/src/astconv/mod.rs b/compiler/rustc_hir_analysis/src/astconv/mod.rs index 6e373e41b4c02..a8f70ba2b3780 100644 --- a/compiler/rustc_hir_analysis/src/astconv/mod.rs +++ b/compiler/rustc_hir_analysis/src/astconv/mod.rs @@ -3051,24 +3051,27 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { .map_or(false, |s| s.trim_end().ends_with('<')); let is_global = poly_trait_ref.trait_ref.path.is_global(); - let sugg = Vec::from_iter([ - ( - self_ty.span.shrink_to_lo(), - format!( - "{}dyn {}", - if needs_bracket { "<" } else { "" }, - if is_global { "(" } else { "" }, - ), + + let mut sugg = Vec::from_iter([( + self_ty.span.shrink_to_lo(), + format!( + "{}dyn {}", + if needs_bracket { "<" } else { "" }, + if is_global { "(" } else { "" }, ), - ( + )]); + + if is_global || needs_bracket { + sugg.push(( self_ty.span.shrink_to_hi(), format!( "{}{}", if is_global { ")" } else { "" }, if needs_bracket { ">" } else { "" }, ), - ), - ]); + )); + } + if self_ty.span.edition() >= Edition::Edition2021 { let msg = "trait objects must include the `dyn` keyword"; let label = "add `dyn` keyword before this trait"; diff --git a/compiler/rustc_parse/src/parser/diagnostics.rs b/compiler/rustc_parse/src/parser/diagnostics.rs index 828b7d2f2f7be..7e11ca57cebda 100644 --- a/compiler/rustc_parse/src/parser/diagnostics.rs +++ b/compiler/rustc_parse/src/parser/diagnostics.rs @@ -1374,9 +1374,17 @@ impl<'a> Parser<'a> { kind: IncDecRecovery, (pre_span, post_span): (Span, Span), ) -> MultiSugg { + let mut patches = Vec::new(); + + if !pre_span.is_empty() { + patches.push((pre_span, String::new())); + } + + patches.push((post_span, format!(" {}= 1", kind.op.chr()))); + MultiSugg { msg: format!("use `{}= 1` instead", kind.op.chr()), - patches: vec![(pre_span, String::new()), (post_span, format!(" {}= 1", kind.op.chr()))], + patches, applicability: Applicability::MachineApplicable, } } diff --git a/src/tools/clippy/clippy_lints/src/manual_assert.rs b/src/tools/clippy/clippy_lints/src/manual_assert.rs index 825ec84b4a812..b8ed9b9ec18f7 100644 --- a/src/tools/clippy/clippy_lints/src/manual_assert.rs +++ b/src/tools/clippy/clippy_lints/src/manual_assert.rs @@ -69,11 +69,13 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert { "only a `panic!` in `if`-then statement", |diag| { // comments can be noisy, do not show them to the user - diag.tool_only_span_suggestion( - expr.span.shrink_to_lo(), - "add comments back", - comments, - applicability); + if !comments.is_empty() { + diag.tool_only_span_suggestion( + expr.span.shrink_to_lo(), + "add comments back", + comments, + applicability); + } diag.span_suggestion( expr.span, "try instead", diff --git a/src/tools/clippy/clippy_lints/src/needless_late_init.rs b/src/tools/clippy/clippy_lints/src/needless_late_init.rs index 9d26e5900866c..67debe7e08af6 100644 --- a/src/tools/clippy/clippy_lints/src/needless_late_init.rs +++ b/src/tools/clippy/clippy_lints/src/needless_late_init.rs @@ -180,10 +180,13 @@ fn assignment_suggestions<'tcx>( let suggestions = assignments .iter() .flat_map(|assignment| { - [ - assignment.span.until(assignment.rhs_span), - assignment.rhs_span.shrink_to_hi().with_hi(assignment.span.hi()), - ] + let mut spans = vec![assignment.span.until(assignment.rhs_span)]; + + if assignment.rhs_span.hi() != assignment.span.hi() { + spans.push(assignment.rhs_span.shrink_to_hi().with_hi(assignment.span.hi())); + } + + spans }) .map(|span| (span, String::new())) .collect::>(); diff --git a/src/tools/clippy/tests/ui/manual_assert.edition2018.stderr b/src/tools/clippy/tests/ui/manual_assert.edition2018.stderr index 7718588fdf6f4..237638ee1344c 100644 --- a/src/tools/clippy/tests/ui/manual_assert.edition2018.stderr +++ b/src/tools/clippy/tests/ui/manual_assert.edition2018.stderr @@ -4,13 +4,9 @@ error: only a `panic!` in `if`-then statement LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ + | |_____^ help: try instead: `assert!(a.is_empty(), "qaqaq{:?}", a);` | = note: `-D clippy::manual-assert` implied by `-D warnings` -help: try instead - | -LL | assert!(a.is_empty(), "qaqaq{:?}", a); - | error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:34:5 @@ -18,12 +14,7 @@ error: only a `panic!` in `if`-then statement LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(a.is_empty(), "qwqwq"); - | + | |_____^ help: try instead: `assert!(a.is_empty(), "qwqwq");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:51:5 @@ -31,12 +22,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!b.is_empty(), "panic1"); - | + | |_____^ help: try instead: `assert!(!b.is_empty(), "panic1");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:54:5 @@ -44,12 +30,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(b.is_empty() && a.is_empty()), "panic2"); - | + | |_____^ help: try instead: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:57:5 @@ -57,12 +38,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3"); - | + | |_____^ help: try instead: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:60:5 @@ -70,12 +46,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(b.is_empty() || a.is_empty()), "panic4"); - | + | |_____^ help: try instead: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:63:5 @@ -83,12 +54,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5"); - | + | |_____^ help: try instead: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:66:5 @@ -96,12 +62,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!a.is_empty(), "with expansion {}", one!()); - | + | |_____^ help: try instead: `assert!(!a.is_empty(), "with expansion {}", one!());` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:73:5 diff --git a/src/tools/clippy/tests/ui/manual_assert.edition2021.stderr b/src/tools/clippy/tests/ui/manual_assert.edition2021.stderr index 7718588fdf6f4..237638ee1344c 100644 --- a/src/tools/clippy/tests/ui/manual_assert.edition2021.stderr +++ b/src/tools/clippy/tests/ui/manual_assert.edition2021.stderr @@ -4,13 +4,9 @@ error: only a `panic!` in `if`-then statement LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ + | |_____^ help: try instead: `assert!(a.is_empty(), "qaqaq{:?}", a);` | = note: `-D clippy::manual-assert` implied by `-D warnings` -help: try instead - | -LL | assert!(a.is_empty(), "qaqaq{:?}", a); - | error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:34:5 @@ -18,12 +14,7 @@ error: only a `panic!` in `if`-then statement LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(a.is_empty(), "qwqwq"); - | + | |_____^ help: try instead: `assert!(a.is_empty(), "qwqwq");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:51:5 @@ -31,12 +22,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!b.is_empty(), "panic1"); - | + | |_____^ help: try instead: `assert!(!b.is_empty(), "panic1");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:54:5 @@ -44,12 +30,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(b.is_empty() && a.is_empty()), "panic2"); - | + | |_____^ help: try instead: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:57:5 @@ -57,12 +38,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3"); - | + | |_____^ help: try instead: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:60:5 @@ -70,12 +46,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(b.is_empty() || a.is_empty()), "panic4"); - | + | |_____^ help: try instead: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:63:5 @@ -83,12 +54,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5"); - | + | |_____^ help: try instead: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:66:5 @@ -96,12 +62,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ - | -help: try instead - | -LL | assert!(!a.is_empty(), "with expansion {}", one!()); - | + | |_____^ help: try instead: `assert!(!a.is_empty(), "with expansion {}", one!());` error: only a `panic!` in `if`-then statement --> $DIR/manual_assert.rs:73:5