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.

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

Restore const PartialEq #118661

Merged
merged 7 commits into from
Dec 11, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
16 changes: 7 additions & 9 deletions compiler/rustc_middle/src/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,8 @@ impl<'hir> Map<'hir> {
/// Returns the `BodyOwnerKind` of this `LocalDefId`.
///
/// Panics if `LocalDefId` does not have an associated body.
pub fn body_owner_kind(self, def_id: LocalDefId) -> BodyOwnerKind {
pub fn body_owner_kind(self, def_id: impl Into<DefId>) -> BodyOwnerKind {
let def_id = def_id.into();
match self.tcx.def_kind(def_id) {
DefKind::Const | DefKind::AssocConst | DefKind::AnonConst => {
BodyOwnerKind::Const { inline: false }
Expand All @@ -356,20 +357,17 @@ impl<'hir> Map<'hir> {
/// This should only be used for determining the context of a body, a return
/// value of `Some` does not always suggest that the owner of the body is `const`,
/// just that it has to be checked as if it were.
pub fn body_const_context(self, def_id: LocalDefId) -> Option<ConstContext> {
pub fn body_const_context(self, def_id: impl Into<DefId>) -> Option<ConstContext> {
let def_id = def_id.into();
let ccx = match self.body_owner_kind(def_id) {
BodyOwnerKind::Const { inline } => ConstContext::Const { inline },
BodyOwnerKind::Static(mt) => ConstContext::Static(mt),

BodyOwnerKind::Fn if self.tcx.is_constructor(def_id.to_def_id()) => return None,
BodyOwnerKind::Fn | BodyOwnerKind::Closure
if self.tcx.is_const_fn_raw(def_id.to_def_id()) =>
{
ConstContext::ConstFn
}
BodyOwnerKind::Fn if self.tcx.is_const_default_method(def_id.to_def_id()) => {
BodyOwnerKind::Fn if self.tcx.is_constructor(def_id) => return None,
BodyOwnerKind::Fn | BodyOwnerKind::Closure if self.tcx.is_const_fn_raw(def_id) => {
ConstContext::ConstFn
}
BodyOwnerKind::Fn if self.tcx.is_const_default_method(def_id) => ConstContext::ConstFn,
BodyOwnerKind::Fn | BodyOwnerKind::Closure => return None,
};

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,8 @@ impl<'tcx> TyCtxt<'tcx> {
|| self.extern_crate(key.as_def_id()).is_some_and(|e| e.is_direct())
}

pub fn expected_host_effect_param_for_body(self, def_id: LocalDefId) -> ty::Const<'tcx> {
pub fn expected_host_effect_param_for_body(self, def_id: impl Into<DefId>) -> ty::Const<'tcx> {
let def_id = def_id.into();
// FIXME(effects): This is suspicious and should probably not be done,
// especially now that we enforce host effects and then properly handle
// effect vars during fallback.
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_lints/src/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,12 +450,12 @@ fn check_partial_eq_without_eq<'tcx>(cx: &LateContext<'tcx>, span: Span, trait_r
&& let Some(def_id) = trait_ref.trait_def_id()
&& cx.tcx.is_diagnostic_item(sym::PartialEq, def_id)
&& let param_env = param_env_for_derived_eq(cx.tcx, adt.did(), eq_trait_def_id)
&& !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[])
&& !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, adt.did(),&[])
// If all of our fields implement `Eq`, we can implement `Eq` too
&& adt
.all_fields()
.map(|f| f.ty(cx.tcx, args))
.all(|ty| implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[]))
.all(|ty| implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, adt.did(), &[]))
{
span_lint_and_sugg(
cx,
Expand Down
4 changes: 4 additions & 0 deletions src/tools/clippy/clippy_lints/src/implied_bounds_in_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,10 @@ fn is_same_generics<'tcx>(
.enumerate()
.skip(1) // skip `Self` implicit arg
.all(|(arg_index, arg)| {
if [implied_by_generics.host_effect_index, implied_generics.host_effect_index].contains(&Some(arg_index)) {
// skip host effect params in determining whether generics are same
return true;
}
if let Some(ty) = arg.as_type() {
if let &ty::Param(ty::ParamTy { index, .. }) = ty.kind()
// `index == 0` means that it's referring to `Self`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ fn is_ref_iterable<'tcx>(
.liberate_late_bound_regions(fn_id, cx.tcx.fn_sig(fn_id).skip_binder())
&& let &[req_self_ty, req_res_ty] = &**sig.inputs_and_output
&& let param_env = cx.tcx.param_env(fn_id)
&& implements_trait_with_env(cx.tcx, param_env, req_self_ty, trait_id, &[])
&& implements_trait_with_env(cx.tcx, param_env, req_self_ty, trait_id, fn_id, &[])
&& let Some(into_iter_ty) =
make_normalized_projection_with_regions(cx.tcx, param_env, trait_id, sym!(IntoIter), [req_self_ty])
&& let req_res_ty = normalize_with_regions(cx.tcx, param_env, req_res_ty)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue {
cx.param_env,
ty,
t,
None,
[Option::<ty::GenericArg<'tcx>>::None],
)
})
Expand Down
34 changes: 23 additions & 11 deletions src/tools/clippy/clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ pub fn implements_trait<'tcx>(
trait_id: DefId,
args: &[GenericArg<'tcx>],
) -> bool {
implements_trait_with_env_from_iter(cx.tcx, cx.param_env, ty, trait_id, args.iter().map(|&x| Some(x)))
let callee_id = cx.enclosing_body.map(|body| cx.tcx.hir().body_owner(body).owner.to_def_id());
implements_trait_with_env_from_iter(cx.tcx, cx.param_env, ty, trait_id, callee_id, args.iter().map(|&x| Some(x)))
}

/// Same as `implements_trait` but allows using a `ParamEnv` different from the lint context.
Expand All @@ -223,9 +224,10 @@ pub fn implements_trait_with_env<'tcx>(
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
callee_id: DefId,
Copy link
Contributor

@smoelius smoelius Jan 11, 2024

Choose a reason for hiding this comment

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

@fee1-dead (cc: @compiler-errors) I am having difficulty understanding what this additional required callee_id parameter is/does.

A comment elsewhere in this PR suggests it is an "effect arg". Moreover, I found this doc comment for with_opt_host_effect_param:

/// Constructs generic args for an item, optionally appending a const effect param type

But I can't find any documentation (e.g., in the Rustc Dev Guide) on what an "effect arg/param" is, or why/when it is needed.

Could you please point me to something that could help me better understand? Thank you.

This comment was marked as off-topic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've hidden the last comment since it could be wrong. I will take a look maybe tomorrow.

args: &[GenericArg<'tcx>],
) -> bool {
implements_trait_with_env_from_iter(tcx, param_env, ty, trait_id, args.iter().map(|&x| Some(x)))
implements_trait_with_env_from_iter(tcx, param_env, ty, trait_id, Some(callee_id), args.iter().map(|&x| Some(x)))
}

/// Same as `implements_trait_from_env` but takes the arguments as an iterator.
Expand All @@ -234,6 +236,7 @@ pub fn implements_trait_with_env_from_iter<'tcx>(
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
callee_id: Option<DefId>,
args: impl IntoIterator<Item = impl Into<Option<GenericArg<'tcx>>>>,
) -> bool {
// Clippy shouldn't have infer types
Expand All @@ -245,20 +248,29 @@ pub fn implements_trait_with_env_from_iter<'tcx>(
}

let infcx = tcx.infer_ctxt().build();
let args = args.into_iter().map(|arg| {
arg.into().unwrap_or_else(|| {
let orig = TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span: DUMMY_SP,
};
infcx.next_ty_var(orig).into()
})
}).collect::<Vec<_>>();

// If an effect arg was not specified, we need to specify it.
let effect_arg = if tcx.generics_of(trait_id).host_effect_index.is_some_and(|x| args.get(x - 1).is_none()) {
Some(GenericArg::from(callee_id.map(|def_id| tcx.expected_host_effect_param_for_body(def_id)).unwrap_or(tcx.consts.true_)))
} else {
None
};
Comment on lines +251 to +266
Copy link
Member

@compiler-errors compiler-errors Dec 10, 2023

Choose a reason for hiding this comment

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

I think this is simplified by with_opt_host_effect_param

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that works. There are times where the caller of this function has already specified the host param themselves (they're modifying an existing GenericArg list) and there's args.get(x-1) (since args is without the Self type) to check for that.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok.


let trait_ref = TraitRef::new(
tcx,
trait_id,
Some(GenericArg::from(ty))
.into_iter()
.chain(args.into_iter().map(|arg| {
arg.into().unwrap_or_else(|| {
let orig = TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span: DUMMY_SP,
};
infcx.next_ty_var(orig).into()
})
})),
.chain(args).chain(effect_arg),
);

debug_assert_matches!(
Expand Down
Loading