From 68fa81baa3acf3a93ce4b41c8366039229926fc2 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 24 Jan 2022 09:19:33 +0000 Subject: [PATCH 1/3] rustc_errors: remove `allow_suggestions` from `DiagnosticBuilder`. --- compiler/rustc_errors/src/diagnostic.rs | 38 +++-- .../rustc_errors/src/diagnostic_builder.rs | 138 ++++-------------- compiler/rustc_errors/src/emitter.rs | 7 +- compiler/rustc_errors/src/json.rs | 2 +- compiler/rustc_middle/src/lint.rs | 2 +- compiler/rustc_typeck/src/collect/type_of.rs | 5 +- 6 files changed, 64 insertions(+), 128 deletions(-) diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs index e5116cd8dfe..8cfecafd20c 100644 --- a/compiler/rustc_errors/src/diagnostic.rs +++ b/compiler/rustc_errors/src/diagnostic.rs @@ -11,6 +11,11 @@ use rustc_span::{MultiSpan, Span, DUMMY_SP}; use std::fmt; use std::hash::{Hash, Hasher}; +/// Error type for `Diagnostic`'s `suggestions` field, indicating that +/// `.disable_suggestions()` was called on the `Diagnostic`. +#[derive(Clone, Debug, PartialEq, Eq, Hash, Encodable, Decodable)] +pub struct SuggestionsDisabled; + #[must_use] #[derive(Clone, Debug, Encodable, Decodable)] pub struct Diagnostic { @@ -19,7 +24,7 @@ pub struct Diagnostic { pub code: Option, pub span: MultiSpan, pub children: Vec, - pub suggestions: Vec, + pub suggestions: Result, SuggestionsDisabled>, /// This is not used for highlighting or rendering any error message. Rather, it can be used /// as a sort key to sort a buffer of diagnostics. By default, it is the primary span of @@ -106,7 +111,7 @@ impl Diagnostic { code, span: MultiSpan::new(), children: vec![], - suggestions: vec![], + suggestions: Ok(vec![]), sort_span: DUMMY_SP, is_lint: false, } @@ -300,6 +305,21 @@ impl Diagnostic { self } + /// Disallow attaching suggestions this diagnostic. + /// Any suggestions attached e.g. with the `span_suggestion_*` methods + /// (before and after the call to `disable_suggestions`) will be ignored. + pub fn disable_suggestions(&mut self) -> &mut Self { + self.suggestions = Err(SuggestionsDisabled); + self + } + + /// Helper for pushing to `self.suggestions`, if available (not disable). + fn push_suggestion(&mut self, suggestion: CodeSuggestion) { + if let Ok(suggestions) = &mut self.suggestions { + suggestions.push(suggestion); + } + } + /// Show a suggestion that has multiple parts to it. /// In other words, multiple changes need to be applied as part of this suggestion. pub fn multipart_suggestion( @@ -340,7 +360,7 @@ impl Diagnostic { style: SuggestionStyle, ) -> &mut Self { assert!(!suggestion.is_empty()); - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions: vec![Substitution { parts: suggestion .into_iter() @@ -368,7 +388,7 @@ impl Diagnostic { applicability: Applicability, ) -> &mut Self { assert!(!suggestion.is_empty()); - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions: vec![Substitution { parts: suggestion .into_iter() @@ -426,7 +446,7 @@ impl Diagnostic { applicability: Applicability, style: SuggestionStyle, ) -> &mut Self { - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions: vec![Substitution { parts: vec![SubstitutionPart { snippet: suggestion, span: sp }], }], @@ -471,7 +491,7 @@ impl Diagnostic { .into_iter() .map(|snippet| Substitution { parts: vec![SubstitutionPart { snippet, span: sp }] }) .collect(); - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions, msg: msg.to_owned(), style: SuggestionStyle::ShowCode, @@ -489,7 +509,7 @@ impl Diagnostic { suggestions: impl Iterator>, applicability: Applicability, ) -> &mut Self { - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions: suggestions .map(|sugg| Substitution { parts: sugg @@ -578,7 +598,7 @@ impl Diagnostic { applicability: Applicability, tool_metadata: Json, ) { - self.suggestions.push(CodeSuggestion { + self.push_suggestion(CodeSuggestion { substitutions: vec![], msg: msg.to_owned(), style: SuggestionStyle::CompletelyHidden, @@ -668,7 +688,7 @@ impl Diagnostic { &Vec<(String, Style)>, &Option, &MultiSpan, - &Vec, + &Result, SuggestionsDisabled>, Option<&Vec>, ) { ( diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs index 6f84b0d400e..200c7cbccd9 100644 --- a/compiler/rustc_errors/src/diagnostic_builder.rs +++ b/compiler/rustc_errors/src/diagnostic_builder.rs @@ -22,12 +22,13 @@ pub struct DiagnosticBuilder<'a>(Box>); /// (RVO) should avoid unnecessary copying. In practice, it does not (at the /// time of writing). The split between `DiagnosticBuilder` and /// `DiagnosticBuilderInner` exists to avoid many `memcpy` calls. +// FIXME(eddyb) try having two pointers in `DiagnosticBuilder`, by only boxing +// `Diagnostic` (i.e. `struct DiagnosticBuilder(&Handler, Box);`). #[must_use] #[derive(Clone)] struct DiagnosticBuilderInner<'a> { handler: &'a Handler, diagnostic: Diagnostic, - allow_suggestions: bool, } /// In general, the `DiagnosticBuilder` uses deref to allow access to @@ -244,164 +245,79 @@ impl<'a> DiagnosticBuilder<'a> { ) -> &mut Self); forward!(pub fn set_is_lint(&mut self,) -> &mut Self); - /// See [`Diagnostic::multipart_suggestion()`]. - pub fn multipart_suggestion( + forward!(pub fn disable_suggestions(&mut self,) -> &mut Self); + + forward!(pub fn multipart_suggestion( &mut self, msg: &str, suggestion: Vec<(Span, String)>, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.multipart_suggestion(msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::multipart_suggestion()`]. - pub fn multipart_suggestion_verbose( + ) -> &mut Self); + forward!(pub fn multipart_suggestion_verbose( &mut self, msg: &str, suggestion: Vec<(Span, String)>, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.multipart_suggestion_verbose(msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::tool_only_multipart_suggestion()`]. - pub fn tool_only_multipart_suggestion( + ) -> &mut Self); + forward!(pub fn tool_only_multipart_suggestion( &mut self, msg: &str, suggestion: Vec<(Span, String)>, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.tool_only_multipart_suggestion(msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::span_suggestion()`]. - pub fn span_suggestion( + ) -> &mut Self); + forward!(pub fn span_suggestion( &mut self, sp: Span, msg: &str, suggestion: String, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.span_suggestion(sp, msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::span_suggestions()`]. - pub fn span_suggestions( + ) -> &mut Self); + forward!(pub fn span_suggestions( &mut self, sp: Span, msg: &str, suggestions: impl Iterator, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.span_suggestions(sp, msg, suggestions, applicability); - self - } - - /// See [`Diagnostic::multipart_suggestions()`]. - pub fn multipart_suggestions( + ) -> &mut Self); + forward!(pub fn multipart_suggestions( &mut self, msg: &str, suggestions: impl Iterator>, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.multipart_suggestions(msg, suggestions, applicability); - self - } - - /// See [`Diagnostic::span_suggestion_short()`]. - pub fn span_suggestion_short( + ) -> &mut Self); + forward!(pub fn span_suggestion_short( &mut self, sp: Span, msg: &str, suggestion: String, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.span_suggestion_short(sp, msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::span_suggestion_verbose()`]. - pub fn span_suggestion_verbose( + ) -> &mut Self); + forward!(pub fn span_suggestion_verbose( &mut self, sp: Span, msg: &str, suggestion: String, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.span_suggestion_verbose(sp, msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::span_suggestion_hidden()`]. - pub fn span_suggestion_hidden( + ) -> &mut Self); + forward!(pub fn span_suggestion_hidden( &mut self, sp: Span, msg: &str, suggestion: String, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.span_suggestion_hidden(sp, msg, suggestion, applicability); - self - } - - /// See [`Diagnostic::tool_only_span_suggestion()`] for more information. - pub fn tool_only_span_suggestion( + ) -> &mut Self); + forward!(pub fn tool_only_span_suggestion( &mut self, sp: Span, msg: &str, suggestion: String, applicability: Applicability, - ) -> &mut Self { - if !self.0.allow_suggestions { - return self; - } - self.0.diagnostic.tool_only_span_suggestion(sp, msg, suggestion, applicability); - self - } + ) -> &mut Self); forward!(pub fn set_primary_message>(&mut self, msg: M) -> &mut Self); forward!(pub fn set_span>(&mut self, sp: S) -> &mut Self); forward!(pub fn code(&mut self, s: DiagnosticId) -> &mut Self); - /// Allow attaching suggestions this diagnostic. - /// If this is set to `false`, then any suggestions attached with the `span_suggestion_*` - /// methods after this is set to `false` will be ignored. - pub fn allow_suggestions(&mut self, allow: bool) -> &mut Self { - self.0.allow_suggestions = allow; - self - } - /// Convenience function for internal use, clients should use one of the /// `struct_*` methods on [`Handler`]. crate fn new(handler: &'a Handler, level: Level, message: &str) -> DiagnosticBuilder<'a> { @@ -424,11 +340,7 @@ impl<'a> DiagnosticBuilder<'a> { /// diagnostic. crate fn new_diagnostic(handler: &'a Handler, diagnostic: Diagnostic) -> DiagnosticBuilder<'a> { debug!("Created new diagnostic"); - DiagnosticBuilder(Box::new(DiagnosticBuilderInner { - handler, - diagnostic, - allow_suggestions: true, - })) + DiagnosticBuilder(Box::new(DiagnosticBuilderInner { handler, diagnostic })) } } diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 3104bc185e7..f90f4d46a9a 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -227,7 +227,8 @@ pub trait Emitter { diag: &'a Diagnostic, ) -> (MultiSpan, &'a [CodeSuggestion]) { let mut primary_span = diag.span.clone(); - if let Some((sugg, rest)) = diag.suggestions.split_first() { + let suggestions = diag.suggestions.as_ref().map_or(&[][..], |suggestions| &suggestions[..]); + if let Some((sugg, rest)) = suggestions.split_first() { if rest.is_empty() && // ^ if there is only one suggestion // don't display multi-suggestions as labels @@ -282,10 +283,10 @@ pub trait Emitter { // to be consistent. We could try to figure out if we can // make one (or the first one) inline, but that would give // undue importance to a semi-random suggestion - (primary_span, &diag.suggestions) + (primary_span, suggestions) } } else { - (primary_span, &diag.suggestions) + (primary_span, suggestions) } } diff --git a/compiler/rustc_errors/src/json.rs b/compiler/rustc_errors/src/json.rs index c2af2b2a86d..ff3478073d9 100644 --- a/compiler/rustc_errors/src/json.rs +++ b/compiler/rustc_errors/src/json.rs @@ -345,7 +345,7 @@ struct UnusedExterns<'a, 'b, 'c> { impl Diagnostic { fn from_errors_diagnostic(diag: &crate::Diagnostic, je: &JsonEmitter) -> Diagnostic { - let sugg = diag.suggestions.iter().map(|sugg| Diagnostic { + let sugg = diag.suggestions.iter().flatten().map(|sugg| Diagnostic { message: sugg.msg.clone(), code: None, level: "help", diff --git a/compiler/rustc_middle/src/lint.rs b/compiler/rustc_middle/src/lint.rs index eef10356ed2..c6226c69f30 100644 --- a/compiler/rustc_middle/src/lint.rs +++ b/compiler/rustc_middle/src/lint.rs @@ -262,7 +262,7 @@ pub fn struct_lint_level<'s, 'd>( if err.span.primary_spans().iter().any(|s| in_external_macro(sess, *s)) { // Any suggestions made here are likely to be incorrect, so anything we // emit shouldn't be automatically fixed by rustfix. - err.allow_suggestions(false); + err.disable_suggestions(); // If this is a future incompatible that is not an edition fixing lint // it'll become a hard error, so we have to emit *something*. Also, diff --git a/compiler/rustc_typeck/src/collect/type_of.rs b/compiler/rustc_typeck/src/collect/type_of.rs index 63020b7f90f..b99e44f2105 100644 --- a/compiler/rustc_typeck/src/collect/type_of.rs +++ b/compiler/rustc_typeck/src/collect/type_of.rs @@ -726,7 +726,10 @@ fn infer_placeholder_type<'a>( if !ty.references_error() { // The parser provided a sub-optimal `HasPlaceholders` suggestion for the type. // We are typeck and have the real type, so remove that and suggest the actual type. - err.suggestions.clear(); + // FIXME(eddyb) this looks like it should be functionality on `Diagnostic`. + if let Ok(suggestions) = &mut err.suggestions { + suggestions.clear(); + } // Suggesting unnameable types won't help. let mut mk_nameable = MakeNameable::new(tcx); From a8dfa3757cddd958c7dcadeeafd62721c45a487e Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 24 Jan 2022 11:23:14 +0000 Subject: [PATCH 2/3] rustc_errors: only box the `diagnostic` field in `DiagnosticBuilder`. --- .../rustc_errors/src/diagnostic_builder.rs | 50 ++++++++----------- compiler/rustc_errors/src/lib.rs | 4 +- src/tools/rustfmt/src/parse/session.rs | 2 +- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs index 200c7cbccd9..3c8751a7a35 100644 --- a/compiler/rustc_errors/src/diagnostic_builder.rs +++ b/compiler/rustc_errors/src/diagnostic_builder.rs @@ -15,20 +15,14 @@ use tracing::debug; /// extending `HandlerFlags`, accessed via `self.handler.flags`. #[must_use] #[derive(Clone)] -pub struct DiagnosticBuilder<'a>(Box>); - -/// This is a large type, and often used as a return value, especially within -/// the frequently-used `PResult` type. In theory, return value optimization -/// (RVO) should avoid unnecessary copying. In practice, it does not (at the -/// time of writing). The split between `DiagnosticBuilder` and -/// `DiagnosticBuilderInner` exists to avoid many `memcpy` calls. -// FIXME(eddyb) try having two pointers in `DiagnosticBuilder`, by only boxing -// `Diagnostic` (i.e. `struct DiagnosticBuilder(&Handler, Box);`). -#[must_use] -#[derive(Clone)] -struct DiagnosticBuilderInner<'a> { +pub struct DiagnosticBuilder<'a> { handler: &'a Handler, - diagnostic: Diagnostic, + + /// `Diagnostic` is a large type, and `DiagnosticBuilder` is often used as a + /// return value, especially within the frequently-used `PResult` type. + /// In theory, return value optimization (RVO) should avoid unnecessary + /// copying. In practice, it does not (at the time of writing). + diagnostic: Box, } /// In general, the `DiagnosticBuilder` uses deref to allow access to @@ -61,7 +55,7 @@ macro_rules! forward { $(#[$attrs])* #[doc = concat!("See [`Diagnostic::", stringify!($n), "()`].")] pub fn $n(&mut self, $($name: $ty),*) -> &mut Self { - self.0.diagnostic.$n($($name),*); + self.diagnostic.$n($($name),*); self } }; @@ -78,7 +72,7 @@ macro_rules! forward { $(#[$attrs])* #[doc = concat!("See [`Diagnostic::", stringify!($n), "()`].")] pub fn $n<$($generic: $bound),*>(&mut self, $($name: $ty),*) -> &mut Self { - self.0.diagnostic.$n($($name),*); + self.diagnostic.$n($($name),*); self } }; @@ -88,20 +82,20 @@ impl<'a> Deref for DiagnosticBuilder<'a> { type Target = Diagnostic; fn deref(&self) -> &Diagnostic { - &self.0.diagnostic + &self.diagnostic } } impl<'a> DerefMut for DiagnosticBuilder<'a> { fn deref_mut(&mut self) -> &mut Diagnostic { - &mut self.0.diagnostic + &mut self.diagnostic } } impl<'a> DiagnosticBuilder<'a> { /// Emit the diagnostic. pub fn emit(&mut self) { - self.0.handler.emit_diagnostic(&self); + self.handler.emit_diagnostic(&self); self.cancel(); } @@ -131,19 +125,19 @@ impl<'a> DiagnosticBuilder<'a> { /// Converts the builder to a `Diagnostic` for later emission, /// unless handler has disabled such buffering. pub fn into_diagnostic(mut self) -> Option<(Diagnostic, &'a Handler)> { - if self.0.handler.flags.dont_buffer_diagnostics - || self.0.handler.flags.treat_err_as_bug.is_some() + if self.handler.flags.dont_buffer_diagnostics + || self.handler.flags.treat_err_as_bug.is_some() { self.emit(); return None; } - let handler = self.0.handler; + let handler = self.handler; // We must use `Level::Cancelled` for `dummy` to avoid an ICE about an // unused diagnostic. let dummy = Diagnostic::new(Level::Cancelled, ""); - let diagnostic = std::mem::replace(&mut self.0.diagnostic, dummy); + let diagnostic = std::mem::replace(&mut *self.diagnostic, dummy); // Logging here is useful to help track down where in logs an error was // actually emitted. @@ -170,7 +164,7 @@ impl<'a> DiagnosticBuilder<'a> { /// locally in whichever way makes the most sense. pub fn delay_as_bug(&mut self) { self.level = Level::Bug; - self.0.handler.delay_as_bug(self.0.diagnostic.clone()); + self.handler.delay_as_bug((*self.diagnostic).clone()); self.cancel(); } @@ -187,7 +181,7 @@ impl<'a> DiagnosticBuilder<'a> { /// ["primary span"][`MultiSpan`]; only the `Span` supplied when creating the diagnostic is /// primary. pub fn span_label(&mut self, span: Span, label: impl Into) -> &mut Self { - self.0.diagnostic.span_label(span, label); + self.diagnostic.span_label(span, label); self } @@ -200,7 +194,7 @@ impl<'a> DiagnosticBuilder<'a> { ) -> &mut Self { let label = label.as_ref(); for span in spans { - self.0.diagnostic.span_label(span, label); + self.diagnostic.span_label(span, label); } self } @@ -340,13 +334,13 @@ impl<'a> DiagnosticBuilder<'a> { /// diagnostic. crate fn new_diagnostic(handler: &'a Handler, diagnostic: Diagnostic) -> DiagnosticBuilder<'a> { debug!("Created new diagnostic"); - DiagnosticBuilder(Box::new(DiagnosticBuilderInner { handler, diagnostic })) + DiagnosticBuilder { handler, diagnostic: Box::new(diagnostic) } } } impl<'a> Debug for DiagnosticBuilder<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.diagnostic.fmt(f) + self.diagnostic.fmt(f) } } @@ -356,7 +350,7 @@ impl<'a> Drop for DiagnosticBuilder<'a> { fn drop(&mut self) { if !panicking() && !self.cancelled() { let mut db = DiagnosticBuilder::new( - self.0.handler, + self.handler, Level::Bug, "the following error was constructed but not emitted", ); diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 16e9b265d69..180e1aca693 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -54,9 +54,9 @@ pub use snippet::Style; pub type PResult<'a, T> = Result>; // `PResult` is used a lot. Make sure it doesn't unintentionally get bigger. -// (See also the comment on `DiagnosticBuilderInner`.) +// (See also the comment on `DiagnosticBuilder`'s `diagnostic` field.) #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] -rustc_data_structures::static_assert_size!(PResult<'_, bool>, 16); +rustc_data_structures::static_assert_size!(PResult<'_, bool>, 24); #[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Encodable, Decodable)] pub enum SuggestionStyle { diff --git a/src/tools/rustfmt/src/parse/session.rs b/src/tools/rustfmt/src/parse/session.rs index 624fed0d2de..7fc3778376c 100644 --- a/src/tools/rustfmt/src/parse/session.rs +++ b/src/tools/rustfmt/src/parse/session.rs @@ -315,7 +315,7 @@ mod tests { code: None, message: vec![], children: vec![], - suggestions: vec![], + suggestions: Ok(vec![]), span: span.unwrap_or_else(MultiSpan::new), sort_span: DUMMY_SP, is_lint: false, From f5a32711dc14ea66510bd5c8a21763183ee5fc99 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 24 Jan 2022 15:14:40 +0000 Subject: [PATCH 3/3] rustc_errors: add a new assert for the size of `PResult<()>`. --- compiler/rustc_errors/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 180e1aca693..7582f317b85 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -56,6 +56,8 @@ pub type PResult<'a, T> = Result>; // `PResult` is used a lot. Make sure it doesn't unintentionally get bigger. // (See also the comment on `DiagnosticBuilder`'s `diagnostic` field.) #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] +rustc_data_structures::static_assert_size!(PResult<'_, ()>, 16); +#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] rustc_data_structures::static_assert_size!(PResult<'_, bool>, 24); #[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Encodable, Decodable)]