From a832f5f7bc33002f2b983b0e05bd3cb98f899ed2 Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Mon, 18 Oct 2021 00:41:57 +0100 Subject: [PATCH 1/2] Create `core::fmt::ArgumentV1` with generics instead of fn pointer --- compiler/rustc_builtin_macros/src/format.rs | 18 ++++-- .../src/function_item_references.rs | 62 +++++++------------ library/core/src/fmt/mod.rs | 22 +++++++ .../expected_show_coverage.closure.txt | 8 +-- .../expected_show_coverage.uses_crate.txt | 8 +-- ...pected_show_coverage.uses_inline_crate.txt | 8 +-- .../ui/attributes/key-value-expansion.stderr | 3 +- ...re-print-generic-trim-off-verbose-2.stderr | 2 +- .../closure-print-generic-verbose-2.stderr | 2 +- src/test/ui/fmt/ifmt-unimpl.stderr | 5 ++ src/test/ui/issues/issue-69455.stderr | 10 +-- src/tools/clippy/clippy_utils/src/macros.rs | 22 +++++-- 12 files changed, 98 insertions(+), 72 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs index d1393528d1ce8..453f2163da1bd 100644 --- a/compiler/rustc_builtin_macros/src/format.rs +++ b/compiler/rustc_builtin_macros/src/format.rs @@ -877,11 +877,21 @@ impl<'a, 'b> Context<'a, 'b> { return ecx.expr_call_global(macsp, path, vec![arg]); } }; + let new_fn_name = match trait_ { + "Display" => "new_display", + "Debug" => "new_debug", + "LowerExp" => "new_lower_exp", + "UpperExp" => "new_upper_exp", + "Octal" => "new_octal", + "Pointer" => "new_pointer", + "Binary" => "new_binary", + "LowerHex" => "new_lower_hex", + "UpperHex" => "new_upper_hex", + _ => unreachable!(), + }; - let path = ecx.std_path(&[sym::fmt, Symbol::intern(trait_), sym::fmt]); - let format_fn = ecx.path_global(sp, path); - let path = ecx.std_path(&[sym::fmt, sym::ArgumentV1, sym::new]); - ecx.expr_call_global(macsp, path, vec![arg, ecx.expr_path(format_fn)]) + let path = ecx.std_path(&[sym::fmt, sym::ArgumentV1, Symbol::intern(new_fn_name)]); + ecx.expr_call_global(sp, path, vec![arg]) } } diff --git a/compiler/rustc_mir_transform/src/function_item_references.rs b/compiler/rustc_mir_transform/src/function_item_references.rs index f364a332a788c..5ea0efde9c0c1 100644 --- a/compiler/rustc_mir_transform/src/function_item_references.rs +++ b/compiler/rustc_mir_transform/src/function_item_references.rs @@ -42,54 +42,28 @@ impl<'tcx> Visitor<'tcx> for FunctionItemRefChecker<'_, 'tcx> { } = &terminator.kind { let source_info = *self.body.source_info(location); - // Only handle function calls outside macros - if !source_info.span.from_expansion() { - let func_ty = func.ty(self.body, self.tcx); - if let ty::FnDef(def_id, substs_ref) = *func_ty.kind() { - // Handle calls to `transmute` - if self.tcx.is_diagnostic_item(sym::transmute, def_id) { - let arg_ty = args[0].ty(self.body, self.tcx); - for generic_inner_ty in arg_ty.walk() { - if let GenericArgKind::Type(inner_ty) = generic_inner_ty.unpack() { - if let Some((fn_id, fn_substs)) = - FunctionItemRefChecker::is_fn_ref(inner_ty) - { - let span = self.nth_arg_span(&args, 0); - self.emit_lint(fn_id, fn_substs, source_info, span); - } + let func_ty = func.ty(self.body, self.tcx); + if let ty::FnDef(def_id, substs_ref) = *func_ty.kind() { + // Handle calls to `transmute` + if self.tcx.is_diagnostic_item(sym::transmute, def_id) { + let arg_ty = args[0].ty(self.body, self.tcx); + for generic_inner_ty in arg_ty.walk() { + if let GenericArgKind::Type(inner_ty) = generic_inner_ty.unpack() { + if let Some((fn_id, fn_substs)) = + FunctionItemRefChecker::is_fn_ref(inner_ty) + { + let span = self.nth_arg_span(&args, 0); + self.emit_lint(fn_id, fn_substs, source_info, span); } } - } else { - self.check_bound_args(def_id, substs_ref, &args, source_info); } + } else { + self.check_bound_args(def_id, substs_ref, &args, source_info); } } } self.super_terminator(terminator, location); } - - /// Emits a lint for function references formatted with `fmt::Pointer::fmt` by macros. These - /// cases are handled as operands instead of call terminators to avoid any dependence on - /// unstable, internal formatting details like whether `fmt` is called directly or not. - fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { - let source_info = *self.body.source_info(location); - if source_info.span.from_expansion() { - let op_ty = operand.ty(self.body, self.tcx); - if let ty::FnDef(def_id, substs_ref) = *op_ty.kind() { - if self.tcx.is_diagnostic_item(sym::pointer_trait_fmt, def_id) { - let param_ty = substs_ref.type_at(0); - if let Some((fn_id, fn_substs)) = FunctionItemRefChecker::is_fn_ref(param_ty) { - // The operand's ctxt wouldn't display the lint since it's inside a macro so - // we have to use the callsite's ctxt. - let callsite_ctxt = source_info.span.source_callsite().ctxt(); - let span = source_info.span.with_ctxt(callsite_ctxt); - self.emit_lint(fn_id, fn_substs, source_info, span); - } - } - } - } - self.super_operand(operand, location); - } } impl<'tcx> FunctionItemRefChecker<'_, 'tcx> { @@ -119,7 +93,13 @@ impl<'tcx> FunctionItemRefChecker<'_, 'tcx> { if let Some((fn_id, fn_substs)) = FunctionItemRefChecker::is_fn_ref(subst_ty) { - let span = self.nth_arg_span(args, arg_num); + let mut span = self.nth_arg_span(args, arg_num); + if span.from_expansion() { + // The operand's ctxt wouldn't display the lint since it's inside a macro so + // we have to use the callsite's ctxt. + let callsite_ctxt = span.source_callsite().ctxt(); + span = span.with_ctxt(callsite_ctxt); + } self.emit_lint(fn_id, fn_substs, source_info, span); } } diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index 8a2a64f8dc97f..e1ea98813031a 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -308,9 +308,21 @@ static USIZE_MARKER: fn(&usize, &mut Formatter<'_>) -> Result = |ptr, _| { loop {} }; +macro_rules! arg_new { + ($f: ident, $t: ident) => { + #[doc(hidden)] + #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] + #[inline] + pub fn $f<'b, T: $t>(x: &'b T) -> ArgumentV1<'_> { + Self::new(x, $t::fmt) + } + }; +} + impl<'a> ArgumentV1<'a> { #[doc(hidden)] #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] + #[inline] pub fn new<'b, T>(x: &'b T, f: fn(&T, &mut Formatter<'_>) -> Result) -> ArgumentV1<'b> { // SAFETY: `mem::transmute(x)` is safe because // 1. `&'b T` keeps the lifetime it originated with `'b` @@ -323,6 +335,16 @@ impl<'a> ArgumentV1<'a> { unsafe { ArgumentV1 { formatter: mem::transmute(f), value: mem::transmute(x) } } } + arg_new!(new_display, Display); + arg_new!(new_debug, Debug); + arg_new!(new_octal, Octal); + arg_new!(new_lower_hex, LowerHex); + arg_new!(new_upper_hex, UpperHex); + arg_new!(new_pointer, Pointer); + arg_new!(new_binary, Binary); + arg_new!(new_lower_exp, LowerExp); + arg_new!(new_upper_exp, UpperExp); + #[doc(hidden)] #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] pub fn from_usize(x: &usize) -> ArgumentV1<'_> { diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt index 7eb393bb4481f..e463099a5ee47 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt @@ -29,8 +29,8 @@ 29| 1| some_string = Some(String::from("the string content")); 30| 1| let 31| 1| a - 32| | = - 33| | || + 32| 1| = + 33| 1| || 34| 0| { 35| 0| let mut countdown = 0; 36| 0| if is_false { @@ -116,8 +116,8 @@ 116| 1| 117| 1| let 118| 1| _unused_closure - 119| 1| = - 120| 1| | + 119| | = + 120| | | 121| | mut countdown 122| | | 123| 0| { diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt index 768dcb2f6084c..c2d5143a61816 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt @@ -19,12 +19,12 @@ 18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg); 19| 2|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: + | used_crate::used_only_from_bin_crate_generic_function::<&str>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&str>: + | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} @@ -36,12 +36,12 @@ 22| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); 23| 2|} ------------------ - | used_crate::used_only_from_this_lib_crate_generic_function::>: + | used_crate::used_only_from_this_lib_crate_generic_function::<&str>: | 21| 1|pub fn used_only_from_this_lib_crate_generic_function(arg: T) { | 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); | 23| 1|} ------------------ - | used_crate::used_only_from_this_lib_crate_generic_function::<&str>: + | used_crate::used_only_from_this_lib_crate_generic_function::>: | 21| 1|pub fn used_only_from_this_lib_crate_generic_function(arg: T) { | 22| 1| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); | 23| 1|} diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt index 89636294035df..dab31cbf4ac9e 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_inline_crate.txt @@ -42,12 +42,12 @@ 40| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg); 41| 2|} ------------------ - | used_inline_crate::used_only_from_bin_crate_generic_function::<&str>: + | used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: | 39| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 41| 1|} ------------------ - | used_inline_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: + | used_inline_crate::used_only_from_bin_crate_generic_function::<&str>: | 39| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 40| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 41| 1|} @@ -61,12 +61,12 @@ 46| 4| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); 47| 4|} ------------------ - | used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>: + | used_inline_crate::used_only_from_this_lib_crate_generic_function::>: | 45| 2|pub fn used_only_from_this_lib_crate_generic_function(arg: T) { | 46| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); | 47| 2|} ------------------ - | used_inline_crate::used_only_from_this_lib_crate_generic_function::>: + | used_inline_crate::used_only_from_this_lib_crate_generic_function::<&str>: | 45| 2|pub fn used_only_from_this_lib_crate_generic_function(arg: T) { | 46| 2| println!("used_only_from_this_lib_crate_generic_function with {:?}", arg); | 47| 2|} diff --git a/src/test/ui/attributes/key-value-expansion.stderr b/src/test/ui/attributes/key-value-expansion.stderr index 268e382327e1d..464e1e1cda779 100644 --- a/src/test/ui/attributes/key-value-expansion.stderr +++ b/src/test/ui/attributes/key-value-expansion.stderr @@ -18,8 +18,7 @@ LL | bug!(); error: unexpected token: `{ let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1(&[""], - &[::core::fmt::ArgumentV1::new(&"u8", - ::core::fmt::Display::fmt)])); + &[::core::fmt::ArgumentV1::new_display(&"u8")])); res }.as_str()` --> $DIR/key-value-expansion.rs:48:23 diff --git a/src/test/ui/closures/print/closure-print-generic-trim-off-verbose-2.stderr b/src/test/ui/closures/print/closure-print-generic-trim-off-verbose-2.stderr index bdc71b8dcaa20..aee782a1c5b85 100644 --- a/src/test/ui/closures/print/closure-print-generic-trim-off-verbose-2.stderr +++ b/src/test/ui/closures/print/closure-print-generic-trim-off-verbose-2.stderr @@ -9,7 +9,7 @@ LL | let c1 : () = c; | expected due to this | = note: expected unit type `()` - found closure `[mod1::f::{closure#0} closure_substs=(unavailable) substs=[T, _#19t, extern "rust-call" fn(()), _#20t]]` + found closure `[mod1::f::{closure#0} closure_substs=(unavailable) substs=[T, _#16t, extern "rust-call" fn(()), _#15t]]` help: use parentheses to call this closure | LL | let c1 : () = c(); diff --git a/src/test/ui/closures/print/closure-print-generic-verbose-2.stderr b/src/test/ui/closures/print/closure-print-generic-verbose-2.stderr index 453f18891d319..6a994ce718eac 100644 --- a/src/test/ui/closures/print/closure-print-generic-verbose-2.stderr +++ b/src/test/ui/closures/print/closure-print-generic-verbose-2.stderr @@ -9,7 +9,7 @@ LL | let c1 : () = c; | expected due to this | = note: expected unit type `()` - found closure `[f::{closure#0} closure_substs=(unavailable) substs=[T, _#19t, extern "rust-call" fn(()), _#20t]]` + found closure `[f::{closure#0} closure_substs=(unavailable) substs=[T, _#16t, extern "rust-call" fn(()), _#15t]]` help: use parentheses to call this closure | LL | let c1 : () = c(); diff --git a/src/test/ui/fmt/ifmt-unimpl.stderr b/src/test/ui/fmt/ifmt-unimpl.stderr index bee165437cb15..6e8dd79219106 100644 --- a/src/test/ui/fmt/ifmt-unimpl.stderr +++ b/src/test/ui/fmt/ifmt-unimpl.stderr @@ -5,6 +5,11 @@ LL | format!("{:X}", "3"); | ^^^ the trait `UpperHex` is not implemented for `str` | = note: required because of the requirements on the impl of `UpperHex` for `&str` +note: required by a bound in `ArgumentV1::<'a>::new_upper_hex` + --> $SRC_DIR/core/src/fmt/mod.rs:LL:COL + | +LL | arg_new!(new_upper_hex, UpperHex); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ArgumentV1::<'a>::new_upper_hex` = note: this error originates in the macro `$crate::__export::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) error: aborting due to previous error diff --git a/src/test/ui/issues/issue-69455.stderr b/src/test/ui/issues/issue-69455.stderr index 95617e4ecc8b8..378fdc97dd315 100644 --- a/src/test/ui/issues/issue-69455.stderr +++ b/src/test/ui/issues/issue-69455.stderr @@ -1,14 +1,14 @@ error[E0282]: type annotations needed - --> $DIR/issue-69455.rs:29:5 + --> $DIR/issue-69455.rs:29:20 | LL | type Output; | ------------ `>::Output` defined here ... LL | println!("{}", 23u64.test(xs.iter().sum())); - | ^^^^^^^^^^^^^^^---------------------------^ - | | | - | | this method call resolves to `>::Output` - | cannot infer type for type parameter `T` declared on the associated function `new` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | this method call resolves to `>::Output` + | cannot infer type for type parameter `T` declared on the associated function `new_display` | = note: this error originates in the macro `$crate::format_args_nl` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/src/tools/clippy/clippy_utils/src/macros.rs b/src/tools/clippy/clippy_utils/src/macros.rs index b7a242cf90a43..a75f6b86a9bae 100644 --- a/src/tools/clippy/clippy_utils/src/macros.rs +++ b/src/tools/clippy/clippy_utils/src/macros.rs @@ -339,15 +339,13 @@ impl<'tcx> FormatArgsExpn<'tcx> { expr_visitor_no_bodies(|e| { // if we're still inside of the macro definition... if e.span.ctxt() == expr.span.ctxt() { - // ArgumnetV1::new(, ::fmt) + // ArgumnetV1::new_() if_chain! { - if let ExprKind::Call(callee, [val, fmt_path]) = e.kind; + if let ExprKind::Call(callee, [val]) = e.kind; if let ExprKind::Path(QPath::TypeRelative(ty, seg)) = callee.kind; - if seg.ident.name == sym::new; if let hir::TyKind::Path(QPath::Resolved(_, path)) = ty.kind; if path.segments.last().unwrap().ident.name == sym::ArgumentV1; - if let ExprKind::Path(QPath::Resolved(_, path)) = fmt_path.kind; - if let [.., fmt_trait, _fmt] = path.segments; + if seg.ident.name.as_str().starts_with("new_"); then { let val_idx = if_chain! { if val.span.ctxt() == expr.span.ctxt(); @@ -361,7 +359,19 @@ impl<'tcx> FormatArgsExpn<'tcx> { formatters.len() } }; - formatters.push((val_idx, fmt_trait.ident.name)); + let fmt_trait = match seg.ident.name.as_str() { + "new_display" => "Display", + "new_debug" => "Debug", + "new_lower_exp" => "LowerExp", + "new_upper_exp" => "UpperExp", + "new_octal" => "Octal", + "new_pointer" => "Pointer", + "new_binary" => "Binary", + "new_lower_hex" => "LowerHex", + "new_upper_hex" => "UpperHex", + _ => unreachable!(), + }; + formatters.push((val_idx, Symbol::intern(fmt_trait))); } } if let ExprKind::Struct(QPath::Resolved(_, path), ..) = e.kind { From 0d4bb0b2952acdfa53844de1e5a515ee62123b9d Mon Sep 17 00:00:00 2001 From: Gary Guo Date: Sat, 8 Jan 2022 01:56:13 +0000 Subject: [PATCH 2/2] Change index_refutable_slice to use FxIndexMap This will prevent unstable order when HirIds are pertubated. --- .../clippy/clippy_lints/src/index_refutable_slice.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/index_refutable_slice.rs b/src/tools/clippy/clippy_lints/src/index_refutable_slice.rs index 4615122bbf9e5..8819fa1bef841 100644 --- a/src/tools/clippy/clippy_lints/src/index_refutable_slice.rs +++ b/src/tools/clippy/clippy_lints/src/index_refutable_slice.rs @@ -4,7 +4,7 @@ use clippy_utils::higher::IfLet; use clippy_utils::ty::is_copy; use clippy_utils::{is_expn_of, is_lint_allowed, meets_msrv, msrvs, path_to_local}; use if_chain::if_chain; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::fx::{FxHashSet, FxIndexMap}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::{self, Visitor}; @@ -92,9 +92,9 @@ impl<'tcx> LateLintPass<'tcx> for IndexRefutableSlice { extract_msrv_attr!(LateContext); } -fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> FxHashMap { +fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> FxIndexMap { let mut removed_pat: FxHashSet = FxHashSet::default(); - let mut slices: FxHashMap = FxHashMap::default(); + let mut slices: FxIndexMap = FxIndexMap::default(); pat.walk_always(|pat| { if let hir::PatKind::Binding(binding, value_hir_id, ident, sub_pat) = pat.kind { // We'll just ignore mut and ref mut for simplicity sake right now @@ -208,10 +208,10 @@ impl SliceLintInformation { fn filter_lintable_slices<'a, 'tcx>( cx: &'a LateContext<'tcx>, - slice_lint_info: FxHashMap, + slice_lint_info: FxIndexMap, max_suggested_slice: u64, scope: &'tcx hir::Expr<'tcx>, -) -> FxHashMap { +) -> FxIndexMap { let mut visitor = SliceIndexLintingVisitor { cx, slice_lint_info, @@ -225,7 +225,7 @@ fn filter_lintable_slices<'a, 'tcx>( struct SliceIndexLintingVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, - slice_lint_info: FxHashMap, + slice_lint_info: FxIndexMap, max_suggested_slice: u64, }