Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change leak check and suspicious auto trait lint warning messages #120716

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Change leak check and suspicious auto trait lint warning messages #120716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eee9d2a
Change leak check lint message to behavior is likely to change in the…
spastorino Feb 6, 2024
086463b
Remove suspicious auto trait lint
spastorino Feb 19, 2024
4803f17
Inline do_orphan_check_impl
spastorino Feb 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 12 additions & 176 deletions compiler/rustc_hir_analysis/src/coherence/orphan.rs
View file
Open in desktop
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
Open in desktop
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:/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
Open in desktop
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:/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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a "removed_lints" storage somewhere, the lint should be mentioned there, otherwise the error on allow(suspicious_auto_trait_impls) is suboptimal, it's done via LintStore::register_removed.

Warn,
"the rules governing auto traits have recently changed resulting in potential breakage",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseSemanticsChange,
reference: "issue #93367 <https:/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
Open in desktop
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
Loading