From 1e10e503fe7e9d645f1ba3638eb17d8ac306093b Mon Sep 17 00:00:00 2001 From: mu001999 Date: Mon, 14 Oct 2024 16:48:27 +0800 Subject: [PATCH 01/11] Remove allowing static_mut_refs lint --- .../rustc_driver_impl/src/signal_handler.rs | 20 ++++++++++--------- library/panic_unwind/src/seh.rs | 2 -- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_driver_impl/src/signal_handler.rs b/compiler/rustc_driver_impl/src/signal_handler.rs index d4f8199390cc..08b7d937661f 100644 --- a/compiler/rustc_driver_impl/src/signal_handler.rs +++ b/compiler/rustc_driver_impl/src/signal_handler.rs @@ -2,7 +2,7 @@ //! Primarily used to extract a backtrace from stack overflow use std::alloc::{Layout, alloc}; -use std::{fmt, mem, ptr}; +use std::{fmt, mem, ptr, slice}; use rustc_interface::util::{DEFAULT_STACK_SIZE, STACK_SIZE}; @@ -35,20 +35,22 @@ macro raw_errln($tokens:tt) { } /// Signal handler installed for SIGSEGV -// FIXME(static_mut_refs): Do not allow `static_mut_refs` lint -#[allow(static_mut_refs)] -extern "C" fn print_stack_trace(_: libc::c_int) { +/// +/// # Safety +/// +/// Caller must ensure that this function is not re-entered. +unsafe extern "C" fn print_stack_trace(_: libc::c_int) { const MAX_FRAMES: usize = 256; - // Reserve data segment so we don't have to malloc in a signal handler, which might fail - // in incredibly undesirable and unexpected ways due to e.g. the allocator deadlocking - static mut STACK_TRACE: [*mut libc::c_void; MAX_FRAMES] = [ptr::null_mut(); MAX_FRAMES]; let stack = unsafe { + // Reserve data segment so we don't have to malloc in a signal handler, which might fail + // in incredibly undesirable and unexpected ways due to e.g. the allocator deadlocking + static mut STACK_TRACE: [*mut libc::c_void; MAX_FRAMES] = [ptr::null_mut(); MAX_FRAMES]; // Collect return addresses - let depth = libc::backtrace(STACK_TRACE.as_mut_ptr(), MAX_FRAMES as i32); + let depth = libc::backtrace(&raw mut STACK_TRACE as _, MAX_FRAMES as i32); if depth == 0 { return; } - &STACK_TRACE.as_slice()[0..(depth as _)] + slice::from_raw_parts(&raw const STACK_TRACE as _, depth as _) }; // Just a stack trace is cryptic. Explain what we're doing. diff --git a/library/panic_unwind/src/seh.rs b/library/panic_unwind/src/seh.rs index 565a2b8c573b..5afa0a197561 100644 --- a/library/panic_unwind/src/seh.rs +++ b/library/panic_unwind/src/seh.rs @@ -288,8 +288,6 @@ cfg_if::cfg_if! { } } -// FIXME(static_mut_refs): Do not allow `static_mut_refs` lint -#[allow(static_mut_refs)] pub unsafe fn panic(data: Box) -> u32 { use core::intrinsics::atomic_store_seqcst; From 403c8c2fd6205ca0687abde55fbe5af7b325b7d9 Mon Sep 17 00:00:00 2001 From: dianne Date: Thu, 21 Nov 2024 03:23:56 -0800 Subject: [PATCH 02/11] E0277: suggest dereferencing function arguments in more cases --- .../src/error_reporting/traits/suggestions.rs | 198 ++++++++---------- tests/ui/no_send-rc.stderr | 4 + .../suggestions/issue-84973-blacklist.stderr | 4 + .../suggest-dereferences/deref-argument.fixed | 37 ++++ .../suggest-dereferences/deref-argument.rs | 37 ++++ .../deref-argument.stderr | 39 ++++ 6 files changed, 206 insertions(+), 113 deletions(-) create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.fixed create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.rs create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.stderr diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs index a5e364d49f7c..f9971e5e2d5a 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs @@ -443,9 +443,8 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { } } - /// When after several dereferencing, the reference satisfies the trait - /// bound. This function provides dereference suggestion for this - /// specific situation. + /// Provide a suggestion to dereference arguments to functions and binary operators, if that + /// would satisfy trait bounds. pub(super) fn suggest_dereferences( &self, obligation: &PredicateObligation<'tcx>, @@ -459,127 +458,100 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { && let Some(arg_ty) = typeck_results.expr_ty_adjusted_opt(expr) { // Suggest dereferencing the argument to a function/method call if possible + + // Get the root obligation, since the leaf obligation we have may be unhelpful (#87437) let mut real_trait_pred = trait_pred; while let Some((parent_code, parent_trait_pred)) = code.parent() { code = parent_code; if let Some(parent_trait_pred) = parent_trait_pred { real_trait_pred = parent_trait_pred; } + } - // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle - // `ReBound`, and we don't particularly care about the regions. - let real_ty = - self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty()); + // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle + // `ReBound`, and we don't particularly care about the regions. + let real_ty = self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty()); + if !self.can_eq(obligation.param_env, real_ty, arg_ty) { + return false; + } - if self.can_eq(obligation.param_env, real_ty, arg_ty) - && let ty::Ref(region, base_ty, mutbl) = *real_ty.kind() + // Potentially, we'll want to place our dereferences under a `&`. We don't try this for + // `&mut`, since we can't be sure users will get the side-effects they want from it. + // If this doesn't work, we'll try removing the `&` in `suggest_remove_reference`. + // FIXME(dianne): this misses the case where users need both to deref and remove `&`s. + // This method could be combined with `TypeErrCtxt::suggest_remove_reference` to handle + // that, similar to what `FnCtxt::suggest_deref_or_ref` does. + let (is_under_ref, base_ty, span) = match expr.kind { + hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Not, subexpr) + if let &ty::Ref(region, base_ty, hir::Mutability::Not) = real_ty.kind() => { - let autoderef = (self.autoderef_steps)(base_ty); - if let Some(steps) = - autoderef.into_iter().enumerate().find_map(|(steps, (ty, obligations))| { - // Re-add the `&` - let ty = Ty::new_ref(self.tcx, region, ty, mutbl); - - // Remapping bound vars here - let real_trait_pred_and_ty = real_trait_pred - .map_bound(|inner_trait_pred| (inner_trait_pred, ty)); - let obligation = self.mk_trait_obligation_with_new_self_ty( - obligation.param_env, - real_trait_pred_and_ty, - ); - let may_hold = obligations - .iter() - .chain([&obligation]) - .all(|obligation| self.predicate_may_hold(obligation)) - .then_some(steps); + (Some(region), base_ty, subexpr.span) + } + // Don't suggest `*&mut`, etc. + hir::ExprKind::AddrOf(..) => return false, + _ => (None, real_ty, obligation.cause.span), + }; - may_hold - }) - { - if steps > 0 { - // Don't care about `&mut` because `DerefMut` is used less - // often and user will not expect that an autoderef happens. - if let hir::Node::Expr(hir::Expr { - kind: - hir::ExprKind::AddrOf( - hir::BorrowKind::Ref, - hir::Mutability::Not, - expr, - ), - .. - }) = self.tcx.hir_node(*arg_hir_id) - { - let derefs = "*".repeat(steps); - err.span_suggestion_verbose( - expr.span.shrink_to_lo(), - "consider dereferencing here", - derefs, - Applicability::MachineApplicable, - ); - return true; - } - } - } else if real_trait_pred != trait_pred { - // This branch addresses #87437. - - let span = obligation.cause.span; - // Remapping bound vars here - let real_trait_pred_and_base_ty = real_trait_pred - .map_bound(|inner_trait_pred| (inner_trait_pred, base_ty)); - let obligation = self.mk_trait_obligation_with_new_self_ty( - obligation.param_env, - real_trait_pred_and_base_ty, - ); - let sized_obligation = Obligation::new( - self.tcx, - obligation.cause.clone(), - obligation.param_env, - ty::TraitRef::new( - self.tcx, - self.tcx.require_lang_item( - hir::LangItem::Sized, - Some(obligation.cause.span), - ), - [base_ty], - ), - ); - if self.predicate_may_hold(&obligation) - && self.predicate_must_hold_modulo_regions(&sized_obligation) - // Do not suggest * if it is already a reference, - // will suggest removing the borrow instead in that case. - && !matches!(expr.kind, hir::ExprKind::AddrOf(..)) - { - let call_node = self.tcx.hir_node(*call_hir_id); - let msg = "consider dereferencing here"; - let is_receiver = matches!( - call_node, - Node::Expr(hir::Expr { - kind: hir::ExprKind::MethodCall(_, receiver_expr, ..), - .. - }) - if receiver_expr.hir_id == *arg_hir_id - ); - if is_receiver { - err.multipart_suggestion_verbose( - msg, - vec![ - (span.shrink_to_lo(), "(*".to_string()), - (span.shrink_to_hi(), ")".to_string()), - ], - Applicability::MachineApplicable, - ) - } else { - err.span_suggestion_verbose( - span.shrink_to_lo(), - msg, - '*', - Applicability::MachineApplicable, - ) - }; - return true; - } - } + let autoderef = (self.autoderef_steps)(base_ty); + let mut is_boxed = base_ty.is_box(); + if let Some(steps) = autoderef.into_iter().position(|(mut ty, obligations)| { + // Ensure one of the following for dereferencing to be valid: we're passing by + // reference, `ty` is `Copy`, or we're moving out of a (potentially nested) `Box`. + let can_deref = is_under_ref.is_some() + || self.type_is_copy_modulo_regions(obligation.param_env, ty) + || ty.is_numeric() // for inference vars (presumably but not provably `Copy`) + || is_boxed && self.type_is_sized_modulo_regions(obligation.param_env, ty); + is_boxed &= ty.is_box(); + + // Re-add the `&` if necessary + if let Some(region) = is_under_ref { + ty = Ty::new_ref(self.tcx, region, ty, hir::Mutability::Not); } + + // Remapping bound vars here + let real_trait_pred_and_ty = + real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, ty)); + let obligation = self.mk_trait_obligation_with_new_self_ty( + obligation.param_env, + real_trait_pred_and_ty, + ); + + can_deref + && obligations + .iter() + .chain([&obligation]) + .all(|obligation| self.predicate_may_hold(obligation)) + }) && steps > 0 + { + let derefs = "*".repeat(steps); + let msg = "consider dereferencing here"; + let call_node = self.tcx.hir_node(*call_hir_id); + let is_receiver = matches!( + call_node, + Node::Expr(hir::Expr { + kind: hir::ExprKind::MethodCall(_, receiver_expr, ..), + .. + }) + if receiver_expr.hir_id == *arg_hir_id + ); + if is_receiver { + err.multipart_suggestion_verbose( + msg, + vec![ + (span.shrink_to_lo(), format!("({derefs}")), + (span.shrink_to_hi(), ")".to_string()), + ], + Applicability::MachineApplicable, + ) + } else { + err.span_suggestion_verbose( + span.shrink_to_lo(), + msg, + derefs, + Applicability::MachineApplicable, + ) + }; + return true; } } else if let ( ObligationCauseCode::BinOp { lhs_hir_id, rhs_hir_id: Some(rhs_hir_id), .. }, diff --git a/tests/ui/no_send-rc.stderr b/tests/ui/no_send-rc.stderr index 3534167870ba..1430a7a29ea2 100644 --- a/tests/ui/no_send-rc.stderr +++ b/tests/ui/no_send-rc.stderr @@ -12,6 +12,10 @@ note: required by a bound in `bar` | LL | fn bar(_: T) {} | ^^^^ required by this bound in `bar` +help: consider dereferencing here + | +LL | bar(*x); + | + error: aborting due to 1 previous error diff --git a/tests/ui/suggestions/issue-84973-blacklist.stderr b/tests/ui/suggestions/issue-84973-blacklist.stderr index a6324a824c1c..3db400418c71 100644 --- a/tests/ui/suggestions/issue-84973-blacklist.stderr +++ b/tests/ui/suggestions/issue-84973-blacklist.stderr @@ -86,6 +86,10 @@ note: required by a bound in `f_send` | LL | fn f_send(t: T) {} | ^^^^ required by this bound in `f_send` +help: consider dereferencing here + | +LL | f_send(*rc); + | + error: aborting due to 5 previous errors diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.fixed b/tests/ui/traits/suggest-dereferences/deref-argument.fixed new file mode 100644 index 000000000000..8235ae0b6284 --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.fixed @@ -0,0 +1,37 @@ +//@ run-rustfix +//! diagnostic test for #90997. +//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed. +use std::ops::Deref; + +trait Test { + fn test(self); +} +fn consume_test(x: impl Test) { x.test() } + +impl Test for u32 { + fn test(self) {} +} +struct MyRef(u32); +impl Deref for MyRef { + type Target = u32; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +struct NonCopy; +impl Test for NonCopy { + fn test(self) {} +} + +fn main() { + let my_ref = MyRef(0); + consume_test(*my_ref); + //~^ ERROR the trait bound `MyRef: Test` is not satisfied + //~| SUGGESTION * + + let nested_box = Box::new(Box::new(Box::new(NonCopy))); + consume_test(***nested_box); + //~^ ERROR the trait bound `Box>>: Test` is not satisfied + //~| SUGGESTION *** +} diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.rs b/tests/ui/traits/suggest-dereferences/deref-argument.rs new file mode 100644 index 000000000000..2f96b75c4e47 --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.rs @@ -0,0 +1,37 @@ +//@ run-rustfix +//! diagnostic test for #90997. +//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed. +use std::ops::Deref; + +trait Test { + fn test(self); +} +fn consume_test(x: impl Test) { x.test() } + +impl Test for u32 { + fn test(self) {} +} +struct MyRef(u32); +impl Deref for MyRef { + type Target = u32; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +struct NonCopy; +impl Test for NonCopy { + fn test(self) {} +} + +fn main() { + let my_ref = MyRef(0); + consume_test(my_ref); + //~^ ERROR the trait bound `MyRef: Test` is not satisfied + //~| SUGGESTION * + + let nested_box = Box::new(Box::new(Box::new(NonCopy))); + consume_test(nested_box); + //~^ ERROR the trait bound `Box>>: Test` is not satisfied + //~| SUGGESTION *** +} diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.stderr b/tests/ui/traits/suggest-dereferences/deref-argument.stderr new file mode 100644 index 000000000000..3dc92fd6ab6f --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.stderr @@ -0,0 +1,39 @@ +error[E0277]: the trait bound `MyRef: Test` is not satisfied + --> $DIR/deref-argument.rs:29:18 + | +LL | consume_test(my_ref); + | ------------ ^^^^^^ the trait `Test` is not implemented for `MyRef` + | | + | required by a bound introduced by this call + | +note: required by a bound in `consume_test` + --> $DIR/deref-argument.rs:9:25 + | +LL | fn consume_test(x: impl Test) { x.test() } + | ^^^^ required by this bound in `consume_test` +help: consider dereferencing here + | +LL | consume_test(*my_ref); + | + + +error[E0277]: the trait bound `Box>>: Test` is not satisfied + --> $DIR/deref-argument.rs:34:18 + | +LL | consume_test(nested_box); + | ------------ ^^^^^^^^^^ the trait `Test` is not implemented for `Box>>` + | | + | required by a bound introduced by this call + | +note: required by a bound in `consume_test` + --> $DIR/deref-argument.rs:9:25 + | +LL | fn consume_test(x: impl Test) { x.test() } + | ^^^^ required by this bound in `consume_test` +help: consider dereferencing here + | +LL | consume_test(***nested_box); + | +++ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0277`. From 62c3c9a5ae2072a04ba5f5d84055d38dcd043427 Mon Sep 17 00:00:00 2001 From: Davis Muro Date: Sat, 28 Dec 2024 20:47:07 -0800 Subject: [PATCH 03/11] add suggestion for wrongly ordered format parameters --- compiler/rustc_builtin_macros/messages.ftl | 2 ++ compiler/rustc_builtin_macros/src/errors.rs | 11 ++++++ compiler/rustc_builtin_macros/src/format.rs | 7 ++++ compiler/rustc_parse_format/src/lib.rs | 35 +++++++++++++++++++ ...ggest-wrongly-order-format-parameter.fixed | 25 +++++++++++++ .../suggest-wrongly-order-format-parameter.rs | 25 +++++++++++++ ...gest-wrongly-order-format-parameter.stderr | 35 +++++++++++++++++++ 7 files changed, 140 insertions(+) create mode 100644 tests/ui/fmt/suggest-wrongly-order-format-parameter.fixed create mode 100644 tests/ui/fmt/suggest-wrongly-order-format-parameter.rs create mode 100644 tests/ui/fmt/suggest-wrongly-order-format-parameter.stderr diff --git a/compiler/rustc_builtin_macros/messages.ftl b/compiler/rustc_builtin_macros/messages.ftl index 7a31d2a22391..4cac7cb93f58 100644 --- a/compiler/rustc_builtin_macros/messages.ftl +++ b/compiler/rustc_builtin_macros/messages.ftl @@ -197,6 +197,8 @@ builtin_macros_format_redundant_args = redundant {$n -> builtin_macros_format_remove_raw_ident = remove the `r#` +builtin_macros_format_reorder_format_parameter = did you mean `{$replacement}`? + builtin_macros_format_requires_string = requires at least a format string argument builtin_macros_format_string_invalid = invalid format string: {$desc} diff --git a/compiler/rustc_builtin_macros/src/errors.rs b/compiler/rustc_builtin_macros/src/errors.rs index 1abdfdb9c65c..6213bd802c75 100644 --- a/compiler/rustc_builtin_macros/src/errors.rs +++ b/compiler/rustc_builtin_macros/src/errors.rs @@ -618,6 +618,17 @@ pub(crate) enum InvalidFormatStringSuggestion { #[primary_span] span: Span, }, + #[suggestion( + builtin_macros_format_reorder_format_parameter, + code = "{replacement}", + style = "verbose", + applicability = "machine-applicable" + )] + ReorderFormatParameter { + #[primary_span] + span: Span, + replacement: String, + }, } #[derive(Diagnostic)] diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs index 528eb7725f5c..d07a0c3a51be 100644 --- a/compiler/rustc_builtin_macros/src/format.rs +++ b/compiler/rustc_builtin_macros/src/format.rs @@ -316,6 +316,13 @@ fn make_format_args( e.sugg_ = Some(errors::InvalidFormatStringSuggestion::RemoveRawIdent { span }) } } + parse::Suggestion::ReorderFormatParameter(span, replacement) => { + let span = fmt_span.from_inner(InnerSpan::new(span.start, span.end)); + e.sugg_ = Some(errors::InvalidFormatStringSuggestion::ReorderFormatParameter { + span, + replacement, + }); + } } let guar = ecx.dcx().emit_err(e); return ExpandResult::Ready(Err(guar)); diff --git a/compiler/rustc_parse_format/src/lib.rs b/compiler/rustc_parse_format/src/lib.rs index 1716f417969e..9eb335cb34cc 100644 --- a/compiler/rustc_parse_format/src/lib.rs +++ b/compiler/rustc_parse_format/src/lib.rs @@ -221,6 +221,11 @@ pub enum Suggestion { /// Remove `r#` from identifier: /// `format!("{r#foo}")` -> `format!("{foo}")` RemoveRawIdent(InnerSpan), + /// Reorder format parameter: + /// `format!("{foo:?#}")` -> `format!("{foo:#?}")` + /// `format!("{foo:?x}")` -> `format!("{foo:x?}")` + /// `format!("{foo:?X}")` -> `format!("{foo:X?}")` + ReorderFormatParameter(InnerSpan, string::String), } /// The parser structure for interpreting the input format string. This is @@ -731,6 +736,12 @@ impl<'a> Parser<'a> { } } else if self.consume('?') { spec.ty = "?"; + if let Some(&(_, maybe)) = self.cur.peek() { + match maybe { + '#' | 'x' | 'X' => self.suggest_format_parameter(maybe), + _ => (), + } + } } else { spec.ty = self.word(); if !spec.ty.is_empty() { @@ -932,6 +943,30 @@ impl<'a> Parser<'a> { } } } + + fn suggest_format_parameter(&mut self, c: char) { + let replacement = match c { + '#' => "#?", + 'x' => "x?", + 'X' => "X?", + _ => return, + }; + let Some(pos) = self.consume_pos(c) else { + return; + }; + + let span = self.span(pos - 1, pos + 1); + let pos = self.to_span_index(pos); + + self.errors.insert(0, ParseError { + description: format!("expected `}}`, found `{c}`"), + note: None, + label: "expected `'}'`".into(), + span: pos.to(pos), + secondary_label: None, + suggestion: Suggestion::ReorderFormatParameter(span, format!("{replacement}")), + }) + } } /// Finds the indices of all characters that have been processed and differ between the actual diff --git a/tests/ui/fmt/suggest-wrongly-order-format-parameter.fixed b/tests/ui/fmt/suggest-wrongly-order-format-parameter.fixed new file mode 100644 index 000000000000..a080a65854ab --- /dev/null +++ b/tests/ui/fmt/suggest-wrongly-order-format-parameter.fixed @@ -0,0 +1,25 @@ +//! Regression test for https://github.com/rust-lang/rust/issues/129966 +//! +//! Ensure we provide suggestion for wrongly ordered format parameters. + +//@ run-rustfix +#![allow(dead_code)] + +#[derive(Debug)] +struct Foo(u8, u8); + +fn main() { + let f = Foo(1, 2); + + println!("{f:#?}"); + //~^ ERROR invalid format string: expected `}`, found `#` + //~| HELP did you mean `#?`? + + println!("{f:x?}"); + //~^ ERROR invalid format string: expected `}`, found `x` + //~| HELP did you mean `x?`? + + println!("{f:X?}"); + //~^ ERROR invalid format string: expected `}`, found `X` + //~| HELP did you mean `X?`? +} diff --git a/tests/ui/fmt/suggest-wrongly-order-format-parameter.rs b/tests/ui/fmt/suggest-wrongly-order-format-parameter.rs new file mode 100644 index 000000000000..830dafd44790 --- /dev/null +++ b/tests/ui/fmt/suggest-wrongly-order-format-parameter.rs @@ -0,0 +1,25 @@ +//! Regression test for https://github.com/rust-lang/rust/issues/129966 +//! +//! Ensure we provide suggestion for wrongly ordered format parameters. + +//@ run-rustfix +#![allow(dead_code)] + +#[derive(Debug)] +struct Foo(u8, u8); + +fn main() { + let f = Foo(1, 2); + + println!("{f:?#}"); + //~^ ERROR invalid format string: expected `}`, found `#` + //~| HELP did you mean `#?`? + + println!("{f:?x}"); + //~^ ERROR invalid format string: expected `}`, found `x` + //~| HELP did you mean `x?`? + + println!("{f:?X}"); + //~^ ERROR invalid format string: expected `}`, found `X` + //~| HELP did you mean `X?`? +} diff --git a/tests/ui/fmt/suggest-wrongly-order-format-parameter.stderr b/tests/ui/fmt/suggest-wrongly-order-format-parameter.stderr new file mode 100644 index 000000000000..fe693d2e9041 --- /dev/null +++ b/tests/ui/fmt/suggest-wrongly-order-format-parameter.stderr @@ -0,0 +1,35 @@ +error: invalid format string: expected `}`, found `#` + --> $DIR/suggest-wrongly-order-format-parameter.rs:14:19 + | +LL | println!("{f:?#}"); + | ^ expected `'}'` in format string + | +help: did you mean `#?`? + | +LL | println!("{f:#?}"); + | ~~ + +error: invalid format string: expected `}`, found `x` + --> $DIR/suggest-wrongly-order-format-parameter.rs:18:19 + | +LL | println!("{f:?x}"); + | ^ expected `'}'` in format string + | +help: did you mean `x?`? + | +LL | println!("{f:x?}"); + | ~~ + +error: invalid format string: expected `}`, found `X` + --> $DIR/suggest-wrongly-order-format-parameter.rs:22:19 + | +LL | println!("{f:?X}"); + | ^ expected `'}'` in format string + | +help: did you mean `X?`? + | +LL | println!("{f:X?}"); + | ~~ + +error: aborting due to 3 previous errors + From b994124778907b5732f5453484b8b7a6812cf0f7 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 31 Dec 2024 01:03:18 +0000 Subject: [PATCH 04/11] Explain how to mutate a HashMap/BTreeMap with more nuance --- compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs | 2 +- tests/ui/borrowck/index-mut-help.stderr | 2 +- tests/ui/btreemap/btreemap-index-mut-2.stderr | 2 +- tests/ui/btreemap/btreemap-index-mut.stderr | 2 +- tests/ui/hashmap/hashmap-index-mut.stderr | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 2484f817a06f..4f82dfeb18bc 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -575,7 +575,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { // ---------- place self.err.multipart_suggestions( format!( - "to modify a `{}`, use `.get_mut()`, `.insert()` or the entry API", + "use `.insert()` to insert a value into a `{}`, `.get_mut()` to modify it, or the entry API for more flexibility", self.ty, ), vec![ diff --git a/tests/ui/borrowck/index-mut-help.stderr b/tests/ui/borrowck/index-mut-help.stderr index fde2b5dc0762..449fe42353a6 100644 --- a/tests/ui/borrowck/index-mut-help.stderr +++ b/tests/ui/borrowck/index-mut-help.stderr @@ -14,7 +14,7 @@ LL | map["peter"] = "0".to_string(); | ^^^^^^^^^^^^ cannot assign | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `HashMap<&str, String>` -help: to modify a `HashMap<&str, String>`, use `.get_mut()`, `.insert()` or the entry API +help: use `.insert()` to insert a value into a `HashMap<&str, String>`, `.get_mut()` to modify it, or the entry API for more flexibility | LL | map.insert("peter", "0".to_string()); | ~~~~~~~~ ~ + diff --git a/tests/ui/btreemap/btreemap-index-mut-2.stderr b/tests/ui/btreemap/btreemap-index-mut-2.stderr index 0b8c77cb9e10..4589416b3196 100644 --- a/tests/ui/btreemap/btreemap-index-mut-2.stderr +++ b/tests/ui/btreemap/btreemap-index-mut-2.stderr @@ -5,7 +5,7 @@ LL | map[&0] = 1; | ^^^^^^^^^^^ cannot assign | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `BTreeMap` -help: to modify a `BTreeMap`, use `.get_mut()`, `.insert()` or the entry API +help: use `.insert()` to insert a value into a `BTreeMap`, `.get_mut()` to modify it, or the entry API for more flexibility | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + diff --git a/tests/ui/btreemap/btreemap-index-mut.stderr b/tests/ui/btreemap/btreemap-index-mut.stderr index cc465fbf3def..2f53296d6302 100644 --- a/tests/ui/btreemap/btreemap-index-mut.stderr +++ b/tests/ui/btreemap/btreemap-index-mut.stderr @@ -5,7 +5,7 @@ LL | map[&0] = 1; | ^^^^^^^^^^^ cannot assign | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `BTreeMap` -help: to modify a `BTreeMap`, use `.get_mut()`, `.insert()` or the entry API +help: use `.insert()` to insert a value into a `BTreeMap`, `.get_mut()` to modify it, or the entry API for more flexibility | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + diff --git a/tests/ui/hashmap/hashmap-index-mut.stderr b/tests/ui/hashmap/hashmap-index-mut.stderr index 2381b8ecb969..6b294823f6f9 100644 --- a/tests/ui/hashmap/hashmap-index-mut.stderr +++ b/tests/ui/hashmap/hashmap-index-mut.stderr @@ -5,7 +5,7 @@ LL | map[&0] = 1; | ^^^^^^^^^^^ cannot assign | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `HashMap` -help: to modify a `HashMap`, use `.get_mut()`, `.insert()` or the entry API +help: use `.insert()` to insert a value into a `HashMap`, `.get_mut()` to modify it, or the entry API for more flexibility | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + From 6a3474e653cbd77941f540852027f136c1d741c4 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 31 Dec 2024 01:11:38 +0000 Subject: [PATCH 05/11] Use if-let in structured suggestion instead of Option::map --- .../src/diagnostics/mutability_errors.rs | 15 +++++++++------ tests/ui/borrowck/index-mut-help.stderr | 4 ++-- tests/ui/btreemap/btreemap-index-mut-2.stderr | 4 ++-- tests/ui/btreemap/btreemap-index-mut.stderr | 4 ++-- tests/ui/hashmap/hashmap-index-mut.stderr | 4 ++-- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 4f82dfeb18bc..109ec096417f 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -575,7 +575,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { // ---------- place self.err.multipart_suggestions( format!( - "use `.insert()` to insert a value into a `{}`, `.get_mut()` to modify it, or the entry API for more flexibility", + "use `.insert()` to insert a value into a `{}`, `.get_mut()` \ + to modify it, or the entry API for more flexibility", self.ty, ), vec![ @@ -592,16 +593,17 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { (rv.span.shrink_to_hi(), ")".to_string()), ], vec![ - // val.get_mut(index).map(|v| { *v = rv; }); + // if let Some(v) = val.get_mut(index) { *v = rv; } + (val.span.shrink_to_lo(), "if let Some(val) = ".to_string()), ( val.span.shrink_to_hi().with_hi(index.span.lo()), ".get_mut(".to_string(), ), ( index.span.shrink_to_hi().with_hi(place.span.hi()), - ").map(|val| { *val".to_string(), + ") { *val".to_string(), ), - (rv.span.shrink_to_hi(), "; })".to_string()), + (rv.span.shrink_to_hi(), "; }".to_string()), ], vec![ // let x = val.entry(index).or_insert(rv); @@ -628,15 +630,16 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { self.err.multipart_suggestion( format!("to modify a `{}` use `.get_mut()`", self.ty), vec![ + (val.span.shrink_to_lo(), "if let Some(val) = ".to_string()), ( val.span.shrink_to_hi().with_hi(index.span.lo()), ".get_mut(".to_string(), ), ( index.span.shrink_to_hi().with_hi(receiver.span.hi()), - ").map(|val| val".to_string(), + ") { val".to_string(), ), - (sp.shrink_to_hi(), ")".to_string()), + (sp.shrink_to_hi(), "); }".to_string()), ], Applicability::MachineApplicable, ); diff --git a/tests/ui/borrowck/index-mut-help.stderr b/tests/ui/borrowck/index-mut-help.stderr index 449fe42353a6..8766ff4a6c41 100644 --- a/tests/ui/borrowck/index-mut-help.stderr +++ b/tests/ui/borrowck/index-mut-help.stderr @@ -18,8 +18,8 @@ help: use `.insert()` to insert a value into a `HashMap<&str, String>`, `.get_mu | LL | map.insert("peter", "0".to_string()); | ~~~~~~~~ ~ + -LL | map.get_mut("peter").map(|val| { *val = "0".to_string(); }); - | ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~ ++++ +LL | if let Some(val) = map.get_mut("peter") { *val = "0".to_string(); }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~~ +++ LL | let val = map.entry("peter").or_insert("0".to_string()); | +++++++++ ~~~~~~~ ~~~~~~~~~~~~ + diff --git a/tests/ui/btreemap/btreemap-index-mut-2.stderr b/tests/ui/btreemap/btreemap-index-mut-2.stderr index 4589416b3196..c42462ee1eb5 100644 --- a/tests/ui/btreemap/btreemap-index-mut-2.stderr +++ b/tests/ui/btreemap/btreemap-index-mut-2.stderr @@ -9,8 +9,8 @@ help: use `.insert()` to insert a value into a `BTreeMap`, `.get_mut() | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + -LL | map.get_mut(&0).map(|val| { *val = 1; }); - | ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~ ++++ +LL | if let Some(val) = map.get_mut(&0) { *val = 1; }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~~ +++ LL | let val = map.entry(&0).or_insert(1); | +++++++++ ~~~~~~~ ~~~~~~~~~~~~ + diff --git a/tests/ui/btreemap/btreemap-index-mut.stderr b/tests/ui/btreemap/btreemap-index-mut.stderr index 2f53296d6302..f402f503c157 100644 --- a/tests/ui/btreemap/btreemap-index-mut.stderr +++ b/tests/ui/btreemap/btreemap-index-mut.stderr @@ -9,8 +9,8 @@ help: use `.insert()` to insert a value into a `BTreeMap`, `.get_mut() | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + -LL | map.get_mut(&0).map(|val| { *val = 1; }); - | ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~ ++++ +LL | if let Some(val) = map.get_mut(&0) { *val = 1; }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~~ +++ LL | let val = map.entry(&0).or_insert(1); | +++++++++ ~~~~~~~ ~~~~~~~~~~~~ + diff --git a/tests/ui/hashmap/hashmap-index-mut.stderr b/tests/ui/hashmap/hashmap-index-mut.stderr index 6b294823f6f9..ad33c6f9b154 100644 --- a/tests/ui/hashmap/hashmap-index-mut.stderr +++ b/tests/ui/hashmap/hashmap-index-mut.stderr @@ -9,8 +9,8 @@ help: use `.insert()` to insert a value into a `HashMap`, `.get_mut()` | LL | map.insert(&0, 1); | ~~~~~~~~ ~ + -LL | map.get_mut(&0).map(|val| { *val = 1; }); - | ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~ ++++ +LL | if let Some(val) = map.get_mut(&0) { *val = 1; }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~~ +++ LL | let val = map.entry(&0).or_insert(1); | +++++++++ ~~~~~~~ ~~~~~~~~~~~~ + From f28e13b0552ab7b95e0cc764c5285ebac23fcaa2 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 31 Dec 2024 01:20:45 +0000 Subject: [PATCH 06/11] Fix span for IndexMut method call on HashMap/BTreeMap --- compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs | 4 ++-- tests/ui/borrowck/index-mut-help.stderr | 5 ++++- tests/ui/issues/issue-41726.stderr | 5 ++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 109ec096417f..c690789b587f 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -624,7 +624,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { self.suggested = true; } else if let hir::ExprKind::MethodCall(_path, receiver, _, sp) = expr.kind && let hir::ExprKind::Index(val, index, _) = receiver.kind - && expr.span == self.assign_span + && receiver.span == self.assign_span { // val[index].path(args..); self.err.multipart_suggestion( @@ -639,7 +639,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> { index.span.shrink_to_hi().with_hi(receiver.span.hi()), ") { val".to_string(), ), - (sp.shrink_to_hi(), "); }".to_string()), + (sp.shrink_to_hi(), "; }".to_string()), ], Applicability::MachineApplicable, ); diff --git a/tests/ui/borrowck/index-mut-help.stderr b/tests/ui/borrowck/index-mut-help.stderr index 8766ff4a6c41..c4c9c1c53139 100644 --- a/tests/ui/borrowck/index-mut-help.stderr +++ b/tests/ui/borrowck/index-mut-help.stderr @@ -5,7 +5,10 @@ LL | map["peter"].clear(); | ^^^^^^^^^^^^ cannot borrow as mutable | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `HashMap<&str, String>` - = help: to modify a `HashMap<&str, String>`, use `.get_mut()`, `.insert()` or the entry API +help: to modify a `HashMap<&str, String>` use `.get_mut()` + | +LL | if let Some(val) = map.get_mut("peter") { val.clear(); }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~ +++ error[E0594]: cannot assign to data in an index of `HashMap<&str, String>` --> $DIR/index-mut-help.rs:11:5 diff --git a/tests/ui/issues/issue-41726.stderr b/tests/ui/issues/issue-41726.stderr index fe7d4df70676..250bba222bfd 100644 --- a/tests/ui/issues/issue-41726.stderr +++ b/tests/ui/issues/issue-41726.stderr @@ -5,7 +5,10 @@ LL | things[src.as_str()].sort(); | ^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable | = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `HashMap>` - = help: to modify a `HashMap>`, use `.get_mut()`, `.insert()` or the entry API +help: to modify a `HashMap>` use `.get_mut()` + | +LL | if let Some(val) = things.get_mut(src.as_str()) { val.sort(); }; + | ++++++++++++++++++ ~~~~~~~~~ ~~~~~~~ +++ error: aborting due to 1 previous error From 1afeeef77d7cc55d37bb00642716b78453d2d476 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 12:14:31 +1100 Subject: [PATCH 07/11] Move most of the `Step::run` impl out of `tool_check_step!` Ordinary code is much easier to work with than macro-generated code. --- src/bootstrap/src/core/build_steps/check.rs | 86 ++++++++++----------- 1 file changed, 41 insertions(+), 45 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index b4d37b25a6c7..81c69cbd5dbd 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -428,53 +428,49 @@ macro_rules! tool_check_step { } fn run(self, builder: &Builder<'_>) { - let compiler = builder.compiler(builder.top_stage, builder.config.build); - let target = self.target; - - builder.ensure(Rustc::new(target, builder)); - - let mut cargo = prepare_tool_cargo( - builder, - compiler, - Mode::ToolRustc, - target, - builder.kind, - $path, - $source_type, - &[], - ); - - // For ./x.py clippy, don't run with --all-targets because - // linting tests and benchmarks can produce very noisy results - if builder.kind != Kind::Clippy { - cargo.arg("--all-targets"); - } - - let _guard = builder.msg_check(&format!("{} artifacts", $display_name), target); - run_cargo( - builder, - cargo, - builder.config.free_args.clone(), - &stamp(builder, compiler, target), - vec![], - true, - false, - ); - - /// Cargo's output path in a given stage, compiled by a particular - /// compiler for the specified target. - fn stamp( - builder: &Builder<'_>, - compiler: Compiler, - target: TargetSelection, - ) -> PathBuf { - builder - .cargo_out(compiler, Mode::ToolRustc, target) - .join(format!(".{}-check.stamp", stringify!($name).to_lowercase())) - } + let Self { target } = self; + run_tool_check_step(builder, target, stringify!($name), $display_name, $path, $source_type); } } - }; + } +} + +/// Used by the implementation of `Step::run` in `tool_check_step!`. +fn run_tool_check_step( + builder: &Builder<'_>, + target: TargetSelection, + step_type_name: &str, + display_name: &str, + path: &str, + source_type: SourceType, +) { + let compiler = builder.compiler(builder.top_stage, builder.config.build); + + builder.ensure(Rustc::new(target, builder)); + + let mut cargo = prepare_tool_cargo( + builder, + compiler, + Mode::ToolRustc, + target, + builder.kind, + path, + source_type, + &[], + ); + + // For ./x.py clippy, don't run with --all-targets because + // linting tests and benchmarks can produce very noisy results + if builder.kind != Kind::Clippy { + cargo.arg("--all-targets"); + } + + let stamp = builder + .cargo_out(compiler, Mode::ToolRustc, target) + .join(format!(".{}-check.stamp", step_type_name.to_lowercase())); + + let _guard = builder.msg_check(format!("{display_name} artifacts"), target); + run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } tool_check_step!(Rustdoc, "rustdoc", "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); From c59ccae7393e62735d529c1d7b7e3d6ff90e1049 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 12:33:24 +1100 Subject: [PATCH 08/11] Infer tool-check-step display name from the last path component --- src/bootstrap/src/core/build_steps/check.rs | 37 ++++++++------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 81c69cbd5dbd..15d7aea52a64 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -402,7 +402,6 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( $name:ident, - $display_name:literal, $path:literal, $($alias:literal, )* $source_type:path @@ -429,7 +428,7 @@ macro_rules! tool_check_step { fn run(self, builder: &Builder<'_>) { let Self { target } = self; - run_tool_check_step(builder, target, stringify!($name), $display_name, $path, $source_type); + run_tool_check_step(builder, target, stringify!($name), $path, $source_type); } } } @@ -440,10 +439,10 @@ fn run_tool_check_step( builder: &Builder<'_>, target: TargetSelection, step_type_name: &str, - display_name: &str, path: &str, source_type: SourceType, ) { + let display_name = path.rsplit('/').next().unwrap(); let compiler = builder.compiler(builder.top_stage, builder.config.build); builder.ensure(Rustc::new(target, builder)); @@ -473,33 +472,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "rustdoc", "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); +tool_check_step!(Rustdoc, "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "clippy", "src/tools/clippy", SourceType::InTree); -tool_check_step!(Miri, "miri", "src/tools/miri", SourceType::InTree); -tool_check_step!(CargoMiri, "cargo-miri", "src/tools/miri/cargo-miri", SourceType::InTree); -tool_check_step!(Rls, "rls", "src/tools/rls", SourceType::InTree); -tool_check_step!(Rustfmt, "rustfmt", "src/tools/rustfmt", SourceType::InTree); -tool_check_step!( - MiroptTestTools, - "miropt-test-tools", - "src/tools/miropt-test-tools", - SourceType::InTree -); -tool_check_step!( - TestFloatParse, - "test-float-parse", - "src/etc/test-float-parse", - SourceType::InTree -); - -tool_check_step!(Bootstrap, "bootstrap", "src/bootstrap", SourceType::InTree, false); +tool_check_step!(Clippy, "src/tools/clippy", SourceType::InTree); +tool_check_step!(Miri, "src/tools/miri", SourceType::InTree); +tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri", SourceType::InTree); +tool_check_step!(Rls, "src/tools/rls", SourceType::InTree); +tool_check_step!(Rustfmt, "src/tools/rustfmt", SourceType::InTree); +tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools", SourceType::InTree); +tool_check_step!(TestFloatParse, "src/etc/test-float-parse", SourceType::InTree); + +tool_check_step!(Bootstrap, "src/bootstrap", SourceType::InTree, false); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "compiletest", "src/tools/compiletest", SourceType::InTree, false); +tool_check_step!(Compiletest, "src/tools/compiletest", SourceType::InTree, false); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target. From 774e83cea1fb3d6ddf15f50405206ae7c9cb0761 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 13:01:58 +1100 Subject: [PATCH 09/11] Make `tool_check_step!` always assume `SourceType::InTree` All of the tools that use this macro are currently in-tree, so support for specifying a `SourceType` was not meaningfully used. It can potentially be re-added in the future if needed. --- src/bootstrap/src/core/build_steps/check.rs | 44 +++++++++++---------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 15d7aea52a64..1ea28901363e 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -402,10 +402,9 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( $name:ident, - $path:literal, - $($alias:literal, )* - $source_type:path - $(, $default:literal )? + $path:literal + $(, alt_path: $alt_path:literal )* + $(, default: $default:literal )? ) => { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct $name { @@ -415,11 +414,11 @@ macro_rules! tool_check_step { impl Step for $name { type Output = (); const ONLY_HOSTS: bool = true; - /// don't ever check out-of-tree tools by default, they'll fail when toolstate is broken - const DEFAULT: bool = matches!($source_type, SourceType::InTree) $( && $default )?; + /// Most of the tool-checks using this macro are run by default. + const DEFAULT: bool = true $( && $default )?; fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { - run.paths(&[ $path, $($alias),* ]) + run.paths(&[ $path, $( $alt_path ),* ]) } fn make_run(run: RunConfig<'_>) { @@ -428,7 +427,7 @@ macro_rules! tool_check_step { fn run(self, builder: &Builder<'_>) { let Self { target } = self; - run_tool_check_step(builder, target, stringify!($name), $path, $source_type); + run_tool_check_step(builder, target, stringify!($name), $path); } } } @@ -440,7 +439,6 @@ fn run_tool_check_step( target: TargetSelection, step_type_name: &str, path: &str, - source_type: SourceType, ) { let display_name = path.rsplit('/').next().unwrap(); let compiler = builder.compiler(builder.top_stage, builder.config.build); @@ -454,7 +452,11 @@ fn run_tool_check_step( target, builder.kind, path, - source_type, + // Currently, all of the tools that use this macro/function are in-tree. + // If support for out-of-tree tools is re-added in the future, those + // steps should probably be marked non-default so that the default + // checks aren't affected by toolstate being broken. + SourceType::InTree, &[], ); @@ -472,23 +474,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "src/tools/rustdoc", "src/librustdoc", SourceType::InTree); +tool_check_step!(Rustdoc, "src/tools/rustdoc", alt_path: "src/librustdoc"); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "src/tools/clippy", SourceType::InTree); -tool_check_step!(Miri, "src/tools/miri", SourceType::InTree); -tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri", SourceType::InTree); -tool_check_step!(Rls, "src/tools/rls", SourceType::InTree); -tool_check_step!(Rustfmt, "src/tools/rustfmt", SourceType::InTree); -tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools", SourceType::InTree); -tool_check_step!(TestFloatParse, "src/etc/test-float-parse", SourceType::InTree); - -tool_check_step!(Bootstrap, "src/bootstrap", SourceType::InTree, false); +tool_check_step!(Clippy, "src/tools/clippy"); +tool_check_step!(Miri, "src/tools/miri"); +tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri"); +tool_check_step!(Rls, "src/tools/rls"); +tool_check_step!(Rustfmt, "src/tools/rustfmt"); +tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools"); +tool_check_step!(TestFloatParse, "src/etc/test-float-parse"); + +tool_check_step!(Bootstrap, "src/bootstrap", default: false); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "src/tools/compiletest", SourceType::InTree, false); +tool_check_step!(Compiletest, "src/tools/compiletest", default: false); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target. From 66fd5340eaf1792f845e6dbfda7063c9b8d856c4 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 31 Dec 2024 13:04:42 +1100 Subject: [PATCH 10/11] Use struct-like syntax in `tool_check_step!` This tricks rustfmt into formatting the macro arguments as expressions, instead of giving up and ignoring them. --- src/bootstrap/src/core/build_steps/check.rs | 33 +++++++++++---------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/check.rs b/src/bootstrap/src/core/build_steps/check.rs index 1ea28901363e..9434d876df8e 100644 --- a/src/bootstrap/src/core/build_steps/check.rs +++ b/src/bootstrap/src/core/build_steps/check.rs @@ -401,10 +401,13 @@ impl Step for RustAnalyzer { macro_rules! tool_check_step { ( - $name:ident, - $path:literal - $(, alt_path: $alt_path:literal )* - $(, default: $default:literal )? + $name:ident { + // The part of this path after the final '/' is also used as a display name. + path: $path:literal + $(, alt_path: $alt_path:literal )* + $(, default: $default:literal )? + $( , )? + } ) => { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct $name { @@ -474,23 +477,23 @@ fn run_tool_check_step( run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false); } -tool_check_step!(Rustdoc, "src/tools/rustdoc", alt_path: "src/librustdoc"); +tool_check_step!(Rustdoc { path: "src/tools/rustdoc", alt_path: "src/librustdoc" }); // Clippy, miri and Rustfmt are hybrids. They are external tools, but use a git subtree instead // of a submodule. Since the SourceType only drives the deny-warnings // behavior, treat it as in-tree so that any new warnings in clippy will be // rejected. -tool_check_step!(Clippy, "src/tools/clippy"); -tool_check_step!(Miri, "src/tools/miri"); -tool_check_step!(CargoMiri, "src/tools/miri/cargo-miri"); -tool_check_step!(Rls, "src/tools/rls"); -tool_check_step!(Rustfmt, "src/tools/rustfmt"); -tool_check_step!(MiroptTestTools, "src/tools/miropt-test-tools"); -tool_check_step!(TestFloatParse, "src/etc/test-float-parse"); - -tool_check_step!(Bootstrap, "src/bootstrap", default: false); +tool_check_step!(Clippy { path: "src/tools/clippy" }); +tool_check_step!(Miri { path: "src/tools/miri" }); +tool_check_step!(CargoMiri { path: "src/tools/miri/cargo-miri" }); +tool_check_step!(Rls { path: "src/tools/rls" }); +tool_check_step!(Rustfmt { path: "src/tools/rustfmt" }); +tool_check_step!(MiroptTestTools { path: "src/tools/miropt-test-tools" }); +tool_check_step!(TestFloatParse { path: "src/etc/test-float-parse" }); + +tool_check_step!(Bootstrap { path: "src/bootstrap", default: false }); // Compiletest is implicitly "checked" when it gets built in order to run tests, // so this is mainly for people working on compiletest to run locally. -tool_check_step!(Compiletest, "src/tools/compiletest", default: false); +tool_check_step!(Compiletest { path: "src/tools/compiletest", default: false }); /// Cargo's output path for the standard library in a given stage, compiled /// by a particular compiler for the specified target. From 2b2ea9e875dc9ae6c2d351078f1f43e533c9d780 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 31 Dec 2024 18:06:01 +0000 Subject: [PATCH 11/11] Provide structured suggestion for `impl Default` of type where all fields have defaults ``` error: `Default` impl doesn't use the declared default field values --> $DIR/manual-default-impl-could-be-derived.rs:28:1 | LL | / impl Default for B { LL | | fn default() -> Self { LL | | B { LL | | x: s(), | | --- this field has a default value LL | | y: 0, | | - this field has a default value ... | LL | | } | |_^ | help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` | LL ~ #[derive(Default)] struct B { | ``` Note that above the structured suggestion also includes completely removing the manual `impl`, but the rendering doesn't. --- .../src/default_could_be_derived.rs | 34 ++++++++++++++----- ...anual-default-impl-could-be-derived.stderr | 5 ++- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs index d95cbb051580..bae9defa6870 100644 --- a/compiler/rustc_lint/src/default_could_be_derived.rs +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -1,9 +1,11 @@ use rustc_data_structures::fx::FxHashMap; -use rustc_errors::Diag; +use rustc_errors::{Applicability, Diag}; use rustc_hir as hir; use rustc_middle::ty; +use rustc_middle::ty::TyCtxt; use rustc_session::{declare_lint, impl_lint_pass}; use rustc_span::Symbol; +use rustc_span::def_id::DefId; use rustc_span::symbol::sym; use crate::{LateContext, LateLintPass}; @@ -149,13 +151,16 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { let hir_id = cx.tcx.local_def_id_to_hir_id(local); let hir::Node::Item(item) = cx.tcx.hir_node(hir_id) else { return }; cx.tcx.node_span_lint(DEFAULT_OVERRIDES_DEFAULT_FIELDS, hir_id, item.span, |diag| { - mk_lint(diag, orig_fields, fields); + mk_lint(cx.tcx, diag, type_def_id, parent, orig_fields, fields); }); } } fn mk_lint( + tcx: TyCtxt<'_>, diag: &mut Diag<'_, ()>, + type_def_id: DefId, + impl_def_id: DefId, orig_fields: FxHashMap>, fields: &[hir::ExprField<'_>], ) { @@ -175,11 +180,24 @@ fn mk_lint( } } - diag.help(if removed_all_fields { - "to avoid divergence in behavior between `Struct { .. }` and \ - `::default()`, derive the `Default`" + if removed_all_fields { + let msg = "to avoid divergence in behavior between `Struct { .. }` and \ + `::default()`, derive the `Default`"; + if let Some(hir::Node::Item(impl_)) = tcx.hir().get_if_local(impl_def_id) { + diag.multipart_suggestion_verbose( + msg, + vec![ + (tcx.def_span(type_def_id).shrink_to_lo(), "#[derive(Default)] ".to_string()), + (impl_.span, String::new()), + ], + Applicability::MachineApplicable, + ); + } else { + diag.help(msg); + } } else { - "use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them \ - diverging over time" - }); + let msg = "use the default values in the `impl` with `Struct { mandatory_field, .. }` to \ + avoid them diverging over time"; + diag.help(msg); + } } diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr index e8f607fac7ed..cf06d5418e12 100644 --- a/tests/ui/structs/manual-default-impl-could-be-derived.stderr +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -31,7 +31,10 @@ LL | | y: 0, LL | | } | |_^ | - = help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` +help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + | +LL ~ #[derive(Default)] struct B { + | error: `Default` impl doesn't use the declared default field values --> $DIR/manual-default-impl-could-be-derived.rs:43:1