From 3855c5441399adf14d8b8b9e7a87f715a0ae3692 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Fri, 19 Jul 2024 11:24:52 +0000 Subject: [PATCH 01/10] compiletest: cleanup dylib name calculation --- src/tools/compiletest/src/runtest.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 01228869617..de62f4dd96a 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -82,26 +82,22 @@ fn disable_error_reporting R, R>(f: F) -> R { } /// The platform-specific library name -fn get_lib_name(lib: &str, aux_type: AuxType) -> Option { +fn get_lib_name(name: &str, aux_type: AuxType) -> Option { match aux_type { AuxType::Bin => None, // In some cases (e.g. MUSL), we build a static // library, rather than a dynamic library. // In this case, the only path we can pass // with '--extern-meta' is the '.rlib' file - AuxType::Lib => Some(format!("lib{}.rlib", lib)), - AuxType::Dylib => Some(if cfg!(windows) { - format!("{}.dll", lib) - } else if cfg!(target_vendor = "apple") { - format!("lib{}.dylib", lib) - } else if cfg!(target_os = "aix") { - format!("lib{}.a", lib) - } else { - format!("lib{}.so", lib) - }), + AuxType::Lib => Some(format!("lib{name}.rlib")), + AuxType::Dylib => Some(dylib_name(name)), } } +fn dylib_name(name: &str) -> String { + format!("{}{name}.{}", std::env::consts::DLL_PREFIX, std::env::consts::DLL_EXTENSION) +} + pub fn run(config: Arc, testpaths: &TestPaths, revision: Option<&str>) { match &*config.target { "arm-linux-androideabi" From 023b583f6ac49643226fe33b3f9f90cc31dce403 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 21 Jul 2024 21:12:28 -0400 Subject: [PATCH 02/10] Reword E0626 to mention static coroutine --- .../src/error_codes/E0626.md | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_error_codes/src/error_codes/E0626.md b/compiler/rustc_error_codes/src/error_codes/E0626.md index 28d543350ff..71c1f811aa7 100644 --- a/compiler/rustc_error_codes/src/error_codes/E0626.md +++ b/compiler/rustc_error_codes/src/error_codes/E0626.md @@ -1,4 +1,4 @@ -This error occurs because a borrow in a coroutine persists across a +This error occurs because a borrow in a movable coroutine persists across a yield point. Erroneous code example: @@ -15,19 +15,35 @@ let mut b = #[coroutine] || { Pin::new(&mut b).resume(()); ``` -At present, it is not permitted to have a yield that occurs while a -borrow is still in scope. To resolve this error, the borrow must -either be "contained" to a smaller scope that does not overlap the -yield or else eliminated in another way. So, for example, we might -resolve the previous example by removing the borrow and just storing -the integer by value: +Coroutines may be either unmarked, or marked with `static`. If it is unmarked, +then the coroutine is considered "movable". At present, it is not permitted to +have a yield in a movable coroutine that occurs while a borrow is still in +scope. To resolve this error, the coroutine may be marked `static`: + +``` +# #![feature(coroutines, coroutine_trait, stmt_expr_attributes)] +# use std::ops::Coroutine; +# use std::pin::Pin; +let mut b = #[coroutine] static || { // <-- note the static keyword + let a = &String::from("hello, world"); + yield (); + println!("{}", a); +}; +let mut b = std::pin::pin!(b); +b.as_mut().resume(()); +``` + +If the coroutine must remain movable, for example to be used as `Unpin` +without pinning it on the stack or in an allocation, we can alternatively +resolve the previous example by removing the borrow and just storing the +type by value: ``` # #![feature(coroutines, coroutine_trait, stmt_expr_attributes)] # use std::ops::Coroutine; # use std::pin::Pin; let mut b = #[coroutine] || { - let a = 3; + let a = String::from("hello, world"); yield (); println!("{}", a); }; From 6dfc9f8886535725630fdf2f5d70459a21f3ca16 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 21 Jul 2024 21:37:22 -0400 Subject: [PATCH 03/10] Explain that coroutine can be marked static And also point out the def span of the coroutine --- .../rustc_borrowck/src/borrowck_errors.rs | 30 +++++++++++++++++-- tests/ui/coroutine/coroutine-with-nll.stderr | 8 +++++ tests/ui/coroutine/issue-48048.stderr | 8 +++++ tests/ui/coroutine/pattern-borrow.stderr | 7 +++++ .../self_referential_gen_block.stderr | 3 ++ tests/ui/coroutine/yield-in-args.stderr | 8 +++++ .../ui/coroutine/yield-while-iterating.stderr | 7 +++++ .../yield-while-local-borrowed.stderr | 15 ++++++++++ tests/ui/nll/issue-55850.stderr | 8 +++++ 9 files changed, 91 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_borrowck/src/borrowck_errors.rs b/compiler/rustc_borrowck/src/borrowck_errors.rs index 8eb44458137..80deea14685 100644 --- a/compiler/rustc_borrowck/src/borrowck_errors.rs +++ b/compiler/rustc_borrowck/src/borrowck_errors.rs @@ -1,7 +1,9 @@ #![allow(rustc::diagnostic_outside_of_impl)] #![allow(rustc::untranslatable_diagnostic)] +use rustc_errors::Applicability; use rustc_errors::{codes::*, struct_span_code_err, Diag, DiagCtxtHandle}; +use rustc_hir as hir; use rustc_middle::span_bug; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_span::Span; @@ -382,13 +384,35 @@ impl<'infcx, 'tcx> crate::MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> { yield_span: Span, ) -> Diag<'infcx> { let coroutine_kind = self.body.coroutine.as_ref().unwrap().coroutine_kind; - struct_span_code_err!( + let mut diag = struct_span_code_err!( self.dcx(), span, E0626, "borrow may still be in use when {coroutine_kind:#} yields", - ) - .with_span_label(yield_span, "possible yield occurs here") + ); + diag.span_label( + self.infcx.tcx.def_span(self.body.source.def_id()), + format!("within this {coroutine_kind:#}"), + ); + diag.span_label(yield_span, "possible yield occurs here"); + if matches!(coroutine_kind, hir::CoroutineKind::Coroutine(_)) { + let hir::Closure { capture_clause, fn_decl_span, .. } = self + .infcx + .tcx + .hir_node_by_def_id(self.body.source.def_id().expect_local()) + .expect_closure(); + let span = match capture_clause { + rustc_hir::CaptureBy::Value { move_kw } => move_kw.shrink_to_lo(), + rustc_hir::CaptureBy::Ref => fn_decl_span.shrink_to_lo(), + }; + diag.span_suggestion_verbose( + span, + "add `static` to mark this coroutine as unmovable", + "static ", + Applicability::MaybeIncorrect, + ); + } + diag } pub(crate) fn cannot_borrow_across_destructor(&self, borrow_span: Span) -> Diag<'infcx> { diff --git a/tests/ui/coroutine/coroutine-with-nll.stderr b/tests/ui/coroutine/coroutine-with-nll.stderr index 3f3d51da311..ee3a8f45f44 100644 --- a/tests/ui/coroutine/coroutine-with-nll.stderr +++ b/tests/ui/coroutine/coroutine-with-nll.stderr @@ -1,11 +1,19 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/coroutine-with-nll.rs:8:17 | +LL | || { + | -- within this coroutine +... LL | let b = &mut true; | ^^^^^^^^^ LL | LL | yield (); | -------- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | static || { + | ++++++ error: aborting due to 1 previous error diff --git a/tests/ui/coroutine/issue-48048.stderr b/tests/ui/coroutine/issue-48048.stderr index 199ecf4ca6a..8231dbef7f6 100644 --- a/tests/ui/coroutine/issue-48048.stderr +++ b/tests/ui/coroutine/issue-48048.stderr @@ -1,10 +1,18 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/issue-48048.rs:9:9 | +LL | #[coroutine] || { + | -- within this coroutine +... LL | x.0({ | ^^^ LL | yield; | ----- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | #[coroutine] static || { + | ++++++ error: aborting due to 1 previous error diff --git a/tests/ui/coroutine/pattern-borrow.stderr b/tests/ui/coroutine/pattern-borrow.stderr index 9e7b330d79d..a3954b0b8ad 100644 --- a/tests/ui/coroutine/pattern-borrow.stderr +++ b/tests/ui/coroutine/pattern-borrow.stderr @@ -1,10 +1,17 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/pattern-borrow.rs:9:24 | +LL | #[coroutine] move || { + | ------- within this coroutine LL | if let Test::A(ref _a) = test { | ^^^^^^ LL | yield (); | -------- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | #[coroutine] static move || { + | ++++++ error: aborting due to 1 previous error diff --git a/tests/ui/coroutine/self_referential_gen_block.stderr b/tests/ui/coroutine/self_referential_gen_block.stderr index e23d653d0d4..2f53e7c84a1 100644 --- a/tests/ui/coroutine/self_referential_gen_block.stderr +++ b/tests/ui/coroutine/self_referential_gen_block.stderr @@ -1,6 +1,9 @@ error[E0626]: borrow may still be in use when `gen` block yields --> $DIR/self_referential_gen_block.rs:9:21 | +LL | let mut x = gen { + | --- within this `gen` block +LL | let y = 42; LL | let z = &y; | ^^ LL | yield 43; diff --git a/tests/ui/coroutine/yield-in-args.stderr b/tests/ui/coroutine/yield-in-args.stderr index 1d2c54f9bdb..1cc3c83deb3 100644 --- a/tests/ui/coroutine/yield-in-args.stderr +++ b/tests/ui/coroutine/yield-in-args.stderr @@ -1,8 +1,16 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/yield-in-args.rs:9:13 | +LL | || { + | -- within this coroutine +LL | let b = true; LL | foo(&b, yield); | ^^ ----- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | static || { + | ++++++ error: aborting due to 1 previous error diff --git a/tests/ui/coroutine/yield-while-iterating.stderr b/tests/ui/coroutine/yield-while-iterating.stderr index f81c914c4bd..a92237e44c1 100644 --- a/tests/ui/coroutine/yield-while-iterating.stderr +++ b/tests/ui/coroutine/yield-while-iterating.stderr @@ -1,10 +1,17 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/yield-while-iterating.rs:13:18 | +LL | let _b =#[coroutine] move || { + | ------- within this coroutine LL | for p in &x { | ^^ LL | yield(); | ------- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | let _b =#[coroutine] static move || { + | ++++++ error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable --> $DIR/yield-while-iterating.rs:58:20 diff --git a/tests/ui/coroutine/yield-while-local-borrowed.stderr b/tests/ui/coroutine/yield-while-local-borrowed.stderr index 8fe981de929..b42ca3ba461 100644 --- a/tests/ui/coroutine/yield-while-local-borrowed.stderr +++ b/tests/ui/coroutine/yield-while-local-borrowed.stderr @@ -1,20 +1,35 @@ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/yield-while-local-borrowed.rs:13:17 | +LL | let mut b = #[coroutine] move || { + | ------- within this coroutine LL | let a = &mut 3; | ^^^^^^ LL | LL | yield (); | -------- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | let mut b = #[coroutine] static move || { + | ++++++ error[E0626]: borrow may still be in use when coroutine yields --> $DIR/yield-while-local-borrowed.rs:40:21 | +LL | let mut b = #[coroutine] move || { + | ------- within this coroutine +... LL | let b = &a; | ^^ LL | LL | yield (); | -------- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | let mut b = #[coroutine] static move || { + | ++++++ error: aborting due to 2 previous errors diff --git a/tests/ui/nll/issue-55850.stderr b/tests/ui/nll/issue-55850.stderr index 3d43817f4d8..5a9c7799197 100644 --- a/tests/ui/nll/issue-55850.stderr +++ b/tests/ui/nll/issue-55850.stderr @@ -10,8 +10,16 @@ LL | yield &s[..] error[E0626]: borrow may still be in use when coroutine yields --> $DIR/issue-55850.rs:28:16 | +LL | GenIter(#[coroutine] move || { + | ------- within this coroutine +LL | let mut s = String::new(); LL | yield &s[..] | -------^---- possible yield occurs here + | +help: add `static` to mark this coroutine as unmovable + | +LL | GenIter(#[coroutine] static move || { + | ++++++ error: aborting due to 2 previous errors From 7bca516b357f5cf596250f99353a08f65cb0456a Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 22 Jul 2024 13:54:48 -0400 Subject: [PATCH 04/10] Get rid of can_eq_shallow --- compiler/rustc_infer/src/infer/mod.rs | 12 ------------ .../src/error_reporting/infer/note_and_explain.rs | 5 +++-- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index 7fc4e36d752..3cee0a622f1 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -755,18 +755,6 @@ impl<'tcx> InferCtxt<'tcx> { .collect() } - // FIXME(-Znext-solver): Get rid of this method, it's never correct. Either that, - // or we need to process the obligations. - pub fn can_eq_shallow(&self, param_env: ty::ParamEnv<'tcx>, a: T, b: T) -> bool - where - T: at::ToTrace<'tcx>, - { - let origin = &ObligationCause::dummy(); - // We're only answering whether the types could be the same, and with - // opaque types, "they can be the same", via registering a hidden type. - self.probe(|_| self.at(origin, param_env).eq(DefineOpaqueTypes::Yes, a, b).is_ok()) - } - #[instrument(skip(self), level = "debug")] pub fn sub_regions( &self, diff --git a/compiler/rustc_trait_selection/src/error_reporting/infer/note_and_explain.rs b/compiler/rustc_trait_selection/src/error_reporting/infer/note_and_explain.rs index 1ff2fca83fa..f9110cfb3b9 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/infer/note_and_explain.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/infer/note_and_explain.rs @@ -12,6 +12,7 @@ use rustc_middle::{ use rustc_span::{def_id::DefId, sym, BytePos, Span, Symbol}; use crate::error_reporting::TypeErrCtxt; +use crate::infer::InferCtxtExt; impl<'tcx> TypeErrCtxt<'_, 'tcx> { pub fn note_and_explain_type_err( @@ -821,7 +822,7 @@ fn foo(&self) -> Self::T { String::new() } tcx.defaultness(item.id.owner_id) { let assoc_ty = tcx.type_of(item.id.owner_id).instantiate_identity(); - if self.infcx.can_eq_shallow(param_env, assoc_ty, found) { + if self.infcx.can_eq(param_env, assoc_ty, found) { diag.span_label( item.span, "associated type defaults can't be assumed inside the \ @@ -844,7 +845,7 @@ fn foo(&self) -> Self::T { String::new() } let assoc_ty = tcx.type_of(item.id.owner_id).instantiate_identity(); if let hir::Defaultness::Default { has_value: true } = tcx.defaultness(item.id.owner_id) - && self.infcx.can_eq_shallow(param_env, assoc_ty, found) + && self.infcx.can_eq(param_env, assoc_ty, found) { diag.span_label( item.span, From 6310e40578909d2747d86b75adf70033065511aa Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 22 Jul 2024 16:11:08 -0400 Subject: [PATCH 05/10] Get rid of infer_ctxt_ext --- compiler/rustc_hir_typeck/src/closure.rs | 4 +- .../traits/fulfillment_errors.rs | 235 ++++++++++++++++- .../error_reporting/traits/infer_ctxt_ext.rs | 244 ------------------ .../src/error_reporting/traits/mod.rs | 2 - .../clippy/clippy_lints/src/eta_reduction.rs | 4 +- 5 files changed, 237 insertions(+), 252 deletions(-) delete mode 100644 compiler/rustc_trait_selection/src/error_reporting/traits/infer_ctxt_ext.rs diff --git a/compiler/rustc_hir_typeck/src/closure.rs b/compiler/rustc_hir_typeck/src/closure.rs index 08de871f6fa..79854976bdd 100644 --- a/compiler/rustc_hir_typeck/src/closure.rs +++ b/compiler/rustc_hir_typeck/src/closure.rs @@ -18,7 +18,6 @@ use rustc_span::def_id::LocalDefId; use rustc_span::Span; use rustc_target::spec::abi::Abi; use rustc_trait_selection::error_reporting::traits::ArgKind; -use rustc_trait_selection::error_reporting::traits::InferCtxtExt as _; use rustc_trait_selection::traits; use rustc_type_ir::ClosureKind; use std::iter; @@ -734,13 +733,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .map(|ty| ArgKind::from_expected_ty(*ty, None)) .collect(); let (closure_span, closure_arg_span, found_args) = - match self.get_fn_like_arguments(expr_map_node) { + match self.err_ctxt().get_fn_like_arguments(expr_map_node) { Some((sp, arg_sp, args)) => (Some(sp), arg_sp, args), None => (None, None, Vec::new()), }; let expected_span = expected_sig.cause_span.unwrap_or_else(|| self.tcx.def_span(expr_def_id)); let guar = self + .err_ctxt() .report_arg_count_mismatch( expected_span, closure_span, diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs index 85b37ff3260..a7ea308a818 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs @@ -1,7 +1,6 @@ use super::on_unimplemented::{AppendConstMessage, OnUnimplementedNote}; use super::suggestions::get_explanation_based_on_obligation; use crate::error_reporting::infer::TyCategory; -use crate::error_reporting::traits::infer_ctxt_ext::InferCtxtExt; use crate::error_reporting::traits::report_object_safety_error; use crate::error_reporting::TypeErrCtxt; use crate::errors::{ @@ -2602,7 +2601,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { }) .unwrap_or((found_span, None, found)); - self.infcx.report_arg_count_mismatch( + self.report_arg_count_mismatch( span, closure_span, expected, @@ -2614,6 +2613,238 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { ) } + /// Given some node representing a fn-like thing in the HIR map, + /// returns a span and `ArgKind` information that describes the + /// arguments it expects. This can be supplied to + /// `report_arg_count_mismatch`. + pub fn get_fn_like_arguments( + &self, + node: Node<'_>, + ) -> Option<(Span, Option, Vec)> { + let sm = self.tcx.sess.source_map(); + let hir = self.tcx.hir(); + Some(match node { + Node::Expr(&hir::Expr { + kind: hir::ExprKind::Closure(&hir::Closure { body, fn_decl_span, fn_arg_span, .. }), + .. + }) => ( + fn_decl_span, + fn_arg_span, + hir.body(body) + .params + .iter() + .map(|arg| { + if let hir::Pat { kind: hir::PatKind::Tuple(args, _), span, .. } = *arg.pat + { + Some(ArgKind::Tuple( + Some(span), + args.iter() + .map(|pat| { + sm.span_to_snippet(pat.span) + .ok() + .map(|snippet| (snippet, "_".to_owned())) + }) + .collect::>>()?, + )) + } else { + let name = sm.span_to_snippet(arg.pat.span).ok()?; + Some(ArgKind::Arg(name, "_".to_owned())) + } + }) + .collect::>>()?, + ), + Node::Item(&hir::Item { kind: hir::ItemKind::Fn(ref sig, ..), .. }) + | Node::ImplItem(&hir::ImplItem { kind: hir::ImplItemKind::Fn(ref sig, _), .. }) + | Node::TraitItem(&hir::TraitItem { + kind: hir::TraitItemKind::Fn(ref sig, _), .. + }) => ( + sig.span, + None, + sig.decl + .inputs + .iter() + .map(|arg| match arg.kind { + hir::TyKind::Tup(tys) => ArgKind::Tuple( + Some(arg.span), + vec![("_".to_owned(), "_".to_owned()); tys.len()], + ), + _ => ArgKind::empty(), + }) + .collect::>(), + ), + Node::Ctor(variant_data) => { + let span = variant_data.ctor_hir_id().map_or(DUMMY_SP, |id| hir.span(id)); + (span, None, vec![ArgKind::empty(); variant_data.fields().len()]) + } + _ => panic!("non-FnLike node found: {node:?}"), + }) + } + + /// Reports an error when the number of arguments needed by a + /// trait match doesn't match the number that the expression + /// provides. + pub fn report_arg_count_mismatch( + &self, + span: Span, + found_span: Option, + expected_args: Vec, + found_args: Vec, + is_closure: bool, + closure_arg_span: Option, + ) -> Diag<'a> { + let kind = if is_closure { "closure" } else { "function" }; + + let args_str = |arguments: &[ArgKind], other: &[ArgKind]| { + let arg_length = arguments.len(); + let distinct = matches!(other, &[ArgKind::Tuple(..)]); + match (arg_length, arguments.get(0)) { + (1, Some(ArgKind::Tuple(_, fields))) => { + format!("a single {}-tuple as argument", fields.len()) + } + _ => format!( + "{} {}argument{}", + arg_length, + if distinct && arg_length > 1 { "distinct " } else { "" }, + pluralize!(arg_length) + ), + } + }; + + let expected_str = args_str(&expected_args, &found_args); + let found_str = args_str(&found_args, &expected_args); + + let mut err = struct_span_code_err!( + self.dcx(), + span, + E0593, + "{} is expected to take {}, but it takes {}", + kind, + expected_str, + found_str, + ); + + err.span_label(span, format!("expected {kind} that takes {expected_str}")); + + if let Some(found_span) = found_span { + err.span_label(found_span, format!("takes {found_str}")); + + // Suggest to take and ignore the arguments with expected_args_length `_`s if + // found arguments is empty (assume the user just wants to ignore args in this case). + // For example, if `expected_args_length` is 2, suggest `|_, _|`. + if found_args.is_empty() && is_closure { + let underscores = vec!["_"; expected_args.len()].join(", "); + err.span_suggestion_verbose( + closure_arg_span.unwrap_or(found_span), + format!( + "consider changing the closure to take and ignore the expected argument{}", + pluralize!(expected_args.len()) + ), + format!("|{underscores}|"), + Applicability::MachineApplicable, + ); + } + + if let &[ArgKind::Tuple(_, ref fields)] = &found_args[..] { + if fields.len() == expected_args.len() { + let sugg = fields + .iter() + .map(|(name, _)| name.to_owned()) + .collect::>() + .join(", "); + err.span_suggestion_verbose( + found_span, + "change the closure to take multiple arguments instead of a single tuple", + format!("|{sugg}|"), + Applicability::MachineApplicable, + ); + } + } + if let &[ArgKind::Tuple(_, ref fields)] = &expected_args[..] + && fields.len() == found_args.len() + && is_closure + { + let sugg = format!( + "|({}){}|", + found_args + .iter() + .map(|arg| match arg { + ArgKind::Arg(name, _) => name.to_owned(), + _ => "_".to_owned(), + }) + .collect::>() + .join(", "), + // add type annotations if available + if found_args.iter().any(|arg| match arg { + ArgKind::Arg(_, ty) => ty != "_", + _ => false, + }) { + format!( + ": ({})", + fields + .iter() + .map(|(_, ty)| ty.to_owned()) + .collect::>() + .join(", ") + ) + } else { + String::new() + }, + ); + err.span_suggestion_verbose( + found_span, + "change the closure to accept a tuple instead of individual arguments", + sugg, + Applicability::MachineApplicable, + ); + } + } + + err + } + + /// Checks if the type implements one of `Fn`, `FnMut`, or `FnOnce` + /// in that order, and returns the generic type corresponding to the + /// argument of that trait (corresponding to the closure arguments). + pub fn type_implements_fn_trait( + &self, + param_env: ty::ParamEnv<'tcx>, + ty: ty::Binder<'tcx, Ty<'tcx>>, + polarity: ty::PredicatePolarity, + ) -> Result<(ty::ClosureKind, ty::Binder<'tcx, Ty<'tcx>>), ()> { + self.commit_if_ok(|_| { + for trait_def_id in [ + self.tcx.lang_items().fn_trait(), + self.tcx.lang_items().fn_mut_trait(), + self.tcx.lang_items().fn_once_trait(), + ] { + let Some(trait_def_id) = trait_def_id else { continue }; + // Make a fresh inference variable so we can determine what the generic parameters + // of the trait are. + let var = self.next_ty_var(DUMMY_SP); + // FIXME(effects) + let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty.skip_binder(), var]); + let obligation = Obligation::new( + self.tcx, + ObligationCause::dummy(), + param_env, + ty.rebind(ty::TraitPredicate { trait_ref, polarity }), + ); + let ocx = ObligationCtxt::new(self); + ocx.register_obligation(obligation); + if ocx.select_all_or_error().is_empty() { + return Ok(( + self.tcx + .fn_trait_kind_from_def_id(trait_def_id) + .expect("expected to map DefId to ClosureKind"), + ty.rebind(self.resolve_vars_if_possible(var)), + )); + } + } + + Err(()) + }) + } + fn report_not_const_evaluatable_error( &self, obligation: &PredicateObligation<'tcx>, diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/infer_ctxt_ext.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/infer_ctxt_ext.rs deleted file mode 100644 index e8d7e80ac56..00000000000 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/infer_ctxt_ext.rs +++ /dev/null @@ -1,244 +0,0 @@ -// FIXME(error_reporting): This should be made into private methods on `TypeErrCtxt`. - -use crate::infer::InferCtxt; -use crate::traits::{Obligation, ObligationCause, ObligationCtxt}; -use rustc_errors::{codes::*, pluralize, struct_span_code_err, Applicability, Diag}; -use rustc_hir as hir; -use rustc_hir::Node; -use rustc_macros::extension; -use rustc_middle::ty::{self, Ty}; -use rustc_span::{Span, DUMMY_SP}; - -use super::ArgKind; - -#[extension(pub trait InferCtxtExt<'tcx>)] -impl<'tcx> InferCtxt<'tcx> { - /// Given some node representing a fn-like thing in the HIR map, - /// returns a span and `ArgKind` information that describes the - /// arguments it expects. This can be supplied to - /// `report_arg_count_mismatch`. - fn get_fn_like_arguments(&self, node: Node<'_>) -> Option<(Span, Option, Vec)> { - let sm = self.tcx.sess.source_map(); - let hir = self.tcx.hir(); - Some(match node { - Node::Expr(&hir::Expr { - kind: hir::ExprKind::Closure(&hir::Closure { body, fn_decl_span, fn_arg_span, .. }), - .. - }) => ( - fn_decl_span, - fn_arg_span, - hir.body(body) - .params - .iter() - .map(|arg| { - if let hir::Pat { kind: hir::PatKind::Tuple(args, _), span, .. } = *arg.pat - { - Some(ArgKind::Tuple( - Some(span), - args.iter() - .map(|pat| { - sm.span_to_snippet(pat.span) - .ok() - .map(|snippet| (snippet, "_".to_owned())) - }) - .collect::>>()?, - )) - } else { - let name = sm.span_to_snippet(arg.pat.span).ok()?; - Some(ArgKind::Arg(name, "_".to_owned())) - } - }) - .collect::>>()?, - ), - Node::Item(&hir::Item { kind: hir::ItemKind::Fn(ref sig, ..), .. }) - | Node::ImplItem(&hir::ImplItem { kind: hir::ImplItemKind::Fn(ref sig, _), .. }) - | Node::TraitItem(&hir::TraitItem { - kind: hir::TraitItemKind::Fn(ref sig, _), .. - }) => ( - sig.span, - None, - sig.decl - .inputs - .iter() - .map(|arg| match arg.kind { - hir::TyKind::Tup(tys) => ArgKind::Tuple( - Some(arg.span), - vec![("_".to_owned(), "_".to_owned()); tys.len()], - ), - _ => ArgKind::empty(), - }) - .collect::>(), - ), - Node::Ctor(variant_data) => { - let span = variant_data.ctor_hir_id().map_or(DUMMY_SP, |id| hir.span(id)); - (span, None, vec![ArgKind::empty(); variant_data.fields().len()]) - } - _ => panic!("non-FnLike node found: {node:?}"), - }) - } - - /// Reports an error when the number of arguments needed by a - /// trait match doesn't match the number that the expression - /// provides. - fn report_arg_count_mismatch( - &self, - span: Span, - found_span: Option, - expected_args: Vec, - found_args: Vec, - is_closure: bool, - closure_arg_span: Option, - ) -> Diag<'_> { - let kind = if is_closure { "closure" } else { "function" }; - - let args_str = |arguments: &[ArgKind], other: &[ArgKind]| { - let arg_length = arguments.len(); - let distinct = matches!(other, &[ArgKind::Tuple(..)]); - match (arg_length, arguments.get(0)) { - (1, Some(ArgKind::Tuple(_, fields))) => { - format!("a single {}-tuple as argument", fields.len()) - } - _ => format!( - "{} {}argument{}", - arg_length, - if distinct && arg_length > 1 { "distinct " } else { "" }, - pluralize!(arg_length) - ), - } - }; - - let expected_str = args_str(&expected_args, &found_args); - let found_str = args_str(&found_args, &expected_args); - - let mut err = struct_span_code_err!( - self.dcx(), - span, - E0593, - "{} is expected to take {}, but it takes {}", - kind, - expected_str, - found_str, - ); - - err.span_label(span, format!("expected {kind} that takes {expected_str}")); - - if let Some(found_span) = found_span { - err.span_label(found_span, format!("takes {found_str}")); - - // Suggest to take and ignore the arguments with expected_args_length `_`s if - // found arguments is empty (assume the user just wants to ignore args in this case). - // For example, if `expected_args_length` is 2, suggest `|_, _|`. - if found_args.is_empty() && is_closure { - let underscores = vec!["_"; expected_args.len()].join(", "); - err.span_suggestion_verbose( - closure_arg_span.unwrap_or(found_span), - format!( - "consider changing the closure to take and ignore the expected argument{}", - pluralize!(expected_args.len()) - ), - format!("|{underscores}|"), - Applicability::MachineApplicable, - ); - } - - if let &[ArgKind::Tuple(_, ref fields)] = &found_args[..] { - if fields.len() == expected_args.len() { - let sugg = fields - .iter() - .map(|(name, _)| name.to_owned()) - .collect::>() - .join(", "); - err.span_suggestion_verbose( - found_span, - "change the closure to take multiple arguments instead of a single tuple", - format!("|{sugg}|"), - Applicability::MachineApplicable, - ); - } - } - if let &[ArgKind::Tuple(_, ref fields)] = &expected_args[..] - && fields.len() == found_args.len() - && is_closure - { - let sugg = format!( - "|({}){}|", - found_args - .iter() - .map(|arg| match arg { - ArgKind::Arg(name, _) => name.to_owned(), - _ => "_".to_owned(), - }) - .collect::>() - .join(", "), - // add type annotations if available - if found_args.iter().any(|arg| match arg { - ArgKind::Arg(_, ty) => ty != "_", - _ => false, - }) { - format!( - ": ({})", - fields - .iter() - .map(|(_, ty)| ty.to_owned()) - .collect::>() - .join(", ") - ) - } else { - String::new() - }, - ); - err.span_suggestion_verbose( - found_span, - "change the closure to accept a tuple instead of individual arguments", - sugg, - Applicability::MachineApplicable, - ); - } - } - - err - } - - /// Checks if the type implements one of `Fn`, `FnMut`, or `FnOnce` - /// in that order, and returns the generic type corresponding to the - /// argument of that trait (corresponding to the closure arguments). - fn type_implements_fn_trait( - &self, - param_env: ty::ParamEnv<'tcx>, - ty: ty::Binder<'tcx, Ty<'tcx>>, - polarity: ty::PredicatePolarity, - ) -> Result<(ty::ClosureKind, ty::Binder<'tcx, Ty<'tcx>>), ()> { - self.commit_if_ok(|_| { - for trait_def_id in [ - self.tcx.lang_items().fn_trait(), - self.tcx.lang_items().fn_mut_trait(), - self.tcx.lang_items().fn_once_trait(), - ] { - let Some(trait_def_id) = trait_def_id else { continue }; - // Make a fresh inference variable so we can determine what the generic parameters - // of the trait are. - let var = self.next_ty_var(DUMMY_SP); - // FIXME(effects) - let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty.skip_binder(), var]); - let obligation = Obligation::new( - self.tcx, - ObligationCause::dummy(), - param_env, - ty.rebind(ty::TraitPredicate { trait_ref, polarity }), - ); - let ocx = ObligationCtxt::new(self); - ocx.register_obligation(obligation); - if ocx.select_all_or_error().is_empty() { - return Ok(( - self.tcx - .fn_trait_kind_from_def_id(trait_def_id) - .expect("expected to map DefId to ClosureKind"), - ty.rebind(self.resolve_vars_if_possible(var)), - )); - } - } - - Err(()) - }) - } -} diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs index 10624786bae..87fdc5ddff8 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs @@ -1,6 +1,5 @@ pub mod ambiguity; mod fulfillment_errors; -mod infer_ctxt_ext; pub mod on_unimplemented; mod overflow; pub mod suggestions; @@ -23,7 +22,6 @@ use rustc_span::{ErrorGuaranteed, ExpnKind, Span}; use crate::error_reporting::TypeErrCtxt; use crate::traits::{FulfillmentError, FulfillmentErrorCode}; -pub use self::infer_ctxt_ext::*; pub use self::overflow::*; // When outputting impl candidates, prefer showing those that are more similar. diff --git a/src/tools/clippy/clippy_lints/src/eta_reduction.rs b/src/tools/clippy/clippy_lints/src/eta_reduction.rs index d2a34c75583..0ed7859418b 100644 --- a/src/tools/clippy/clippy_lints/src/eta_reduction.rs +++ b/src/tools/clippy/clippy_lints/src/eta_reduction.rs @@ -15,7 +15,7 @@ use rustc_middle::ty::{ use rustc_session::declare_lint_pass; use rustc_span::symbol::sym; use rustc_target::spec::abi::Abi; -use rustc_trait_selection::error_reporting::traits::InferCtxtExt as _; +use rustc_trait_selection::error_reporting::InferCtxtErrorExt as _; declare_clippy_lint! { /// ### What it does @@ -178,7 +178,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction { // 'cuz currently nothing changes after deleting this check. local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr) }) { - match cx.tcx.infer_ctxt().build().type_implements_fn_trait( + match cx.tcx.infer_ctxt().build().err_ctxt().type_implements_fn_trait( cx.param_env, Binder::bind_with_vars(callee_ty_adjusted, List::empty()), ty::PredicatePolarity::Positive, From 3fdd8d5ef3a8b26c3e24d9baf706fc7fb16717f9 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 18:48:42 -0700 Subject: [PATCH 06/10] compiler: treat `&raw (const|mut) UNSAFE_STATIC` implied deref as safe The implied deref to statics introduced by HIR->THIR lowering is only used to create place expressions, it lacks unsafe semantics. It is also confusing, as there is no visible `*ident` in the source. For both classes of "unsafe static" (extern static and static mut) allow this operation. We lack a clear story around `thread_local! { static mut }`, which is actually its own category of item that reuses the static syntax but has its own rules. It's possible they should be similarly included, but in the absence of a good reason one way or another, we do not bless it. --- .../rustc_mir_build/src/check_unsafety.rs | 18 +++++++++++++ compiler/rustc_mir_build/src/thir/cx/expr.rs | 6 +++-- tests/ui/consts/const_refs_to_static.rs | 2 +- tests/ui/consts/mut-ptr-to-static.rs | 9 +++---- tests/ui/static/raw-ref-deref-with-unsafe.rs | 16 +++++++++++ .../ui/static/raw-ref-deref-without-unsafe.rs | 18 +++++++++++++ .../raw-ref-deref-without-unsafe.stderr | 19 +++++++++++++ tests/ui/static/raw-ref-extern-static.rs | 27 +++++++++++++++++++ tests/ui/static/raw-ref-static-mut.rs | 17 ++++++++++++ 9 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 tests/ui/static/raw-ref-deref-with-unsafe.rs create mode 100644 tests/ui/static/raw-ref-deref-without-unsafe.rs create mode 100644 tests/ui/static/raw-ref-deref-without-unsafe.stderr create mode 100644 tests/ui/static/raw-ref-extern-static.rs create mode 100644 tests/ui/static/raw-ref-static-mut.rs diff --git a/compiler/rustc_mir_build/src/check_unsafety.rs b/compiler/rustc_mir_build/src/check_unsafety.rs index 24098282d93..b4a69d7d7b7 100644 --- a/compiler/rustc_mir_build/src/check_unsafety.rs +++ b/compiler/rustc_mir_build/src/check_unsafety.rs @@ -449,6 +449,24 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { } } } + ExprKind::AddressOf { arg, .. } => { + if let ExprKind::Scope { value: arg, .. } = self.thir[arg].kind + // THIR desugars UNSAFE_STATIC into *UNSAFE_STATIC_REF, where + // UNSAFE_STATIC_REF holds the addr of the UNSAFE_STATIC, so: take two steps + && let ExprKind::Deref { arg } = self.thir[arg].kind + // FIXME(workingjubiee): we lack a clear reason to reject ThreadLocalRef here, + // but we also have no conclusive reason to allow it either! + && let ExprKind::StaticRef { .. } = self.thir[arg].kind + { + // A raw ref to a place expr, even an "unsafe static", is okay! + // We short-circuit to not recursively traverse this expression. + return; + // note: const_mut_refs enables this code, and it currently remains unsafe: + // static mut BYTE: u8 = 0; + // static mut BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(BYTE) }; + // static mut DEREF_BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(*BYTE_PTR) }; + } + } ExprKind::Deref { arg } => { if let ExprKind::StaticRef { def_id, .. } | ExprKind::ThreadLocalRef(def_id) = self.thir[arg].kind diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index bd66257e6b6..0ba31f820b5 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -939,9 +939,11 @@ impl<'tcx> Cx<'tcx> { } } - // We encode uses of statics as a `*&STATIC` where the `&STATIC` part is - // a constant reference (or constant raw pointer for `static mut`) in MIR + // A source Rust `path::to::STATIC` is a place expr like *&ident is. + // In THIR, we make them exactly equivalent by inserting the implied *& or *&raw, + // but distinguish between &STATIC and &THREAD_LOCAL as they have different semantics Res::Def(DefKind::Static { .. }, id) => { + // this is &raw for extern static or static mut, and & for other statics let ty = self.tcx.static_ptr_ty(id); let temp_lifetime = self .rvalue_scopes diff --git a/tests/ui/consts/const_refs_to_static.rs b/tests/ui/consts/const_refs_to_static.rs index 1baa8535b2c..f41725b786e 100644 --- a/tests/ui/consts/const_refs_to_static.rs +++ b/tests/ui/consts/const_refs_to_static.rs @@ -9,7 +9,7 @@ const C1: &i32 = &S; const C1_READ: () = { assert!(*C1 == 0); }; -const C2: *const i32 = unsafe { std::ptr::addr_of!(S_MUT) }; +const C2: *const i32 = std::ptr::addr_of!(S_MUT); fn main() { assert_eq!(*C1, 0); diff --git a/tests/ui/consts/mut-ptr-to-static.rs b/tests/ui/consts/mut-ptr-to-static.rs index d8a788bb37d..e921365c7d4 100644 --- a/tests/ui/consts/mut-ptr-to-static.rs +++ b/tests/ui/consts/mut-ptr-to-static.rs @@ -16,12 +16,9 @@ static mut STATIC: u32 = 42; static INTERIOR_MUTABLE_STATIC: SyncUnsafeCell = SyncUnsafeCell::new(42); // A static that mutably points to STATIC. -static PTR: SyncPtr = SyncPtr { - foo: unsafe { ptr::addr_of_mut!(STATIC) }, -}; -static INTERIOR_MUTABLE_PTR: SyncPtr = SyncPtr { - foo: ptr::addr_of!(INTERIOR_MUTABLE_STATIC) as *mut u32, -}; +static PTR: SyncPtr = SyncPtr { foo: ptr::addr_of_mut!(STATIC) }; +static INTERIOR_MUTABLE_PTR: SyncPtr = + SyncPtr { foo: ptr::addr_of!(INTERIOR_MUTABLE_STATIC) as *mut u32 }; fn main() { let ptr = PTR.foo; diff --git a/tests/ui/static/raw-ref-deref-with-unsafe.rs b/tests/ui/static/raw-ref-deref-with-unsafe.rs new file mode 100644 index 00000000000..4b8f72de5b7 --- /dev/null +++ b/tests/ui/static/raw-ref-deref-with-unsafe.rs @@ -0,0 +1,16 @@ +//@ check-pass +#![feature(const_mut_refs)] +use std::ptr; + +// This code should remain unsafe because of the two unsafe operations here, +// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe. + +static mut BYTE: u8 = 0; +static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE); +// An unsafe static's ident is a place expression in its own right, so despite the above being safe +// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref +static mut DEREF_BYTE_PTR: *mut u8 = unsafe { ptr::addr_of_mut!(*BYTE_PTR) }; + +fn main() { + let _ = unsafe { DEREF_BYTE_PTR }; +} diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.rs b/tests/ui/static/raw-ref-deref-without-unsafe.rs new file mode 100644 index 00000000000..f9bce4368c1 --- /dev/null +++ b/tests/ui/static/raw-ref-deref-without-unsafe.rs @@ -0,0 +1,18 @@ +#![feature(const_mut_refs)] + +use std::ptr; + +// This code should remain unsafe because of the two unsafe operations here, +// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe. + +static mut BYTE: u8 = 0; +static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE); +// An unsafe static's ident is a place expression in its own right, so despite the above being safe +// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref! +static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); +//~^ ERROR: use of mutable static +//~| ERROR: dereference of raw pointer + +fn main() { + let _ = unsafe { DEREF_BYTE_PTR }; +} diff --git a/tests/ui/static/raw-ref-deref-without-unsafe.stderr b/tests/ui/static/raw-ref-deref-without-unsafe.stderr new file mode 100644 index 00000000000..ac4df8b410c --- /dev/null +++ b/tests/ui/static/raw-ref-deref-without-unsafe.stderr @@ -0,0 +1,19 @@ +error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block + --> $DIR/raw-ref-deref-without-unsafe.rs:12:56 + | +LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); + | ^^^^^^^^^ dereference of raw pointer + | + = note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior + +error[E0133]: use of mutable static is unsafe and requires unsafe function or block + --> $DIR/raw-ref-deref-without-unsafe.rs:12:57 + | +LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR); + | ^^^^^^^^ use of mutable static + | + = note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0133`. diff --git a/tests/ui/static/raw-ref-extern-static.rs b/tests/ui/static/raw-ref-extern-static.rs new file mode 100644 index 00000000000..95a53a8640d --- /dev/null +++ b/tests/ui/static/raw-ref-extern-static.rs @@ -0,0 +1,27 @@ +//@ check-pass +#![feature(raw_ref_op)] +use std::ptr; + +// see https://github.com/rust-lang/rust/issues/125833 +// notionally, taking the address of an extern static is a safe operation, +// as we only point at it instead of generating a true reference to it + +// it may potentially induce linker errors, but the safety of that is not about taking addresses! +// any safety obligation of the extern static's correctness in declaration is on the extern itself, +// see RFC 3484 for more on that: https://rust-lang.github.io/rfcs/3484-unsafe-extern-blocks.html + +extern "C" { + static THERE: u8; + static mut SOMEWHERE: u8; +} + +fn main() { + let ptr2there = ptr::addr_of!(THERE); + let ptr2somewhere = ptr::addr_of!(SOMEWHERE); + let ptr2somewhere = ptr::addr_of_mut!(SOMEWHERE); + + // testing both addr_of and the expression it directly expands to + let raw2there = &raw const THERE; + let raw2somewhere = &raw const SOMEWHERE; + let raw2somewhere = &raw mut SOMEWHERE; +} diff --git a/tests/ui/static/raw-ref-static-mut.rs b/tests/ui/static/raw-ref-static-mut.rs new file mode 100644 index 00000000000..6855cc7b050 --- /dev/null +++ b/tests/ui/static/raw-ref-static-mut.rs @@ -0,0 +1,17 @@ +//@ check-pass +#![feature(raw_ref_op)] +use std::ptr; + +// see https://github.com/rust-lang/rust/issues/125833 +// notionally, taking the address of a static mut is a safe operation, +// as we only point at it instead of generating a true reference to it +static mut NOWHERE: usize = 0; + +fn main() { + let p2nowhere = ptr::addr_of!(NOWHERE); + let p2nowhere = ptr::addr_of_mut!(NOWHERE); + + // testing both addr_of and the expression it directly expands to + let raw2nowhere = &raw const NOWHERE; + let raw2nowhere = &raw mut NOWHERE; +} From bf454afcaabb2406dda5e5d880ad061750caf4a8 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 18:28:03 -0700 Subject: [PATCH 07/10] library: vary unsafety in bootstrapping for SEH --- library/panic_unwind/src/seh.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/panic_unwind/src/seh.rs b/library/panic_unwind/src/seh.rs index 04c3d3bf9c3..82c248c5a7b 100644 --- a/library/panic_unwind/src/seh.rs +++ b/library/panic_unwind/src/seh.rs @@ -157,7 +157,10 @@ mod imp { // going to be cross-lang LTOed anyway. However, using expose is shorter and // requires less unsafe. let addr: usize = ptr.expose_provenance(); + #[cfg(bootstrap)] let image_base = unsafe { addr_of!(__ImageBase) }.addr(); + #[cfg(not(bootstrap))] + let image_base = addr_of!(__ImageBase).addr(); let offset: usize = addr - image_base; Self(offset as u32) } @@ -250,7 +253,10 @@ extern "C" { // This is fine since the MSVC runtime uses string comparison on the type name // to match TypeDescriptors rather than pointer equality. static mut TYPE_DESCRIPTOR: _TypeDescriptor = _TypeDescriptor { + #[cfg(bootstrap)] pVFTable: unsafe { addr_of!(TYPE_INFO_VTABLE) } as *const _, + #[cfg(not(bootstrap))] + pVFTable: addr_of!(TYPE_INFO_VTABLE) as *const _, spare: core::ptr::null_mut(), name: TYPE_NAME, }; From b3cd9b5cd3470da7c8f05ed27a8c1db02422b805 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Fri, 31 May 2024 23:40:11 -0700 Subject: [PATCH 08/10] miri: fixup for allowing &raw UNSAFE_STATIC --- src/tools/miri/tests/fail/extern_static.rs | 2 +- src/tools/miri/tests/fail/extern_static.stderr | 4 ++-- src/tools/miri/tests/pass/static_mut.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/tests/fail/extern_static.rs b/src/tools/miri/tests/fail/extern_static.rs index f8805db8d14..0cbf1646be9 100644 --- a/src/tools/miri/tests/fail/extern_static.rs +++ b/src/tools/miri/tests/fail/extern_static.rs @@ -5,5 +5,5 @@ extern "C" { } fn main() { - let _val = unsafe { std::ptr::addr_of!(FOO) }; //~ ERROR: is not supported by Miri + let _val = std::ptr::addr_of!(FOO); //~ ERROR: is not supported by Miri } diff --git a/src/tools/miri/tests/fail/extern_static.stderr b/src/tools/miri/tests/fail/extern_static.stderr index 21759f96019..c7ab128e2fe 100644 --- a/src/tools/miri/tests/fail/extern_static.stderr +++ b/src/tools/miri/tests/fail/extern_static.stderr @@ -1,8 +1,8 @@ error: unsupported operation: extern static `FOO` is not supported by Miri --> $DIR/extern_static.rs:LL:CC | -LL | let _val = unsafe { std::ptr::addr_of!(FOO) }; - | ^^^ extern static `FOO` is not supported by Miri +LL | let _val = std::ptr::addr_of!(FOO); + | ^^^ extern static `FOO` is not supported by Miri | = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support = note: BACKTRACE: diff --git a/src/tools/miri/tests/pass/static_mut.rs b/src/tools/miri/tests/pass/static_mut.rs index 1b416cc4e9b..4488b5a09d5 100644 --- a/src/tools/miri/tests/pass/static_mut.rs +++ b/src/tools/miri/tests/pass/static_mut.rs @@ -2,7 +2,7 @@ use std::ptr::addr_of; static mut FOO: i32 = 42; -static BAR: Foo = Foo(unsafe { addr_of!(FOO) }); +static BAR: Foo = Foo(addr_of!(FOO)); #[allow(dead_code)] struct Foo(*const i32); From ed809e9b79f70719fa9807d363230e576912606b Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 23 Jul 2024 01:12:29 -0700 Subject: [PATCH 09/10] std: Unsafe-wrap alloc code held in-common --- library/std/src/sys/pal/common/alloc.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/library/std/src/sys/pal/common/alloc.rs b/library/std/src/sys/pal/common/alloc.rs index 598b6db71f5..54506c32296 100644 --- a/library/std/src/sys/pal/common/alloc.rs +++ b/library/std/src/sys/pal/common/alloc.rs @@ -1,3 +1,4 @@ +#![forbid(unsafe_op_in_unsafe_fn)] use crate::alloc::{GlobalAlloc, Layout, System}; use crate::cmp; use crate::ptr; @@ -46,14 +47,16 @@ pub unsafe fn realloc_fallback( old_layout: Layout, new_size: usize, ) -> *mut u8 { - // Docs for GlobalAlloc::realloc require this to be valid: - let new_layout = Layout::from_size_align_unchecked(new_size, old_layout.align()); + // SAFETY: Docs for GlobalAlloc::realloc require this to be valid + unsafe { + let new_layout = Layout::from_size_align_unchecked(new_size, old_layout.align()); - let new_ptr = GlobalAlloc::alloc(alloc, new_layout); - if !new_ptr.is_null() { - let size = cmp::min(old_layout.size(), new_size); - ptr::copy_nonoverlapping(ptr, new_ptr, size); - GlobalAlloc::dealloc(alloc, ptr, old_layout); + let new_ptr = GlobalAlloc::alloc(alloc, new_layout); + if !new_ptr.is_null() { + let size = cmp::min(old_layout.size(), new_size); + ptr::copy_nonoverlapping(ptr, new_ptr, size); + GlobalAlloc::dealloc(alloc, ptr, old_layout); + } + new_ptr } - new_ptr } From e4d89bc802bb26fd489aa63733633c7c034830f1 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 23 Jul 2024 01:16:46 -0700 Subject: [PATCH 10/10] std: Unsafe-wrap backtrace code held in-common --- library/std/src/sys/backtrace.rs | 122 ++++++++++++++++--------------- 1 file changed, 63 insertions(+), 59 deletions(-) diff --git a/library/std/src/sys/backtrace.rs b/library/std/src/sys/backtrace.rs index 7401d8ce320..133ea520e30 100644 --- a/library/std/src/sys/backtrace.rs +++ b/library/std/src/sys/backtrace.rs @@ -1,4 +1,5 @@ //! Common code for printing backtraces. +#![forbid(unsafe_op_in_unsafe_fn)] use crate::backtrace_rs::{self, BacktraceFmt, BytesOrWideString, PrintFmt}; use crate::borrow::Cow; @@ -62,73 +63,76 @@ unsafe fn _print_fmt(fmt: &mut fmt::Formatter<'_>, print_fmt: PrintFmt) -> fmt:: // Start immediately if we're not using a short backtrace. let mut start = print_fmt != PrintFmt::Short; set_image_base(); - backtrace_rs::trace_unsynchronized(|frame| { - if print_fmt == PrintFmt::Short && idx > MAX_NB_FRAMES { - return false; - } - - let mut hit = false; - backtrace_rs::resolve_frame_unsynchronized(frame, |symbol| { - hit = true; - - // Any frames between `__rust_begin_short_backtrace` and `__rust_end_short_backtrace` - // are omitted from the backtrace in short mode, `__rust_end_short_backtrace` will be - // called before the panic hook, so we won't ignore any frames if there is no - // invoke of `__rust_begin_short_backtrace`. - if print_fmt == PrintFmt::Short { - if let Some(sym) = symbol.name().and_then(|s| s.as_str()) { - if start && sym.contains("__rust_begin_short_backtrace") { - start = false; - return; - } - if sym.contains("__rust_end_short_backtrace") { - start = true; - return; - } - if !start { - omitted_count += 1; - } - } + // SAFETY: we roll our own locking in this town + unsafe { + backtrace_rs::trace_unsynchronized(|frame| { + if print_fmt == PrintFmt::Short && idx > MAX_NB_FRAMES { + return false; } - if start { - if omitted_count > 0 { - debug_assert!(print_fmt == PrintFmt::Short); - // only print the message between the middle of frames - if !first_omit { - let _ = writeln!( - bt_fmt.formatter(), - " [... omitted {} frame{} ...]", - omitted_count, - if omitted_count > 1 { "s" } else { "" } - ); + let mut hit = false; + backtrace_rs::resolve_frame_unsynchronized(frame, |symbol| { + hit = true; + + // Any frames between `__rust_begin_short_backtrace` and `__rust_end_short_backtrace` + // are omitted from the backtrace in short mode, `__rust_end_short_backtrace` will be + // called before the panic hook, so we won't ignore any frames if there is no + // invoke of `__rust_begin_short_backtrace`. + if print_fmt == PrintFmt::Short { + if let Some(sym) = symbol.name().and_then(|s| s.as_str()) { + if start && sym.contains("__rust_begin_short_backtrace") { + start = false; + return; + } + if sym.contains("__rust_end_short_backtrace") { + start = true; + return; + } + if !start { + omitted_count += 1; + } } - first_omit = false; - omitted_count = 0; } - res = bt_fmt.frame().symbol(frame, symbol); + + if start { + if omitted_count > 0 { + debug_assert!(print_fmt == PrintFmt::Short); + // only print the message between the middle of frames + if !first_omit { + let _ = writeln!( + bt_fmt.formatter(), + " [... omitted {} frame{} ...]", + omitted_count, + if omitted_count > 1 { "s" } else { "" } + ); + } + first_omit = false; + omitted_count = 0; + } + res = bt_fmt.frame().symbol(frame, symbol); + } + }); + #[cfg(target_os = "nto")] + if libc::__my_thread_exit as *mut libc::c_void == frame.ip() { + if !hit && start { + use crate::backtrace_rs::SymbolName; + res = bt_fmt.frame().print_raw( + frame.ip(), + Some(SymbolName::new("__my_thread_exit".as_bytes())), + None, + None, + ); + } + return false; } - }); - #[cfg(target_os = "nto")] - if libc::__my_thread_exit as *mut libc::c_void == frame.ip() { if !hit && start { - use crate::backtrace_rs::SymbolName; - res = bt_fmt.frame().print_raw( - frame.ip(), - Some(SymbolName::new("__my_thread_exit".as_bytes())), - None, - None, - ); + res = bt_fmt.frame().print_raw(frame.ip(), None, None, None); } - return false; - } - if !hit && start { - res = bt_fmt.frame().print_raw(frame.ip(), None, None, None); - } - idx += 1; - res.is_ok() - }); + idx += 1; + res.is_ok() + }) + }; res?; bt_fmt.finish()?; if print_fmt == PrintFmt::Short {