Auto merge of #93259 - eddyb:diagbld-scalar-pair, r=jackh726

rustc_errors: only box the `diagnostic` field in `DiagnosticBuilder`.

I happened to need to do the first change (replacing `allow_suggestions` with equivalent functionality on `Diagnostic` itself) as part of a larger change, and noticed that there's only two fields left in `DiagnosticBuilderInner`.

So with this PR, instead of a single pointer, `DiagnosticBuilder` is two pointers, which should work just as well for passing *it* by value (and may even work better wrt some operations, though probably not by much).

But anything that was already taking advantage of `DiagnosticBuilder` being a single pointer, and wrapping it further (e.g. `Result<T, DiagnosticBuilder>` w/ non-ZST `T`), ~~will probably see a slowdown~~, so I want to do a perf run before even trying to propose this.

Author: bors

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<DiagnosticId>,
     pub span: MultiSpan,
     pub children: Vec<SubDiagnostic>,
-    pub suggestions: Vec<CodeSuggestion>,
+    pub suggestions: Result<Vec<CodeSuggestion>, 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<Item = Vec<(Span, String)>>,
         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<DiagnosticId>,
         &MultiSpan,
-        &Vec<CodeSuggestion>,
+        &Result<Vec<CodeSuggestion>, SuggestionsDisabled>,
         Option<&Vec<SubDiagnostic>>,
     ) {
         (

compiler/rustc_errors/src/diagnostic_builder.rs

@@ -15,19 +15,14 @@ use tracing::debug;
 /// extending `HandlerFlags`, accessed via `self.handler.flags`.
 #[must_use]
 #[derive(Clone)]
-pub struct DiagnosticBuilder<'a>(Box<DiagnosticBuilderInner<'a>>);
-
-/// 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.
-#[must_use]
-#[derive(Clone)]
-struct DiagnosticBuilderInner<'a> {
+pub struct DiagnosticBuilder<'a> {
     handler: &'a Handler,
-    diagnostic: Diagnostic,
-    allow_suggestions: bool,
+
+    /// `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<Diagnostic>,
 }
 
 /// In general, the `DiagnosticBuilder` uses deref to allow access to
@@ -60,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
         }
     };
@@ -77,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
         }
     };
@@ -87,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();
     }
 
@@ -130,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.
@@ -169,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();
     }
 
@@ -186,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<String>) -> &mut Self {
-        self.0.diagnostic.span_label(span, label);
+        self.diagnostic.span_label(span, label);
         self
     }
 
@@ -199,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
     }
@@ -244,164 +239,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<Item = String>,
         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<Item = Vec<(Span, String)>>,
         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<M: Into<String>>(&mut self, msg: M) -> &mut Self);
     forward!(pub fn set_span<S: Into<MultiSpan>>(&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,17 +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,
-            allow_suggestions: true,
-        }))
+        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)
     }
 }
 
@@ -444,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",
             );

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)
         }
     }
 

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",

compiler/rustc_errors/src/lib.rs

@@ -54,9 +54,11 @@ pub use snippet::Style;
 pub type PResult<'a, T> = Result<T, DiagnosticBuilder<'a>>;
 
 // `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<'_, ()>, 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)]
 pub enum SuggestionStyle {

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,

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);

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,