Skip to content

Commit

Permalink
Rollup merge of #120716 - spastorino:change-some-lint-msgs, r=lcnr
Browse files Browse the repository at this point in the history 
Change leak check and suspicious auto trait lint warning messages

The leak check lint message "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!" is misleading as some cases may not be phased out and could end being accepted. This is under discussion still.

The suspicious auto trait lint the change in behavior already happened, so the new message is probably more accurate.

r? `@lcnr`

Closes #93367
  • Loading branch information
@Noratrieb
Noratrieb authored Feb 20, 2024
2 parents 0b9f6ad + 4803f17 commit 5b8b435
Show file tree
Hide file tree
Showing 35 changed files with 104 additions and 563 deletions.
188 changes: 12 additions & 176 deletions compiler/rustc_hir_analysis/src/coherence/orphan.rs
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,12 @@
//! Orphan checker: every impl either implements a trait defined in this
//! crate or pertains to a type defined in this crate.
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{DelayDm, ErrorGuaranteed};
use rustc_errors::ErrorGuaranteed;
use rustc_hir as hir;
use rustc_middle::ty::util::CheckRegions;
use rustc_middle::ty::GenericArgs;
use rustc_middle::ty::{
self, AliasKind, ImplPolarity, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
TypeVisitor,
};
use rustc_session::lint;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_middle::ty::{self, AliasKind, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::def_id::LocalDefId;
use rustc_span::Span;
use rustc_trait_selection::traits;
use std::ops::ControlFlow;

use crate::errors;

Expand All @@ -26,30 +18,17 @@ pub(crate) fn orphan_check_impl(
let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap().instantiate_identity();
trait_ref.error_reported()?;

let ret = do_orphan_check_impl(tcx, trait_ref, impl_def_id);
if tcx.trait_is_auto(trait_ref.def_id) {
lint_auto_trait_impl(tcx, trait_ref, impl_def_id);
}

ret
}

fn do_orphan_check_impl<'tcx>(
tcx: TyCtxt<'tcx>,
trait_ref: ty::TraitRef<'tcx>,
def_id: LocalDefId,
) -> Result<(), ErrorGuaranteed> {
let trait_def_id = trait_ref.def_id;

match traits::orphan_check(tcx, def_id.to_def_id()) {
match traits::orphan_check(tcx, impl_def_id.to_def_id()) {
Ok(()) => {}
Err(err) => {
let item = tcx.hir().expect_item(def_id);
let item = tcx.hir().expect_item(impl_def_id);
let hir::ItemKind::Impl(impl_) = item.kind else {
bug!("{:?} is not an impl: {:?}", def_id, item);
bug!("{:?} is not an impl: {:?}", impl_def_id, item);
};
let tr = impl_.of_trait.as_ref().unwrap();
let sp = tcx.def_span(def_id);
let sp = tcx.def_span(impl_def_id);

emit_orphan_check_error(
tcx,
Expand Down Expand Up @@ -193,7 +172,7 @@ fn do_orphan_check_impl<'tcx>(
// impl<T> AutoTrait for T {}
// impl<T: ?Sized> AutoTrait for T {}
ty::Param(..) => (
if self_ty.is_sized(tcx, tcx.param_env(def_id)) {
if self_ty.is_sized(tcx, tcx.param_env(impl_def_id)) {
LocalImpl::Allow
} else {
LocalImpl::Disallow { problematic_kind: "generic type" }
Expand Down Expand Up @@ -250,7 +229,7 @@ fn do_orphan_check_impl<'tcx>(
| ty::Bound(..)
| ty::Placeholder(..)
| ty::Infer(..) => {
let sp = tcx.def_span(def_id);
let sp = tcx.def_span(impl_def_id);
span_bug!(sp, "weird self type for autotrait impl")
}

Expand All @@ -262,7 +241,7 @@ fn do_orphan_check_impl<'tcx>(
LocalImpl::Allow => {}
LocalImpl::Disallow { problematic_kind } => {
return Err(tcx.dcx().emit_err(errors::TraitsWithDefaultImpl {
span: tcx.def_span(def_id),
span: tcx.def_span(impl_def_id),
traits: tcx.def_path_str(trait_def_id),
problematic_kind,
self_ty,
Expand All @@ -274,13 +253,13 @@ fn do_orphan_check_impl<'tcx>(
NonlocalImpl::Allow => {}
NonlocalImpl::DisallowBecauseNonlocal => {
return Err(tcx.dcx().emit_err(errors::CrossCrateTraitsDefined {
span: tcx.def_span(def_id),
span: tcx.def_span(impl_def_id),
traits: tcx.def_path_str(trait_def_id),
}));
}
NonlocalImpl::DisallowOther => {
return Err(tcx.dcx().emit_err(errors::CrossCrateTraits {
span: tcx.def_span(def_id),
span: tcx.def_span(impl_def_id),
traits: tcx.def_path_str(trait_def_id),
self_ty,
}));
Expand Down Expand Up @@ -445,146 +424,3 @@ fn emit_orphan_check_error<'tcx>(
}
})
}

/// Lint impls of auto traits if they are likely to have
/// unsound or surprising effects on auto impls.
fn lint_auto_trait_impl<'tcx>(
tcx: TyCtxt<'tcx>,
trait_ref: ty::TraitRef<'tcx>,
impl_def_id: LocalDefId,
) {
if trait_ref.args.len() != 1 {
tcx.dcx().span_delayed_bug(
tcx.def_span(impl_def_id),
"auto traits cannot have generic parameters",
);
return;
}
let self_ty = trait_ref.self_ty();
let (self_type_did, args) = match self_ty.kind() {
ty::Adt(def, args) => (def.did(), args),
_ => {
// FIXME: should also lint for stuff like `&i32` but
// considering that auto traits are unstable, that
// isn't too important for now as this only affects
// crates using `nightly`, and std.
return;
}
};

// Impls which completely cover a given root type are fine as they
// disable auto impls entirely. So only lint if the args
// are not a permutation of the identity args.
let Err(arg) = tcx.uses_unique_generic_params(args, CheckRegions::No) else {
// ok
return;
};

// Ideally:
//
// - compute the requirements for the auto impl candidate
// - check whether these are implied by the non covering impls
// - if not, emit the lint
//
// What we do here is a bit simpler:
//
// - badly check if an auto impl candidate definitely does not apply
// for the given simplified type
// - if so, do not lint
if fast_reject_auto_impl(tcx, trait_ref.def_id, self_ty) {
// ok
return;
}

tcx.node_span_lint(
lint::builtin::SUSPICIOUS_AUTO_TRAIT_IMPLS,
tcx.local_def_id_to_hir_id(impl_def_id),
tcx.def_span(impl_def_id),
DelayDm(|| {
format!(
"cross-crate traits with a default impl, like `{}`, \
should not be specialized",
tcx.def_path_str(trait_ref.def_id),
)
}),
|lint| {
let item_span = tcx.def_span(self_type_did);
let self_descr = tcx.def_descr(self_type_did);
match arg {
ty::util::NotUniqueParam::DuplicateParam(arg) => {
lint.note(format!("`{arg}` is mentioned multiple times"));
}
ty::util::NotUniqueParam::NotParam(arg) => {
lint.note(format!("`{arg}` is not a generic parameter"));
}
}
lint.span_note(
item_span,
format!(
"try using the same sequence of generic parameters as the {self_descr} definition",
),
);
},
);
}

fn fast_reject_auto_impl<'tcx>(tcx: TyCtxt<'tcx>, trait_def_id: DefId, self_ty: Ty<'tcx>) -> bool {
struct DisableAutoTraitVisitor<'tcx> {
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
self_ty_root: Ty<'tcx>,
seen: FxHashSet<DefId>,
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for DisableAutoTraitVisitor<'tcx> {
type BreakTy = ();
fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
let tcx = self.tcx;
if ty != self.self_ty_root {
for impl_def_id in tcx.non_blanket_impls_for_ty(self.trait_def_id, ty) {
match tcx.impl_polarity(impl_def_id) {
ImplPolarity::Negative => return ControlFlow::Break(()),
ImplPolarity::Reservation => {}
// FIXME(@lcnr): That's probably not good enough, idk
//
// We might just want to take the rustdoc code and somehow avoid
// explicit impls for `Self`.
ImplPolarity::Positive => return ControlFlow::Continue(()),
}
}
}

match ty.kind() {
ty::Adt(def, args) if def.is_phantom_data() => args.visit_with(self),
ty::Adt(def, args) => {
// @lcnr: This is the only place where cycles can happen. We avoid this
// by only visiting each `DefId` once.
//
// This will be is incorrect in subtle cases, but I don't care :)
if self.seen.insert(def.did()) {
for ty in def.all_fields().map(|field| field.ty(tcx, args)) {
ty.visit_with(self)?;
}
}

ControlFlow::Continue(())
}
_ => ty.super_visit_with(self),
}
}
}

let self_ty_root = match self_ty.kind() {
ty::Adt(def, _) => Ty::new_adt(tcx, *def, GenericArgs::identity_for_item(tcx, def.did())),
_ => unimplemented!("unexpected self ty {:?}", self_ty),
};

self_ty_root
.visit_with(&mut DisableAutoTraitVisitor {
tcx,
self_ty_root,
trait_def_id,
seen: FxHashSet::default(),
})
.is_break()
}
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,11 @@ fn register_builtins(store: &mut LintStore) {
"no longer needed, see RFC #3535 \
<https://rust-lang.github.io/rfcs/3535-constants-in-patterns.html> for more information",
);
store.register_removed(
"suspicious_auto_trait_impls",
"no longer needed, see #93367 \
<https://github.com/rust-lang/rust/issues/93367> for more information",
);
}

fn register_internals(store: &mut LintStore) {
Expand Down
37 changes: 1 addition & 36 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ declare_lint_pass! {
SOFT_UNSTABLE,
STABLE_FEATURES,
STATIC_MUT_REFS,
SUSPICIOUS_AUTO_TRAIT_IMPLS,
TEST_UNSTABLE_LINT,
TEXT_DIRECTION_CODEPOINT_IN_COMMENT,
TRIVIAL_CASTS,
Expand Down Expand Up @@ -1503,7 +1502,7 @@ declare_lint! {
Warn,
"distinct impls distinguished only by the leak-check code",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps,
reason: FutureIncompatibilityReason::Custom("the behavior may change in a future release"),
reference: "issue #56105 <https://github.com/rust-lang/rust/issues/56105>",
};
}
Expand Down Expand Up @@ -4032,40 +4031,6 @@ declare_lint! {
"duplicated attribute"
}

declare_lint! {
/// The `suspicious_auto_trait_impls` lint checks for potentially incorrect
/// implementations of auto traits.
///
/// ### Example
///
/// ```rust
/// struct Foo<T>(T);
///
/// unsafe impl<T> Send for Foo<*const T> {}
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// A type can implement auto traits, e.g. `Send`, `Sync` and `Unpin`,
/// in two different ways: either by writing an explicit impl or if
/// all fields of the type implement that auto trait.
///
/// The compiler disables the automatic implementation if an explicit one
/// exists for given type constructor. The exact rules governing this
/// were previously unsound, quite subtle, and have been recently modified.
/// This change caused the automatic implementation to be disabled in more
/// cases, potentially breaking some code.
pub SUSPICIOUS_AUTO_TRAIT_IMPLS,
Warn,
"the rules governing auto traits have recently changed resulting in potential breakage",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseSemanticsChange,
reference: "issue #93367 <https://github.com/rust-lang/rust/issues/93367>",
};
}

declare_lint! {
/// The `deprecated_where_clause_location` lint detects when a where clause in front of the equals
/// in an associated type.
Expand Down
1 change: 0 additions & 1 deletion src/tools/clippy/tests/ui/non_send_fields_in_send_ty.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![warn(clippy::non_send_fields_in_send_ty)]
#![allow(suspicious_auto_trait_impls)]
#![feature(extern_types)]

use std::cell::UnsafeCell;
Expand Down
Loading

0 comments on commit 5b8b435

Please sign in to comment.