From 58781edc5459d9835b80d819d0469a23bdfc0026 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 12 Apr 2022 16:02:57 +0200 Subject: [PATCH 01/11] update coherence docs, fix opaque type + generator ice --- compiler/rustc_middle/src/traits/mod.rs | 3 ++ .../src/traits/coherence.rs | 27 ++++++++++------- .../src/traits/select/mod.rs | 30 +++++++++++-------- .../ui/coherence/coherence-with-closure.rs | 15 ++++++++++ .../coherence/coherence-with-closure.stderr | 24 +++++++++++++++ .../ui/coherence/coherence-with-generator.rs | 19 ++++++++++++ .../coherence/coherence-with-generator.stderr | 24 +++++++++++++++ 7 files changed, 120 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/coherence/coherence-with-closure.rs create mode 100644 src/test/ui/coherence/coherence-with-closure.stderr create mode 100644 src/test/ui/coherence/coherence-with-generator.rs create mode 100644 src/test/ui/coherence/coherence-with-generator.stderr diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index 7c3d08b26bf54..3f4c43dceff0d 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/compiler/rustc_middle/src/traits/mod.rs @@ -60,6 +60,9 @@ pub enum Reveal { /// let <() as Assoc>::Output = true; /// } /// ``` + /// + /// We also do not reveal the hidden type of opaque types during + /// type-checking. UserFacing, /// At codegen time, all monomorphic projections will succeed. diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 40c3f4047ae6e..37e2eeca86fd2 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -645,7 +645,7 @@ fn orphan_check_trait_ref<'tcx>( .substs .types() .flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate)) - .find(|ty| ty_is_local_constructor(*ty, in_crate)); + .find(|&ty| ty_is_local_constructor(tcx, ty, in_crate)); debug!("orphan_check_trait_ref: uncovered ty local_type: `{:?}`", local_type); @@ -677,7 +677,7 @@ fn contained_non_local_types<'tcx>( ty: Ty<'tcx>, in_crate: InCrate, ) -> Vec> { - if ty_is_local_constructor(ty, in_crate) { + if ty_is_local_constructor(tcx, ty, in_crate) { Vec::new() } else { match fundamental_ty_inner_tys(tcx, ty) { @@ -730,7 +730,7 @@ fn def_id_is_local(def_id: DefId, in_crate: InCrate) -> bool { } } -fn ty_is_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> bool { +fn ty_is_local_constructor(tcx: TyCtxt<'_>, ty: Ty<'_>, in_crate: InCrate) -> bool { debug!("ty_is_local_constructor({:?})", ty); match *ty.kind() { @@ -789,11 +789,6 @@ fn ty_is_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> bool { false } - ty::Closure(..) => { - // Similar to the `Opaque` case (#83613). - false - } - ty::Dynamic(ref tt, ..) => { if let Some(principal) = tt.principal() { def_id_is_local(principal.def_id(), in_crate) @@ -804,8 +799,20 @@ fn ty_is_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> bool { ty::Error(_) => true, - ty::Generator(..) | ty::GeneratorWitness(..) => { - bug!("ty_is_local invoked on unexpected type: {:?}", ty) + // These variants should never appear during coherence checking because they + // cannot be named directly. + // + // They could be indirectly used through an opaque type. While using opaque types + // in impls causes an error, this path can still be hit afterwards. + // + // See `test/ui/coherence/coherence-with-closure.rs` for an example where this + // could happens. + ty::Closure(..) | ty::Generator(..) | ty::GeneratorWitness(..) => { + tcx.sess.delay_span_bug( + DUMMY_SP, + format!("ty_is_local invoked on closure or generator: {:?}", ty), + ); + true } } } diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index e69bf6eca90ef..379f5a120406b 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -103,22 +103,31 @@ pub struct SelectionContext<'cx, 'tcx> { /// require themselves. freshener: TypeFreshener<'cx, 'tcx>, - /// If `true`, indicates that the evaluation should be conservative - /// and consider the possibility of types outside this crate. + /// During coherence we have to assume that other crates may add + /// additional impls which we currently don't know about. + /// + /// To deal with this evaluation should be conservative + /// and consider the possibility of impls from outside this crate. /// This comes up primarily when resolving ambiguity. Imagine /// there is some trait reference `$0: Bar` where `$0` is an /// inference variable. If `intercrate` is true, then we can never /// say for sure that this reference is not implemented, even if /// there are *no impls at all for `Bar`*, because `$0` could be /// bound to some type that in a downstream crate that implements - /// `Bar`. This is the suitable mode for coherence. Elsewhere, - /// though, we set this to false, because we are only interested - /// in types that the user could actually have written --- in - /// other words, we consider `$0: Bar` to be unimplemented if + /// `Bar`. + /// + /// Outside of coherence we set this to false because we are only + /// interested in types that the user could actually have written. + /// In other words, we consider `$0: Bar` to be unimplemented if /// there is no type that the user could *actually name* that /// would satisfy it. This avoids crippling inference, basically. intercrate: bool, - + /// If `intercrate` is set, we remember predicates which were + /// considered ambiguous because of impls potentially added in other crates. + /// This is used in coherence to give improved diagnostics. + /// We don't do his until we detect a coherence error because it can + /// lead to false overflow results (#47139) and because always + /// computing it may negatively impact performance. intercrate_ambiguity_causes: Option>, /// The mode that trait queries run in, which informs our error handling @@ -240,11 +249,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } - /// Enables tracking of intercrate ambiguity causes. These are - /// used in coherence to give improved diagnostics. We don't do - /// this until we detect a coherence error because it can lead to - /// false overflow results (#47139) and because it costs - /// computation time. + /// Enables tracking of intercrate ambiguity causes. See + /// the documentation of [`Self::intercrate_ambiguity_causes`] for more. pub fn enable_tracking_intercrate_ambiguity_causes(&mut self) { assert!(self.intercrate); assert!(self.intercrate_ambiguity_causes.is_none()); diff --git a/src/test/ui/coherence/coherence-with-closure.rs b/src/test/ui/coherence/coherence-with-closure.rs new file mode 100644 index 0000000000000..6e3281d8508ab --- /dev/null +++ b/src/test/ui/coherence/coherence-with-closure.rs @@ -0,0 +1,15 @@ +// Test that encountering closures during coherence does not cause issues. +#![feature(type_alias_impl_trait)] +type OpaqueClosure = impl Sized; +fn defining_use() -> OpaqueClosure { + || () +} + +struct Wrapper(T); +trait Trait {} +impl Trait for Wrapper {} +//~^ ERROR cannot implement trait on type alias impl trait +impl Trait for Wrapper {} +//~^ ERROR conflicting implementations of trait `Trait` for type `Wrapper` + +fn main() {} diff --git a/src/test/ui/coherence/coherence-with-closure.stderr b/src/test/ui/coherence/coherence-with-closure.stderr new file mode 100644 index 0000000000000..20b986cee6913 --- /dev/null +++ b/src/test/ui/coherence/coherence-with-closure.stderr @@ -0,0 +1,24 @@ +error: cannot implement trait on type alias impl trait + --> $DIR/coherence-with-closure.rs:10:24 + | +LL | impl Trait for Wrapper {} + | ^^^^^^^^^^^^^ + | +note: type alias impl trait defined here + --> $DIR/coherence-with-closure.rs:3:22 + | +LL | type OpaqueClosure = impl Sized; + | ^^^^^^^^^^ + +error[E0119]: conflicting implementations of trait `Trait` for type `Wrapper` + --> $DIR/coherence-with-closure.rs:12:1 + | +LL | impl Trait for Wrapper {} + | ------------------------------------- first implementation here +LL | +LL | impl Trait for Wrapper {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Wrapper` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0119`. diff --git a/src/test/ui/coherence/coherence-with-generator.rs b/src/test/ui/coherence/coherence-with-generator.rs new file mode 100644 index 0000000000000..d34c391db9fb0 --- /dev/null +++ b/src/test/ui/coherence/coherence-with-generator.rs @@ -0,0 +1,19 @@ +// Test that encountering closures during coherence does not cause issues. +#![feature(type_alias_impl_trait, generators)] +type OpaqueGenerator = impl Sized; +fn defining_use() -> OpaqueGenerator { + || { + for i in 0..10 { + yield i; + } + } +} + +struct Wrapper(T); +trait Trait {} +impl Trait for Wrapper {} +//~^ ERROR cannot implement trait on type alias impl trait +impl Trait for Wrapper {} +//~^ ERROR conflicting implementations of trait `Trait` for type `Wrapper` + +fn main() {} diff --git a/src/test/ui/coherence/coherence-with-generator.stderr b/src/test/ui/coherence/coherence-with-generator.stderr new file mode 100644 index 0000000000000..249ad3cb9ec7f --- /dev/null +++ b/src/test/ui/coherence/coherence-with-generator.stderr @@ -0,0 +1,24 @@ +error: cannot implement trait on type alias impl trait + --> $DIR/coherence-with-generator.rs:14:24 + | +LL | impl Trait for Wrapper {} + | ^^^^^^^^^^^^^^^ + | +note: type alias impl trait defined here + --> $DIR/coherence-with-generator.rs:3:24 + | +LL | type OpaqueGenerator = impl Sized; + | ^^^^^^^^^^ + +error[E0119]: conflicting implementations of trait `Trait` for type `Wrapper` + --> $DIR/coherence-with-generator.rs:16:1 + | +LL | impl Trait for Wrapper {} + | --------------------------------------- first implementation here +LL | +LL | impl Trait for Wrapper {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Wrapper` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0119`. From 0c92519d01b30996594cc16832fc52f0702a4855 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Mon, 9 May 2022 12:04:53 -0400 Subject: [PATCH 02/11] Make HashMap fall back to RtlGenRandom if BCryptGenRandom fails Issue #84096 changed the hashmap RNG to use BCryptGenRandom instead of RtlGenRandom on Windows. Mozilla Firefox started experiencing random failures in env_logger::Builder::new() (Issue #94098) during initialization of their unsandboxed main process with an "Access Denied" error message from BCryptGenRandom(), which is used by the HashMap contained in env_logger::Builder The root cause appears to be a virus scanner or other software interfering with BCrypt DLLs loading. This change adds a fallback option if BCryptGenRandom is unusable for whatever reason. It will fallback to RtlGenRandom in this case. Fixes #94098 --- library/std/src/sys/windows/c.rs | 4 ++ library/std/src/sys/windows/rand.rs | 83 +++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index 0692da1d79519..0bb6fee60c92e 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -788,6 +788,10 @@ if #[cfg(not(target_vendor = "uwp"))] { #[link(name = "advapi32")] extern "system" { + // Forbidden when targeting UWP + #[link_name = "SystemFunction036"] + pub fn RtlGenRandom(RandomBuffer: *mut u8, RandomBufferLength: ULONG) -> BOOLEAN; + // Allowed but unused by UWP pub fn OpenProcessToken( ProcessHandle: HANDLE, diff --git a/library/std/src/sys/windows/rand.rs b/library/std/src/sys/windows/rand.rs index de73e9154b45e..ec6c40d2f4978 100644 --- a/library/std/src/sys/windows/rand.rs +++ b/library/std/src/sys/windows/rand.rs @@ -1,8 +1,69 @@ use crate::io; use crate::mem; +use crate::sync; use crate::sys::c; +// The kinds of HashMap RNG that may be available +#[derive(Clone, Copy, Debug, PartialEq)] +enum HashMapRng { + Preferred, + Fallback, +} + pub fn hashmap_random_keys() -> (u64, u64) { + match get_hashmap_rng() { + HashMapRng::Preferred => { + preferred_rng().expect("couldn't generate random bytes with preferred RNG") + } + HashMapRng::Fallback => { + fallback_rng().unwrap().expect("couldn't generate random bytes with fallback RNG") + } + } +} + +// Returns the HashMap RNG that should be used +// +// Panics if they are both broken +fn get_hashmap_rng() -> HashMapRng { + // Assume that if the preferred RNG is broken the first time we use it, it likely means + // that: the DLL has failed to load, there is no point to calling it over-and-over again, + // and we should cache the result + static INIT: sync::Once = sync::Once::new(); + static mut HASHMAP_RNG: HashMapRng = HashMapRng::Preferred; + + unsafe { + INIT.call_once(|| HASHMAP_RNG = choose_hashmap_rng()); + HASHMAP_RNG + } +} + +// Test whether we should use the preferred or fallback RNG +// +// If the preferred RNG is successful, we choose it. Otherwise, if the fallback RNG is successful, +// we choose that +// +// Panics if both the preferred and the fallback RNG are both non-functional +fn choose_hashmap_rng() -> HashMapRng { + let preferred_error = match preferred_rng() { + Ok(_) => return HashMapRng::Preferred, + Err(e) => e, + }; + + // On UWP, there is no fallback + let fallback_result = fallback_rng() + .unwrap_or_else(|| panic!("preferred RNG broken: `{}`, no fallback", preferred_error)); + + match fallback_result { + Ok(_) => return HashMapRng::Fallback, + Err(fallback_error) => panic!( + "preferred RNG broken: `{}`, fallback RNG broken: `{}`", + preferred_error, fallback_error + ), + } +} + +// Generate random numbers using the preferred RNG function (BCryptGenRandom) +fn preferred_rng() -> Result<(u64, u64), io::Error> { use crate::ptr; let mut v = (0, 0); @@ -14,8 +75,22 @@ pub fn hashmap_random_keys() -> (u64, u64) { c::BCRYPT_USE_SYSTEM_PREFERRED_RNG, ) }; - if ret != 0 { - panic!("couldn't generate random bytes: {}", io::Error::last_os_error()); - } - return v; + + if ret == 0 { Ok(v) } else { Err(io::Error::last_os_error()) } +} + +// Generate random numbers using the fallback RNG function (RtlGenRandom) +#[cfg(not(target_vendor = "uwp"))] +fn fallback_rng() -> Option> { + let mut v = (0, 0); + let ret = + unsafe { c::RtlGenRandom(&mut v as *mut _ as *mut u8, mem::size_of_val(&v) as c::ULONG) }; + + Some(if ret != 0 { Ok(v) } else { Err(io::Error::last_os_error()) }) +} + +// We can't use RtlGenRandom with UWP, so there is no fallback +#[cfg(target_vendor = "uwp")] +fn fallback_rng() -> Option> { + None } From 3de6c2ca33f45b283ebb177bb29c0c756c6b75cb Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Fri, 13 May 2022 18:14:03 -0400 Subject: [PATCH 03/11] Address review feedback --- library/std/src/sys/windows/rand.rs | 53 ++++++++++++----------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/library/std/src/sys/windows/rand.rs b/library/std/src/sys/windows/rand.rs index ec6c40d2f4978..836fc14898600 100644 --- a/library/std/src/sys/windows/rand.rs +++ b/library/std/src/sys/windows/rand.rs @@ -1,9 +1,9 @@ use crate::io; +use crate::lazy; use crate::mem; -use crate::sync; use crate::sys::c; -// The kinds of HashMap RNG that may be available +/// The kinds of HashMap RNG that may be available #[derive(Clone, Copy, Debug, PartialEq)] enum HashMapRng { Preferred, @@ -16,44 +16,35 @@ pub fn hashmap_random_keys() -> (u64, u64) { preferred_rng().expect("couldn't generate random bytes with preferred RNG") } HashMapRng::Fallback => { - fallback_rng().unwrap().expect("couldn't generate random bytes with fallback RNG") + fallback_rng().expect("couldn't generate random bytes with fallback RNG") } } } -// Returns the HashMap RNG that should be used -// -// Panics if they are both broken +/// Returns the HashMap RNG that should be used +/// +/// Panics if they are both broken fn get_hashmap_rng() -> HashMapRng { // Assume that if the preferred RNG is broken the first time we use it, it likely means // that: the DLL has failed to load, there is no point to calling it over-and-over again, // and we should cache the result - static INIT: sync::Once = sync::Once::new(); - static mut HASHMAP_RNG: HashMapRng = HashMapRng::Preferred; - - unsafe { - INIT.call_once(|| HASHMAP_RNG = choose_hashmap_rng()); - HASHMAP_RNG - } + static VALUE: lazy::SyncOnceCell = lazy::SyncOnceCell::new(); + *VALUE.get_or_init(choose_hashmap_rng) } -// Test whether we should use the preferred or fallback RNG -// -// If the preferred RNG is successful, we choose it. Otherwise, if the fallback RNG is successful, -// we choose that -// -// Panics if both the preferred and the fallback RNG are both non-functional +/// Test whether we should use the preferred or fallback RNG +/// +/// If the preferred RNG is successful, we choose it. Otherwise, if the fallback RNG is successful, +/// we choose that +/// +/// Panics if both the preferred and the fallback RNG are both non-functional fn choose_hashmap_rng() -> HashMapRng { let preferred_error = match preferred_rng() { Ok(_) => return HashMapRng::Preferred, Err(e) => e, }; - // On UWP, there is no fallback - let fallback_result = fallback_rng() - .unwrap_or_else(|| panic!("preferred RNG broken: `{}`, no fallback", preferred_error)); - - match fallback_result { + match fallback_rng() { Ok(_) => return HashMapRng::Fallback, Err(fallback_error) => panic!( "preferred RNG broken: `{}`, fallback RNG broken: `{}`", @@ -62,7 +53,7 @@ fn choose_hashmap_rng() -> HashMapRng { } } -// Generate random numbers using the preferred RNG function (BCryptGenRandom) +/// Generate random numbers using the preferred RNG function (BCryptGenRandom) fn preferred_rng() -> Result<(u64, u64), io::Error> { use crate::ptr; @@ -79,18 +70,18 @@ fn preferred_rng() -> Result<(u64, u64), io::Error> { if ret == 0 { Ok(v) } else { Err(io::Error::last_os_error()) } } -// Generate random numbers using the fallback RNG function (RtlGenRandom) +/// Generate random numbers using the fallback RNG function (RtlGenRandom) #[cfg(not(target_vendor = "uwp"))] -fn fallback_rng() -> Option> { +fn fallback_rng() -> Result<(u64, u64), io::Error> { let mut v = (0, 0); let ret = unsafe { c::RtlGenRandom(&mut v as *mut _ as *mut u8, mem::size_of_val(&v) as c::ULONG) }; - Some(if ret != 0 { Ok(v) } else { Err(io::Error::last_os_error()) }) + if ret != 0 { Ok(v) } else { Err(io::Error::last_os_error()) } } -// We can't use RtlGenRandom with UWP, so there is no fallback +/// We can't use RtlGenRandom with UWP, so there is no fallback #[cfg(target_vendor = "uwp")] -fn fallback_rng() -> Option> { - None +fn fallback_rng() -> Result<(u64, u64), io::Error> { + Err(io::const_io_error!(io::ErrorKind::Unsupported, "unsupported on UWP")) } From aba3454aa136bce4f65978e5bf50683380fbddf3 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Mon, 16 May 2022 13:49:12 -0400 Subject: [PATCH 04/11] Improve error message for fallback RNG failure --- library/std/src/sys/windows/rand.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/windows/rand.rs b/library/std/src/sys/windows/rand.rs index 836fc14898600..22e024d8552ec 100644 --- a/library/std/src/sys/windows/rand.rs +++ b/library/std/src/sys/windows/rand.rs @@ -83,5 +83,5 @@ fn fallback_rng() -> Result<(u64, u64), io::Error> { /// We can't use RtlGenRandom with UWP, so there is no fallback #[cfg(target_vendor = "uwp")] fn fallback_rng() -> Result<(u64, u64), io::Error> { - Err(io::const_io_error!(io::ErrorKind::Unsupported, "unsupported on UWP")) + Err(io::const_io_error!(io::ErrorKind::Unsupported, "RtlGenRandom() not supported on UWP")) } From e68e9775e2718ce0018f902d7cbc98a04923eb42 Mon Sep 17 00:00:00 2001 From: Noa Date: Mon, 16 May 2022 22:56:26 -0500 Subject: [PATCH 05/11] Add tracking issue for ExitCode::exit_process --- library/std/src/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 6c5c08d0bea2c..e253f46406fb7 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1764,7 +1764,7 @@ impl ExitCode { /// code.exit_process() /// } /// ``` - #[unstable(feature = "exitcode_exit_method", issue = "none")] + #[unstable(feature = "exitcode_exit_method", issue = "97100")] pub fn exit_process(self) -> ! { exit(self.to_i32()) } From 4d7e014550444129deb3c83dd588357cb36241b2 Mon Sep 17 00:00:00 2001 From: ricked-twice <39213807+ricked-twice@users.noreply.github.com> Date: Tue, 17 May 2022 20:31:48 +0200 Subject: [PATCH 06/11] Clean fix for #96223 - Modified `InferCtxt::mk_trait_obligation_with_new_self_ty` to take as argument a `Binder<(TraitPredicate, Ty)>` instead of a `Binder` and a separate `Ty` with no bound vars. - Modified all call places to avoid calling `Binder::no_bounds_var` or `Binder::skip_binder` when it is not safe. --- .../src/traits/error_reporting/mod.rs | 12 +- .../src/traits/error_reporting/suggestions.rs | 224 +++++++++--------- 2 files changed, 121 insertions(+), 115 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 6082d7529c32e..266fcc777ef56 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -1384,8 +1384,7 @@ trait InferCtxtPrivExt<'hir, 'tcx> { fn mk_trait_obligation_with_new_self_ty( &self, param_env: ty::ParamEnv<'tcx>, - trait_ref: ty::PolyTraitPredicate<'tcx>, - new_self_ty: Ty<'tcx>, + trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>, ) -> PredicateObligation<'tcx>; fn maybe_report_ambiguity( @@ -1923,14 +1922,11 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> { fn mk_trait_obligation_with_new_self_ty( &self, param_env: ty::ParamEnv<'tcx>, - trait_ref: ty::PolyTraitPredicate<'tcx>, - new_self_ty: Ty<'tcx>, + trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>, ) -> PredicateObligation<'tcx> { - assert!(!new_self_ty.has_escaping_bound_vars()); - - let trait_pred = trait_ref.map_bound_ref(|tr| ty::TraitPredicate { + let trait_pred = trait_ref_and_ty.map_bound_ref(|(tr, new_self_ty)| ty::TraitPredicate { trait_ref: ty::TraitRef { - substs: self.tcx.mk_substs_trait(new_self_ty, &tr.trait_ref.substs[1..]), + substs: self.tcx.mk_substs_trait(*new_self_ty, &tr.trait_ref.substs[1..]), ..tr.trait_ref }, ..*tr diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index 833e232e63665..becf3fdd5ee45 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -637,8 +637,10 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { if let Some(steps) = autoderef.find_map(|(ty, steps)| { // Re-add the `&` let ty = self.tcx.mk_ref(region, TypeAndMut { ty, mutbl }); - let obligation = - self.mk_trait_obligation_with_new_self_ty(param_env, real_trait_pred, ty); + 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(param_env, real_trait_pred_and_ty); Some(steps).filter(|_| self.predicate_may_hold(&obligation)) }) { if steps > 0 { @@ -659,10 +661,11 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { } } else if real_trait_pred != trait_pred { // This branch addresses #87437. + 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( param_env, - real_trait_pred, - base_ty, + real_trait_pred_and_base_ty, ); if self.predicate_may_hold(&obligation) { err.span_suggestion_verbose( @@ -720,9 +723,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { err: &mut Diagnostic, trait_pred: ty::PolyTraitPredicate<'tcx>, ) -> bool { - let Some(self_ty) = trait_pred.self_ty().no_bound_vars() else { - return false; - }; + let self_ty = trait_pred.self_ty().skip_binder(); let (def_id, output_ty, callable) = match *self_ty.kind() { ty::Closure(def_id, substs) => (def_id, substs.as_closure().sig().output(), "closure"), @@ -731,14 +732,12 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { }; let msg = format!("use parentheses to call the {}", callable); - // `mk_trait_obligation_with_new_self_ty` only works for types with no escaping bound - // variables, so bail out if we have any. - let Some(output_ty) = output_ty.no_bound_vars() else { - return false; - }; + let output_ty = self.tcx.liberate_late_bound_regions(def_id, output_ty); + + let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output_ty)); let new_obligation = - self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, output_ty); + self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred_and_self); match self.evaluate_obligation(&new_obligation) { Ok( @@ -842,96 +841,102 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { let param_env = obligation.param_env; // Try to apply the original trait binding obligation by borrowing. - let mut try_borrowing = |old_pred: ty::PolyTraitPredicate<'tcx>, - blacklist: &[DefId]| - -> bool { - if blacklist.contains(&old_pred.def_id()) { - return false; - } + let mut try_borrowing = + |old_pred: ty::PolyTraitPredicate<'tcx>, blacklist: &[DefId]| -> bool { + if blacklist.contains(&old_pred.def_id()) { + return false; + } + // We map bounds to `&T` and `&mut T` + let trait_pred_and_imm_ref = old_pred.map_bound(|trait_pred| { + ( + trait_pred, + self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()), + ) + }); + let trait_pred_and_mut_ref = old_pred.map_bound(|trait_pred| { + ( + trait_pred, + self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()), + ) + }); - // This is a quick fix to resolve an ICE (#96223). - // This change should probably be deeper. - // As suggested by @jackh726, `mk_trait_obligation_with_new_self_ty` could take a `Binder<(TraitRef, Ty)> - // instead of `Binder` leading to some changes to its call places. - let Some(orig_ty) = old_pred.self_ty().no_bound_vars() else { - return false; - }; - let mk_result = |new_ty| { - let obligation = - self.mk_trait_obligation_with_new_self_ty(param_env, old_pred, new_ty); - self.predicate_must_hold_modulo_regions(&obligation) - }; - let imm_result = mk_result(self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, orig_ty)); - let mut_result = mk_result(self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, orig_ty)); - - if imm_result || mut_result { - if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { - // We have a very specific type of error, where just borrowing this argument - // might solve the problem. In cases like this, the important part is the - // original type obligation, not the last one that failed, which is arbitrary. - // Because of this, we modify the error to refer to the original obligation and - // return early in the caller. - - let msg = format!( - "the trait bound `{}: {}` is not satisfied", - orig_ty, - old_pred.print_modifiers_and_trait_path(), - ); - if has_custom_message { - err.note(&msg); - } else { - err.message = - vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)]; - } - if snippet.starts_with('&') { - // This is already a literal borrow and the obligation is failing - // somewhere else in the obligation chain. Do not suggest non-sense. - return false; - } - err.span_label( - span, - &format!( - "expected an implementor of trait `{}`", + let mk_result = |trait_pred_and_new_ty| { + let obligation = + self.mk_trait_obligation_with_new_self_ty(param_env, trait_pred_and_new_ty); + self.predicate_must_hold_modulo_regions(&obligation) + }; + let imm_result = mk_result(trait_pred_and_imm_ref); + let mut_result = mk_result(trait_pred_and_mut_ref); + + if imm_result || mut_result { + if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { + // We have a very specific type of error, where just borrowing this argument + // might solve the problem. In cases like this, the important part is the + // original type obligation, not the last one that failed, which is arbitrary. + // Because of this, we modify the error to refer to the original obligation and + // return early in the caller. + + let msg = format!( + "the trait bound `{}: {}` is not satisfied", + // Safe to skip binder here + old_pred.self_ty().skip_binder(), old_pred.print_modifiers_and_trait_path(), - ), - ); - - // This if is to prevent a special edge-case - if matches!( - span.ctxt().outer_expn_data().kind, - ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop) - ) { - // We don't want a borrowing suggestion on the fields in structs, - // ``` - // struct Foo { - // the_foos: Vec - // } - // ``` - - if imm_result && mut_result { - err.span_suggestions( - span.shrink_to_lo(), - "consider borrowing here", - ["&".to_string(), "&mut ".to_string()].into_iter(), - Applicability::MaybeIncorrect, - ); + ); + if has_custom_message { + err.note(&msg); } else { - err.span_suggestion_verbose( - span.shrink_to_lo(), - &format!( - "consider{} borrowing here", - if mut_result { " mutably" } else { "" } - ), - format!("&{}", if mut_result { "mut " } else { "" }), - Applicability::MaybeIncorrect, - ); + err.message = + vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)]; } + if snippet.starts_with('&') { + // This is already a literal borrow and the obligation is failing + // somewhere else in the obligation chain. Do not suggest non-sense. + return false; + } + err.span_label( + span, + &format!( + "expected an implementor of trait `{}`", + old_pred.print_modifiers_and_trait_path(), + ), + ); + + // This if is to prevent a special edge-case + if matches!( + span.ctxt().outer_expn_data().kind, + ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop) + ) { + // We don't want a borrowing suggestion on the fields in structs, + // ``` + // struct Foo { + // the_foos: Vec + // } + // ``` + + if imm_result && mut_result { + err.span_suggestions( + span.shrink_to_lo(), + "consider borrowing here", + ["&".to_string(), "&mut ".to_string()].into_iter(), + Applicability::MaybeIncorrect, + ); + } else { + err.span_suggestion_verbose( + span.shrink_to_lo(), + &format!( + "consider{} borrowing here", + if mut_result { " mutably" } else { "" } + ), + format!("&{}", if mut_result { "mut " } else { "" }), + Applicability::MaybeIncorrect, + ); + } + } + return true; } - return true; } - } - return false; - }; + return false; + }; if let ObligationCauseCode::ImplDerivedObligation(cause) = &*code { try_borrowing(cause.derived.parent_trait_pred, &[]) @@ -992,9 +997,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return false; } - let Some(mut suggested_ty) = trait_pred.self_ty().no_bound_vars() else { - return false; - }; + // We skip binder here + let mut suggested_ty = trait_pred.self_ty().skip_binder(); for refs_remaining in 0..refs_number { let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else { @@ -1002,10 +1006,13 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { }; suggested_ty = *inner_ty; + // We remap bounds here + let trait_pred_and_suggested_ty = + trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty)); + let new_obligation = self.mk_trait_obligation_with_new_self_ty( obligation.param_env, - trait_pred, - suggested_ty, + trait_pred_and_suggested_ty, ); if self.predicate_may_hold(&new_obligation) { @@ -1141,10 +1148,12 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { hir::Mutability::Not => self.tcx.mk_mut_ref(region, t_type), }; + let trait_pred_and_suggested_ty = + trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty)); + let new_obligation = self.mk_trait_obligation_with_new_self_ty( obligation.param_env, - trait_pred, - suggested_ty, + trait_pred_and_suggested_ty, ); let suggested_ty_would_satisfy_obligation = self .evaluate_obligation_no_overflow(&new_obligation) @@ -1195,7 +1204,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { // Only suggest this if the expression behind the semicolon implements the predicate && let Some(typeck_results) = self.in_progress_typeck_results && let Some(ty) = typeck_results.borrow().expr_ty_opt(expr) - && self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, ty)) + && self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty( + obligation.param_env, trait_pred.map_bound(|trait_pred| (trait_pred, ty)) + )) { err.span_label( expr.span, @@ -2727,8 +2738,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { ); let try_obligation = self.mk_trait_obligation_with_new_self_ty( obligation.param_env, - trait_pred, - normalized_ty.ty().unwrap(), + trait_pred.map_bound(|trait_pred| (trait_pred, normalized_ty.ty().unwrap())), ); debug!("suggest_await_before_try: try_trait_obligation {:?}", try_obligation); if self.predicate_may_hold(&try_obligation) From ac5366b669b5a4f9a6c7e231ac6f99e709c44fe9 Mon Sep 17 00:00:00 2001 From: ricked-twice <39213807+ricked-twice@users.noreply.github.com> Date: Tue, 17 May 2022 22:59:13 +0200 Subject: [PATCH 07/11] Taking review into account --- .../src/traits/error_reporting/suggestions.rs | 36 +++++++++---------- src/test/ui/binop/issue-77910-1.stderr | 14 +++----- ...-trait-object-literal-bound-regions.stderr | 1 + 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index becf3fdd5ee45..c3ee849d85716 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -628,15 +628,17 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { if let Some(parent_trait_pred) = parent_trait_pred { real_trait_pred = parent_trait_pred; } - let Some(real_ty) = real_trait_pred.self_ty().no_bound_vars() else { - continue; - }; + + // Skipping binder here, remapping below + let real_ty = real_trait_pred.self_ty().skip_binder(); if let ty::Ref(region, base_ty, mutbl) = *real_ty.kind() { let mut autoderef = Autoderef::new(self, param_env, body_id, span, base_ty, span); if let Some(steps) = autoderef.find_map(|(ty, steps)| { // Re-add the `&` let ty = self.tcx.mk_ref(region, TypeAndMut { 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 @@ -661,6 +663,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { } } else if real_trait_pred != trait_pred { // This branch addresses #87437. + + // 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( @@ -723,6 +727,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { err: &mut Diagnostic, trait_pred: ty::PolyTraitPredicate<'tcx>, ) -> bool { + // Skipping binder here, remapping below let self_ty = trait_pred.self_ty().skip_binder(); let (def_id, output_ty, callable) = match *self_ty.kind() { @@ -732,8 +737,11 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { }; let msg = format!("use parentheses to call the {}", callable); + // "We should really create a single list of bound vars from the combined vars + // from the predicate and function, but instead we just liberate the function bound vars" let output_ty = self.tcx.liberate_late_bound_regions(def_id, output_ty); + // Remapping bound vars here let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output_ty)); let new_obligation = @@ -876,12 +884,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { // Because of this, we modify the error to refer to the original obligation and // return early in the caller. - let msg = format!( - "the trait bound `{}: {}` is not satisfied", - // Safe to skip binder here - old_pred.self_ty().skip_binder(), - old_pred.print_modifiers_and_trait_path(), - ); + let msg = format!("the trait bound `{}` is not satisfied", old_pred); if has_custom_message { err.note(&msg); } else { @@ -997,7 +1000,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return false; } - // We skip binder here + // Skipping binder here, remapping below let mut suggested_ty = trait_pred.self_ty().skip_binder(); for refs_remaining in 0..refs_number { @@ -1006,7 +1009,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { }; suggested_ty = *inner_ty; - // We remap bounds here + // Remapping bound vars here let trait_pred_and_suggested_ty = trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty)); @@ -1132,22 +1135,15 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return; } + // Skipping binder here, remapping below if let ty::Ref(region, t_type, mutability) = *trait_pred.skip_binder().self_ty().kind() { - if region.is_late_bound() || t_type.has_escaping_bound_vars() { - // Avoid debug assertion in `mk_obligation_for_def_id`. - // - // If the self type has escaping bound vars then it's not - // going to be the type of an expression, so the suggestion - // probably won't apply anyway. - return; - } - let suggested_ty = match mutability { hir::Mutability::Mut => self.tcx.mk_imm_ref(region, t_type), hir::Mutability::Not => self.tcx.mk_mut_ref(region, t_type), }; + // Remapping bound vars here let trait_pred_and_suggested_ty = trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty)); diff --git a/src/test/ui/binop/issue-77910-1.stderr b/src/test/ui/binop/issue-77910-1.stderr index 95ee51a88261a..68303b842088e 100644 --- a/src/test/ui/binop/issue-77910-1.stderr +++ b/src/test/ui/binop/issue-77910-1.stderr @@ -12,20 +12,14 @@ LL | assert_eq!(foo, y); error[E0277]: `for<'r> fn(&'r i32) -> &'r i32 {foo}` doesn't implement `Debug` --> $DIR/issue-77910-1.rs:8:5 | +LL | fn foo(s: &i32) -> &i32 { + | --- consider calling this function +... LL | assert_eq!(foo, y); | ^^^^^^^^^^^^^^^^^^ `for<'r> fn(&'r i32) -> &'r i32 {foo}` cannot be formatted using `{:?}` because it doesn't implement `Debug` | = help: the trait `Debug` is not implemented for `for<'r> fn(&'r i32) -> &'r i32 {foo}` - = help: the following other types implement trait `Debug`: - extern "C" fn() -> Ret - extern "C" fn(A) -> Ret - extern "C" fn(A, ...) -> Ret - extern "C" fn(A, B) -> Ret - extern "C" fn(A, B, ...) -> Ret - extern "C" fn(A, B, C) -> Ret - extern "C" fn(A, B, C, ...) -> Ret - extern "C" fn(A, B, C, D) -> Ret - and 68 others + = help: use parentheses to call the function: `foo(s)` = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) error: aborting due to 2 previous errors diff --git a/src/test/ui/suggestions/imm-ref-trait-object-literal-bound-regions.stderr b/src/test/ui/suggestions/imm-ref-trait-object-literal-bound-regions.stderr index 0783f04dc9bd3..ba6af8f15fa89 100644 --- a/src/test/ui/suggestions/imm-ref-trait-object-literal-bound-regions.stderr +++ b/src/test/ui/suggestions/imm-ref-trait-object-literal-bound-regions.stderr @@ -5,6 +5,7 @@ LL | foo::(s); | ^^^^^^^^ the trait `for<'b> Trait` is not implemented for `&'b S` | = help: the trait `Trait` is implemented for `&'a mut S` + = note: `for<'b> Trait` is implemented for `&'b mut S`, but not for `&'b S` note: required by a bound in `foo` --> $DIR/imm-ref-trait-object-literal-bound-regions.rs:11:20 | From a5c4f4cc4b14b2dff2506d08defb1b97adcbf333 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 4 Mar 2022 21:59:13 -0800 Subject: [PATCH 08/11] Suggest deref non-lvalue mutable reference --- compiler/rustc_typeck/src/check/expr.rs | 22 +++++++++++++++++-- compiler/rustc_typeck/src/check/op.rs | 2 +- .../ui/typeck/assign-non-lval-mut-ref.fixed | 7 ++++++ src/test/ui/typeck/assign-non-lval-mut-ref.rs | 7 ++++++ .../ui/typeck/assign-non-lval-mut-ref.stderr | 16 ++++++++++++++ src/test/ui/typeck/issue-93486.stderr | 5 +++++ 6 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/typeck/assign-non-lval-mut-ref.fixed create mode 100644 src/test/ui/typeck/assign-non-lval-mut-ref.rs create mode 100644 src/test/ui/typeck/assign-non-lval-mut-ref.stderr diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index f3a5b9f13dd1b..9015868b49992 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -836,6 +836,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { lhs: &'tcx hir::Expr<'tcx>, err_code: &'static str, op_span: Span, + adjust_err: impl FnOnce(&mut DiagnosticBuilder<'tcx, ErrorGuaranteed>), ) { if lhs.is_syntactic_place_expr() { return; @@ -858,6 +859,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); }); + adjust_err(&mut err); + err.emit(); } @@ -1050,9 +1053,24 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return self.tcx.ty_error(); } - self.check_lhs_assignable(lhs, "E0070", span); - let lhs_ty = self.check_expr_with_needs(&lhs, Needs::MutPlace); + + self.check_lhs_assignable(lhs, "E0070", span, |err| { + let rhs_ty = self.check_expr(&rhs); + + if let ty::Ref(_, lhs_inner_ty, hir::Mutability::Mut) = lhs_ty.kind() { + if self.can_coerce(rhs_ty, *lhs_inner_ty) { + err.span_suggestion_verbose( + lhs.span.shrink_to_lo(), + "consider dereferencing here to assign to the mutable \ + borrowed piece of memory", + "*".to_string(), + Applicability::MachineApplicable, + ); + } + } + }); + let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty, Some(lhs)); self.require_type_is_sized(lhs_ty, lhs.span, traits::AssignmentLhsSized); diff --git a/compiler/rustc_typeck/src/check/op.rs b/compiler/rustc_typeck/src/check/op.rs index 1ae53a77adc56..5066e21dc8d85 100644 --- a/compiler/rustc_typeck/src/check/op.rs +++ b/compiler/rustc_typeck/src/check/op.rs @@ -41,7 +41,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return_ty }; - self.check_lhs_assignable(lhs, "E0067", op.span); + self.check_lhs_assignable(lhs, "E0067", op.span, |_| {}); ty } diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed new file mode 100644 index 0000000000000..76e2afc672a1a --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed @@ -0,0 +1,7 @@ +// run-rustfix + +fn main() { + let mut x = vec![1usize]; + *x.last_mut().unwrap() = 2usize; + //~^ ERROR invalid left-hand side of assignment +} diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.rs b/src/test/ui/typeck/assign-non-lval-mut-ref.rs new file mode 100644 index 0000000000000..ff91f2297c814 --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.rs @@ -0,0 +1,7 @@ +// run-rustfix + +fn main() { + let mut x = vec![1usize]; + x.last_mut().unwrap() = 2usize; + //~^ ERROR invalid left-hand side of assignment +} diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr new file mode 100644 index 0000000000000..745ada5de0bc3 --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr @@ -0,0 +1,16 @@ +error[E0070]: invalid left-hand side of assignment + --> $DIR/assign-non-lval-mut-ref.rs:5:27 + | +LL | x.last_mut().unwrap() = 2usize; + | --------------------- ^ + | | + | cannot assign to this expression + | +help: consider dereferencing here to assign to the mutable borrowed piece of memory + | +LL | *x.last_mut().unwrap() = 2usize; + | + + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0070`. diff --git a/src/test/ui/typeck/issue-93486.stderr b/src/test/ui/typeck/issue-93486.stderr index 70b5b63f1cba7..95eb021965f4d 100644 --- a/src/test/ui/typeck/issue-93486.stderr +++ b/src/test/ui/typeck/issue-93486.stderr @@ -5,6 +5,11 @@ LL | vec![].last_mut().unwrap() = 3_u8; | -------------------------- ^ | | | cannot assign to this expression + | +help: consider dereferencing here to assign to the mutable borrowed piece of memory + | +LL | *vec![].last_mut().unwrap() = 3_u8; + | + error: aborting due to previous error From b26580f2149c7f4196eac76525cc1d53f215b29b Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 4 Mar 2022 23:17:31 -0800 Subject: [PATCH 09/11] better error for bad LHS in binop-assign --- compiler/rustc_typeck/src/check/expr.rs | 2 ++ compiler/rustc_typeck/src/check/op.rs | 24 ++++++++++++++++--- src/test/ui/issues/issue-5239-1.stderr | 5 ---- .../ui/typeck/assign-non-lval-mut-ref.fixed | 4 +++- src/test/ui/typeck/assign-non-lval-mut-ref.rs | 4 +++- .../ui/typeck/assign-non-lval-mut-ref.stderr | 22 +++++++++++++---- 6 files changed, 47 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 9015868b49992..bf438b4498903 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -1058,6 +1058,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.check_lhs_assignable(lhs, "E0070", span, |err| { let rhs_ty = self.check_expr(&rhs); + // FIXME: This could be done any time lhs_ty is DerefMut into something that + // is compatible with rhs_ty, and not _just_ `&mut` if let ty::Ref(_, lhs_inner_ty, hir::Mutability::Mut) = lhs_ty.kind() { if self.can_coerce(rhs_ty, *lhs_inner_ty) { err.span_suggestion_verbose( diff --git a/compiler/rustc_typeck/src/check/op.rs b/compiler/rustc_typeck/src/check/op.rs index 5066e21dc8d85..d250f38fffa3c 100644 --- a/compiler/rustc_typeck/src/check/op.rs +++ b/compiler/rustc_typeck/src/check/op.rs @@ -41,7 +41,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return_ty }; - self.check_lhs_assignable(lhs, "E0067", op.span, |_| {}); + self.check_lhs_assignable(lhs, "E0067", op.span, |err| { + if let Ref(_, rty, hir::Mutability::Mut) = lhs_ty.kind() { + if self + .lookup_op_method(*rty, Some(rhs_ty), Some(rhs), Op::Binary(op, IsAssign::Yes)) + .is_ok() + { + // Suppress this error, since we already emitted + // a deref suggestion in check_overloaded_binop + err.delay_as_bug(); + } + } + }); ty } @@ -404,8 +415,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { (err, missing_trait, use_output) } }; - if let Ref(_, rty, _) = lhs_ty.kind() { - if self.infcx.type_is_copy_modulo_regions(self.param_env, *rty, lhs_expr.span) + if let Ref(_, rty, mutability) = lhs_ty.kind() { + let is_copy = + self.infcx.type_is_copy_modulo_regions(self.param_env, *rty, lhs_expr.span); + // We should suggest `a + b` => `*a + b` if `a` is copy, and suggest + // `a += b` => `*a += b` if a is a mut ref. + // FIXME: This could be done any time lhs_ty is DerefMut into something that + // is compatible with rhs_ty, and not _just_ `&mut` (for IsAssign::Yes). + if ((is_assign == IsAssign::No && is_copy) + || (is_assign == IsAssign::Yes && *mutability == hir::Mutability::Mut)) && self .lookup_op_method( *rty, diff --git a/src/test/ui/issues/issue-5239-1.stderr b/src/test/ui/issues/issue-5239-1.stderr index b743f0df4b11a..f53ddb95416d1 100644 --- a/src/test/ui/issues/issue-5239-1.stderr +++ b/src/test/ui/issues/issue-5239-1.stderr @@ -5,11 +5,6 @@ LL | let x = |ref x: isize| { x += 1; }; | -^^^^^ | | | cannot use `+=` on type `&isize` - | -help: `+=` can be used on `isize`, you can dereference `x` - | -LL | let x = |ref x: isize| { *x += 1; }; - | + error: aborting due to previous error diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed index 76e2afc672a1a..c4dadfbdfcea4 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed @@ -2,6 +2,8 @@ fn main() { let mut x = vec![1usize]; - *x.last_mut().unwrap() = 2usize; + *x.last_mut().unwrap() = 2; //~^ ERROR invalid left-hand side of assignment + *x.last_mut().unwrap() += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` } diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.rs b/src/test/ui/typeck/assign-non-lval-mut-ref.rs index ff91f2297c814..39573ddb6d0e8 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.rs +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.rs @@ -2,6 +2,8 @@ fn main() { let mut x = vec![1usize]; - x.last_mut().unwrap() = 2usize; + x.last_mut().unwrap() = 2; //~^ ERROR invalid left-hand side of assignment + x.last_mut().unwrap() += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` } diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr index 745ada5de0bc3..b0ca089b7092e 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr @@ -1,16 +1,30 @@ error[E0070]: invalid left-hand side of assignment --> $DIR/assign-non-lval-mut-ref.rs:5:27 | -LL | x.last_mut().unwrap() = 2usize; +LL | x.last_mut().unwrap() = 2; | --------------------- ^ | | | cannot assign to this expression | help: consider dereferencing here to assign to the mutable borrowed piece of memory | -LL | *x.last_mut().unwrap() = 2usize; +LL | *x.last_mut().unwrap() = 2; | + -error: aborting due to previous error +error[E0368]: binary assignment operation `+=` cannot be applied to type `&mut usize` + --> $DIR/assign-non-lval-mut-ref.rs:7:5 + | +LL | x.last_mut().unwrap() += 1; + | ---------------------^^^^^ + | | + | cannot use `+=` on type `&mut usize` + | +help: `+=` can be used on `usize`, you can dereference `x.last_mut().unwrap()` + | +LL | *x.last_mut().unwrap() += 1; + | + + +error: aborting due to 2 previous errors -For more information about this error, try `rustc --explain E0070`. +Some errors have detailed explanations: E0070, E0368. +For more information about an error, try `rustc --explain E0070`. From d50d3fccdd3038b2621245e9591ea6e20eebde2a Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 5 Mar 2022 01:57:43 -0800 Subject: [PATCH 10/11] better lvalue errors for things implementing DerefMut --- compiler/rustc_typeck/src/check/coercion.rs | 23 +++++++- compiler/rustc_typeck/src/check/demand.rs | 21 +------ compiler/rustc_typeck/src/check/expr.rs | 41 +++++++++---- compiler/rustc_typeck/src/check/op.rs | 50 +++++++++------- .../suggestions/mut-ref-reassignment.stderr | 4 +- .../ui/typeck/assign-non-lval-derefmut.fixed | 15 +++++ .../ui/typeck/assign-non-lval-derefmut.rs | 15 +++++ .../ui/typeck/assign-non-lval-derefmut.stderr | 58 +++++++++++++++++++ .../ui/typeck/assign-non-lval-mut-ref.fixed | 6 ++ src/test/ui/typeck/assign-non-lval-mut-ref.rs | 6 ++ .../ui/typeck/assign-non-lval-mut-ref.stderr | 32 +++++++++- src/test/ui/typeck/issue-93486.stderr | 2 +- 12 files changed, 218 insertions(+), 55 deletions(-) create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.fixed create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.rs create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.stderr diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs index 57d3079935415..22683b1de75f1 100644 --- a/compiler/rustc_typeck/src/check/coercion.rs +++ b/compiler/rustc_typeck/src/check/coercion.rs @@ -58,7 +58,8 @@ use rustc_session::parse::feature_err; use rustc_span::symbol::sym; use rustc_span::{self, BytePos, DesugaringKind, Span}; use rustc_target::spec::abi::Abi; -use rustc_trait_selection::traits::error_reporting::InferCtxtExt; +use rustc_trait_selection::infer::InferCtxtExt as _; +use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _; use rustc_trait_selection::traits::{self, ObligationCause, ObligationCauseCode}; use smallvec::{smallvec, SmallVec}; @@ -962,6 +963,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .find_map(|(ty, steps)| self.probe(|_| coerce.unify(ty, target)).ok().map(|_| steps)) } + /// Given a type, this function will calculate and return the type given + /// for `::Target` only if `Ty` also implements `DerefMut`. + /// + /// This function is for diagnostics only, since it does not register + /// trait or region sub-obligations. (presumably we could, but it's not + /// particularly important for diagnostics...) + pub fn deref_once_mutably_for_diagnostic(&self, expr_ty: Ty<'tcx>) -> Option> { + self.autoderef(rustc_span::DUMMY_SP, expr_ty).nth(1).and_then(|(deref_ty, _)| { + self.infcx + .type_implements_trait( + self.infcx.tcx.lang_items().deref_mut_trait()?, + expr_ty, + ty::List::empty(), + self.param_env, + ) + .may_apply() + .then(|| deref_ty) + }) + } + /// Given some expressions, their known unified type and another expression, /// tries to unify the types, potentially inserting coercions on any of the /// provided expressions and returns their LUB (aka "common supertype"). diff --git a/compiler/rustc_typeck/src/check/demand.rs b/compiler/rustc_typeck/src/check/demand.rs index ae4cebe866bdb..ede2180a8e9e7 100644 --- a/compiler/rustc_typeck/src/check/demand.rs +++ b/compiler/rustc_typeck/src/check/demand.rs @@ -696,28 +696,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; if let Some(hir::Node::Expr(hir::Expr { - kind: hir::ExprKind::Assign(left_expr, ..), + kind: hir::ExprKind::Assign(..), .. })) = self.tcx.hir().find(self.tcx.hir().get_parent_node(expr.hir_id)) { if mutability == hir::Mutability::Mut { - // Found the following case: - // fn foo(opt: &mut Option){ opt = None } - // --- ^^^^ - // | | - // consider dereferencing here: `*opt` | - // expected mutable reference, found enum `Option` - if sm.span_to_snippet(left_expr.span).is_ok() { - return Some(( - left_expr.span.shrink_to_lo(), - "consider dereferencing here to assign to the mutable \ - borrowed piece of memory" - .to_string(), - "*".to_string(), - Applicability::MachineApplicable, - true, - )); - } + // Suppressing this diagnostic, we'll properly print it in `check_expr_assign` + return None; } } diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index bf438b4498903..6d56445771a07 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -51,6 +51,7 @@ use rustc_span::lev_distance::find_best_match_for_name; use rustc_span::source_map::Span; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{BytePos, Pos}; +use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::{self, ObligationCauseCode}; impl<'a, 'tcx> FnCtxt<'a, 'tcx> { @@ -1055,25 +1056,45 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let lhs_ty = self.check_expr_with_needs(&lhs, Needs::MutPlace); - self.check_lhs_assignable(lhs, "E0070", span, |err| { - let rhs_ty = self.check_expr(&rhs); - - // FIXME: This could be done any time lhs_ty is DerefMut into something that - // is compatible with rhs_ty, and not _just_ `&mut` - if let ty::Ref(_, lhs_inner_ty, hir::Mutability::Mut) = lhs_ty.kind() { - if self.can_coerce(rhs_ty, *lhs_inner_ty) { + let suggest_deref_binop = |err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>, + rhs_ty: Ty<'tcx>| { + if let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) { + // Can only assign if the type is sized, so if `DerefMut` yields a type that is + // unsized, do not suggest dereferencing it. + let lhs_deref_ty_is_sized = self + .infcx + .type_implements_trait( + self.tcx.lang_items().sized_trait().unwrap(), + lhs_deref_ty, + ty::List::empty(), + self.param_env, + ) + .may_apply(); + if lhs_deref_ty_is_sized && self.can_coerce(rhs_ty, lhs_deref_ty) { err.span_suggestion_verbose( lhs.span.shrink_to_lo(), - "consider dereferencing here to assign to the mutable \ - borrowed piece of memory", + "consider dereferencing here to assign to the mutably borrowed value", "*".to_string(), Applicability::MachineApplicable, ); } } + }; + + self.check_lhs_assignable(lhs, "E0070", span, |err| { + let rhs_ty = self.check_expr(&rhs); + suggest_deref_binop(err, rhs_ty); }); - let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty, Some(lhs)); + // This is (basically) inlined `check_expr_coercable_to_type`, but we want + // to suggest an additional fixup here in `suggest_deref_binop`. + let rhs_ty = self.check_expr_with_hint(&rhs, lhs_ty); + if let (_, Some(mut diag)) = + self.demand_coerce_diag(rhs, rhs_ty, lhs_ty, Some(lhs), AllowTwoPhase::No) + { + suggest_deref_binop(&mut diag, rhs_ty); + diag.emit(); + } self.require_type_is_sized(lhs_ty, lhs.span, traits::AssignmentLhsSized); diff --git a/compiler/rustc_typeck/src/check/op.rs b/compiler/rustc_typeck/src/check/op.rs index d250f38fffa3c..c99d9d8f9230d 100644 --- a/compiler/rustc_typeck/src/check/op.rs +++ b/compiler/rustc_typeck/src/check/op.rs @@ -42,9 +42,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; self.check_lhs_assignable(lhs, "E0067", op.span, |err| { - if let Ref(_, rty, hir::Mutability::Mut) = lhs_ty.kind() { + if let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) { if self - .lookup_op_method(*rty, Some(rhs_ty), Some(rhs), Op::Binary(op, IsAssign::Yes)) + .lookup_op_method( + lhs_deref_ty, + Some(rhs_ty), + Some(rhs), + Op::Binary(op, IsAssign::Yes), + ) .is_ok() { // Suppress this error, since we already emitted @@ -415,23 +420,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { (err, missing_trait, use_output) } }; - if let Ref(_, rty, mutability) = lhs_ty.kind() { - let is_copy = - self.infcx.type_is_copy_modulo_regions(self.param_env, *rty, lhs_expr.span); - // We should suggest `a + b` => `*a + b` if `a` is copy, and suggest - // `a += b` => `*a += b` if a is a mut ref. - // FIXME: This could be done any time lhs_ty is DerefMut into something that - // is compatible with rhs_ty, and not _just_ `&mut` (for IsAssign::Yes). - if ((is_assign == IsAssign::No && is_copy) - || (is_assign == IsAssign::Yes && *mutability == hir::Mutability::Mut)) - && self - .lookup_op_method( - *rty, - Some(rhs_ty), - Some(rhs_expr), - Op::Binary(op, is_assign), - ) - .is_ok() + + let mut suggest_deref_binop = |lhs_deref_ty: Ty<'tcx>| { + if self + .lookup_op_method( + lhs_deref_ty, + Some(rhs_ty), + Some(rhs_expr), + Op::Binary(op, is_assign), + ) + .is_ok() { if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) { let msg = &format!( @@ -441,7 +439,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { IsAssign::Yes => "=", IsAssign::No => "", }, - rty.peel_refs(), + lhs_deref_ty.peel_refs(), lstring, ); err.span_suggestion_verbose( @@ -452,6 +450,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); } } + }; + + // We should suggest `a + b` => `*a + b` if `a` is copy, and suggest + // `a += b` => `*a += b` if a is a mut ref. + if is_assign == IsAssign::Yes + && let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) { + suggest_deref_binop(lhs_deref_ty); + } else if is_assign == IsAssign::No + && let Ref(_, lhs_deref_ty, _) = lhs_ty.kind() { + if self.infcx.type_is_copy_modulo_regions(self.param_env, *lhs_deref_ty, lhs_expr.span) { + suggest_deref_binop(*lhs_deref_ty); + } } if let Some(missing_trait) = missing_trait { let mut visitor = TypeParamVisitor(vec![]); diff --git a/src/test/ui/suggestions/mut-ref-reassignment.stderr b/src/test/ui/suggestions/mut-ref-reassignment.stderr index 3bd98c7630780..b3cb6dd06142b 100644 --- a/src/test/ui/suggestions/mut-ref-reassignment.stderr +++ b/src/test/ui/suggestions/mut-ref-reassignment.stderr @@ -8,7 +8,7 @@ LL | opt = None; | = note: expected mutable reference `&mut Option` found enum `Option<_>` -help: consider dereferencing here to assign to the mutable borrowed piece of memory +help: consider dereferencing here to assign to the mutably borrowed value | LL | *opt = None; | + @@ -34,7 +34,7 @@ LL | opt = Some(String::new()) | = note: expected mutable reference `&mut Option` found enum `Option` -help: consider dereferencing here to assign to the mutable borrowed piece of memory +help: consider dereferencing here to assign to the mutably borrowed value | LL | *opt = Some(String::new()) | + diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.fixed b/src/test/ui/typeck/assign-non-lval-derefmut.fixed new file mode 100644 index 0000000000000..0c23199af2270 --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-derefmut.fixed @@ -0,0 +1,15 @@ +// run-rustfix + +fn main() { + let x = std::sync::Mutex::new(1usize); + *x.lock().unwrap() = 2; + //~^ ERROR invalid left-hand side of assignment + *x.lock().unwrap() += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` + + let mut y = x.lock().unwrap(); + *y = 2; + //~^ ERROR mismatched types + *y += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` +} diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.rs b/src/test/ui/typeck/assign-non-lval-derefmut.rs new file mode 100644 index 0000000000000..ec1882f5271b1 --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-derefmut.rs @@ -0,0 +1,15 @@ +// run-rustfix + +fn main() { + let x = std::sync::Mutex::new(1usize); + x.lock().unwrap() = 2; + //~^ ERROR invalid left-hand side of assignment + x.lock().unwrap() += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` + + let mut y = x.lock().unwrap(); + y = 2; + //~^ ERROR mismatched types + y += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` +} diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.stderr b/src/test/ui/typeck/assign-non-lval-derefmut.stderr new file mode 100644 index 0000000000000..a6fcdfe21f481 --- /dev/null +++ b/src/test/ui/typeck/assign-non-lval-derefmut.stderr @@ -0,0 +1,58 @@ +error[E0070]: invalid left-hand side of assignment + --> $DIR/assign-non-lval-derefmut.rs:5:23 + | +LL | x.lock().unwrap() = 2; + | ----------------- ^ + | | + | cannot assign to this expression + | +help: consider dereferencing here to assign to the mutably borrowed value + | +LL | *x.lock().unwrap() = 2; + | + + +error[E0368]: binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` + --> $DIR/assign-non-lval-derefmut.rs:7:5 + | +LL | x.lock().unwrap() += 1; + | -----------------^^^^^ + | | + | cannot use `+=` on type `MutexGuard<'_, usize>` + | +help: `+=` can be used on `usize`, you can dereference `x.lock().unwrap()` + | +LL | *x.lock().unwrap() += 1; + | + + +error[E0308]: mismatched types + --> $DIR/assign-non-lval-derefmut.rs:11:9 + | +LL | let mut y = x.lock().unwrap(); + | ----------------- expected due to this value +LL | y = 2; + | ^ expected struct `MutexGuard`, found integer + | + = note: expected struct `MutexGuard<'_, usize>` + found type `{integer}` +help: consider dereferencing here to assign to the mutably borrowed value + | +LL | *y = 2; + | + + +error[E0368]: binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>` + --> $DIR/assign-non-lval-derefmut.rs:13:5 + | +LL | y += 1; + | -^^^^^ + | | + | cannot use `+=` on type `MutexGuard<'_, usize>` + | +help: `+=` can be used on `usize`, you can dereference `y` + | +LL | *y += 1; + | + + +error: aborting due to 4 previous errors + +Some errors have detailed explanations: E0070, E0308, E0368. +For more information about an error, try `rustc --explain E0070`. diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed index c4dadfbdfcea4..10c7b9dbfb331 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed @@ -6,4 +6,10 @@ fn main() { //~^ ERROR invalid left-hand side of assignment *x.last_mut().unwrap() += 1; //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` + + let y = x.last_mut().unwrap(); + *y = 2; + //~^ ERROR mismatched types + *y += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` } diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.rs b/src/test/ui/typeck/assign-non-lval-mut-ref.rs index 39573ddb6d0e8..bceff0ef09d19 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.rs +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.rs @@ -6,4 +6,10 @@ fn main() { //~^ ERROR invalid left-hand side of assignment x.last_mut().unwrap() += 1; //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` + + let y = x.last_mut().unwrap(); + y = 2; + //~^ ERROR mismatched types + y += 1; + //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize` } diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr index b0ca089b7092e..be2e9fe95e871 100644 --- a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr +++ b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr @@ -6,7 +6,7 @@ LL | x.last_mut().unwrap() = 2; | | | cannot assign to this expression | -help: consider dereferencing here to assign to the mutable borrowed piece of memory +help: consider dereferencing here to assign to the mutably borrowed value | LL | *x.last_mut().unwrap() = 2; | + @@ -24,7 +24,33 @@ help: `+=` can be used on `usize`, you can dereference `x.last_mut().unwrap()` LL | *x.last_mut().unwrap() += 1; | + -error: aborting due to 2 previous errors +error[E0308]: mismatched types + --> $DIR/assign-non-lval-mut-ref.rs:11:9 + | +LL | let y = x.last_mut().unwrap(); + | --------------------- expected due to this value +LL | y = 2; + | ^ expected `&mut usize`, found integer + | +help: consider dereferencing here to assign to the mutably borrowed value + | +LL | *y = 2; + | + + +error[E0368]: binary assignment operation `+=` cannot be applied to type `&mut usize` + --> $DIR/assign-non-lval-mut-ref.rs:13:5 + | +LL | y += 1; + | -^^^^^ + | | + | cannot use `+=` on type `&mut usize` + | +help: `+=` can be used on `usize`, you can dereference `y` + | +LL | *y += 1; + | + + +error: aborting due to 4 previous errors -Some errors have detailed explanations: E0070, E0368. +Some errors have detailed explanations: E0070, E0308, E0368. For more information about an error, try `rustc --explain E0070`. diff --git a/src/test/ui/typeck/issue-93486.stderr b/src/test/ui/typeck/issue-93486.stderr index 95eb021965f4d..167edc8942aec 100644 --- a/src/test/ui/typeck/issue-93486.stderr +++ b/src/test/ui/typeck/issue-93486.stderr @@ -6,7 +6,7 @@ LL | vec![].last_mut().unwrap() = 3_u8; | | | cannot assign to this expression | -help: consider dereferencing here to assign to the mutable borrowed piece of memory +help: consider dereferencing here to assign to the mutably borrowed value | LL | *vec![].last_mut().unwrap() = 3_u8; | + From 0de7568e417ae001a45ae0a85b8bc2067a5f9c46 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 24 Apr 2022 21:07:46 -0700 Subject: [PATCH 11/11] Mention traits being upcasted, types being coerced --- compiler/rustc_typeck/src/check/coercion.rs | 19 ++++++++++++------- .../feature-gate-trait_upcasting.stderr | 3 ++- src/test/ui/issues/issue-11515.stderr | 3 ++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs index 57d3079935415..9790a42102837 100644 --- a/compiler/rustc_typeck/src/check/coercion.rs +++ b/compiler/rustc_typeck/src/check/coercion.rs @@ -615,7 +615,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { )]; let mut has_unsized_tuple_coercion = false; - let mut has_trait_upcasting_coercion = false; + let mut has_trait_upcasting_coercion = None; // Keep resolving `CoerceUnsized` and `Unsize` predicates to avoid // emitting a coercion in cases like `Foo<$1>` -> `Foo<$2>`, where @@ -635,7 +635,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { && data_a.principal_def_id() != data_b.principal_def_id() { debug!("coerce_unsized: found trait upcasting coercion"); - has_trait_upcasting_coercion = true; + has_trait_upcasting_coercion = Some((self_ty, unsize_ty)); } if let ty::Tuple(..) = unsize_ty.kind() { debug!("coerce_unsized: found unsized tuple coercion"); @@ -706,14 +706,19 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { .emit(); } - if has_trait_upcasting_coercion && !self.tcx().features().trait_upcasting { - feature_err( + if let Some((sub, sup)) = has_trait_upcasting_coercion + && !self.tcx().features().trait_upcasting + { + // Renders better when we erase regions, since they're not really the point here. + let (sub, sup) = self.tcx.erase_regions((sub, sup)); + let mut err = feature_err( &self.tcx.sess.parse_sess, sym::trait_upcasting, self.cause.span, - "trait upcasting coercion is experimental", - ) - .emit(); + &format!("cannot cast `{sub}` to `{sup}`, trait upcasting coercion is experimental"), + ); + err.note(&format!("required when coercing `{source}` into `{target}`")); + err.emit(); } Ok(coercion) diff --git a/src/test/ui/feature-gates/feature-gate-trait_upcasting.stderr b/src/test/ui/feature-gates/feature-gate-trait_upcasting.stderr index bc13a5d7d7b46..93afa78459d3d 100644 --- a/src/test/ui/feature-gates/feature-gate-trait_upcasting.stderr +++ b/src/test/ui/feature-gates/feature-gate-trait_upcasting.stderr @@ -1,4 +1,4 @@ -error[E0658]: trait upcasting coercion is experimental +error[E0658]: cannot cast `dyn Bar` to `dyn Foo`, trait upcasting coercion is experimental --> $DIR/feature-gate-trait_upcasting.rs:11:25 | LL | let foo: &dyn Foo = bar; @@ -6,6 +6,7 @@ LL | let foo: &dyn Foo = bar; | = note: see issue #65991 for more information = help: add `#![feature(trait_upcasting)]` to the crate attributes to enable + = note: required when coercing `&dyn Bar` into `&dyn Foo` error: aborting due to previous error diff --git a/src/test/ui/issues/issue-11515.stderr b/src/test/ui/issues/issue-11515.stderr index a70e7c416bc0f..17bfae2a4e46b 100644 --- a/src/test/ui/issues/issue-11515.stderr +++ b/src/test/ui/issues/issue-11515.stderr @@ -1,4 +1,4 @@ -error[E0658]: trait upcasting coercion is experimental +error[E0658]: cannot cast `dyn Fn()` to `dyn FnMut()`, trait upcasting coercion is experimental --> $DIR/issue-11515.rs:9:33 | LL | let test = box Test { func: closure }; @@ -6,6 +6,7 @@ LL | let test = box Test { func: closure }; | = note: see issue #65991 for more information = help: add `#![feature(trait_upcasting)]` to the crate attributes to enable + = note: required when coercing `Box<(dyn Fn() + 'static)>` into `Box<(dyn FnMut() + 'static)>` error: aborting due to previous error