Skip to content

Commit eee8a0f

Browse files
committed
Fix clippy
When mgca is enabled, const rhs's that are paths may have false negatives with the lints in non_copy_const.rs. But these should probably be using the trait solver anyway, and it only happens under mgca.
1 parent a990797 commit eee8a0f

14 files changed

+157
-75
lines changed

src/tools/clippy/clippy_lints/src/non_copy_const.rs

Lines changed: 44 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
// definitely contains interior mutability.
1919

2020
use clippy_config::Conf;
21-
use clippy_utils::consts::{ConstEvalCtxt, Constant};
21+
use clippy_utils::consts::{ConstEvalCtxt, Constant, const_item_rhs_to_expr};
2222
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
2323
use clippy_utils::is_in_const_context;
2424
use clippy_utils::macros::macro_backtrace;
@@ -28,7 +28,8 @@ use rustc_data_structures::fx::FxHashMap;
2828
use rustc_hir::def::{DefKind, Res};
2929
use rustc_hir::def_id::{DefId, DefIdSet};
3030
use rustc_hir::{
31-
Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, StructTailExpr, TraitItem, TraitItemKind, UnOp,
31+
ConstArgKind, ConstItemRhs, Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, StructTailExpr,
32+
TraitItem, TraitItemKind, UnOp,
3233
};
3334
use rustc_lint::{LateContext, LateLintPass, LintContext};
3435
use rustc_middle::mir::{ConstValue, UnevaluatedConst};
@@ -272,6 +273,7 @@ impl<'tcx> NonCopyConst<'tcx> {
272273

273274
/// Checks if a value of the given type is `Freeze`, or may be depending on the value.
274275
fn is_ty_freeze(&mut self, tcx: TyCtxt<'tcx>, typing_env: TypingEnv<'tcx>, ty: Ty<'tcx>) -> IsFreeze {
276+
// FIXME: this should probably be using the trait solver
275277
let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty);
276278
match self.freeze_tys.entry(ty) {
277279
Entry::Occupied(e) => *e.get(),
@@ -695,21 +697,22 @@ impl<'tcx> NonCopyConst<'tcx> {
695697

696698
impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
697699
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
698-
if let ItemKind::Const(ident, .., body_id) = item.kind
700+
if let ItemKind::Const(ident, .., ct_rhs) = item.kind
699701
&& !ident.is_special()
700702
&& let ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
701703
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) {
702704
IsFreeze::No => true,
703705
IsFreeze::Yes => false,
704706
IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) {
705707
Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => !is_freeze,
706-
_ => !self.is_init_expr_freeze(
708+
// FIXME: we just assume mgca rhs's are freeze
709+
_ => const_item_rhs_to_expr(cx.tcx, ct_rhs).map_or(false, |e| !self.is_init_expr_freeze(
707710
cx.tcx,
708711
cx.typing_env(),
709712
cx.tcx.typeck(item.owner_id),
710713
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
711-
cx.tcx.hir_body(body_id).value,
712-
),
714+
e
715+
)),
713716
},
714717
}
715718
&& !item.span.in_external_macro(cx.sess().source_map())
@@ -736,22 +739,25 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
736739
}
737740

738741
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
739-
if let TraitItemKind::Const(_, body_id_opt) = item.kind
742+
if let TraitItemKind::Const(_, ct_rhs_opt) = item.kind
740743
&& let ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
741744
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) {
742745
IsFreeze::No => true,
743-
IsFreeze::Maybe if let Some(body_id) = body_id_opt => {
746+
IsFreeze::Maybe if let Some(ct_rhs) = ct_rhs_opt => {
744747
match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) {
745748
Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => {
746749
!is_freeze
747750
},
748-
_ => !self.is_init_expr_freeze(
749-
cx.tcx,
750-
cx.typing_env(),
751-
cx.tcx.typeck(item.owner_id),
752-
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
753-
cx.tcx.hir_body(body_id).value,
754-
),
751+
// FIXME: we just assume mgca rhs's are freeze
752+
_ => const_item_rhs_to_expr(cx.tcx, ct_rhs).map_or(false, |e| {
753+
!self.is_init_expr_freeze(
754+
cx.tcx,
755+
cx.typing_env(),
756+
cx.tcx.typeck(item.owner_id),
757+
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
758+
e,
759+
)
760+
}),
755761
}
756762
},
757763
IsFreeze::Yes | IsFreeze::Maybe => false,
@@ -768,7 +774,7 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
768774
}
769775

770776
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {
771-
if let ImplItemKind::Const(_, body_id) = item.kind
777+
if let ImplItemKind::Const(_, ct_rhs) = item.kind
772778
&& let ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
773779
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) {
774780
IsFreeze::Yes => false,
@@ -799,13 +805,16 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
799805
// interior mutability.
800806
IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) {
801807
Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => !is_freeze,
802-
_ => !self.is_init_expr_freeze(
803-
cx.tcx,
804-
cx.typing_env(),
805-
cx.tcx.typeck(item.owner_id),
806-
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
807-
cx.tcx.hir_body(body_id).value,
808-
),
808+
// FIXME: we just assume mgca rhs's are freeze
809+
_ => const_item_rhs_to_expr(cx.tcx, ct_rhs).map_or(false, |e| {
810+
!self.is_init_expr_freeze(
811+
cx.tcx,
812+
cx.typing_env(),
813+
cx.tcx.typeck(item.owner_id),
814+
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
815+
e,
816+
)
817+
}),
809818
},
810819
}
811820
&& !item.span.in_external_macro(cx.sess().source_map())
@@ -913,20 +922,26 @@ fn get_const_hir_value<'tcx>(
913922
args: GenericArgsRef<'tcx>,
914923
) -> Option<(&'tcx TypeckResults<'tcx>, &'tcx Expr<'tcx>)> {
915924
let did = did.as_local()?;
916-
let (did, body_id) = match tcx.hir_node(tcx.local_def_id_to_hir_id(did)) {
917-
Node::Item(item) if let ItemKind::Const(.., body_id) = item.kind => (did, body_id),
918-
Node::ImplItem(item) if let ImplItemKind::Const(.., body_id) = item.kind => (did, body_id),
925+
let (did, ct_rhs) = match tcx.hir_node(tcx.local_def_id_to_hir_id(did)) {
926+
Node::Item(item) if let ItemKind::Const(.., ct_rhs) = item.kind => (did, ct_rhs),
927+
Node::ImplItem(item) if let ImplItemKind::Const(.., ct_rhs) = item.kind => (did, ct_rhs),
919928
Node::TraitItem(_)
920929
if let Ok(Some(inst)) = Instance::try_resolve(tcx, typing_env, did.into(), args)
921930
&& let Some(did) = inst.def_id().as_local() =>
922931
{
923932
match tcx.hir_node(tcx.local_def_id_to_hir_id(did)) {
924-
Node::ImplItem(item) if let ImplItemKind::Const(.., body_id) = item.kind => (did, body_id),
925-
Node::TraitItem(item) if let TraitItemKind::Const(.., Some(body_id)) = item.kind => (did, body_id),
933+
Node::ImplItem(item) if let ImplItemKind::Const(.., ct_rhs) = item.kind => (did, ct_rhs),
934+
Node::TraitItem(item) if let TraitItemKind::Const(.., Some(ct_rhs)) = item.kind => (did, ct_rhs),
926935
_ => return None,
927936
}
928937
},
929938
_ => return None,
930939
};
931-
Some((tcx.typeck(did), tcx.hir_body(body_id).value))
940+
match ct_rhs {
941+
ConstItemRhs::Body(body_id) => Some((tcx.typeck(did), tcx.hir_body(body_id).value)),
942+
ConstItemRhs::TypeConst(ct_arg) => match ct_arg.kind {
943+
ConstArgKind::Anon(anon_const) => Some((tcx.typeck(did), tcx.hir_body(anon_const.body).value)),
944+
_ => None,
945+
},
946+
}
932947
}

src/tools/clippy/clippy_lints/src/undocumented_unsafe_blocks.rs

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::ops::ControlFlow;
22
use std::sync::Arc;
33

44
use clippy_config::Conf;
5+
use clippy_utils::consts::const_item_rhs_to_expr;
56
use clippy_utils::diagnostics::span_lint_and_then;
67
use clippy_utils::is_lint_allowed;
78
use clippy_utils::source::walk_span_to_context;
@@ -184,7 +185,7 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks {
184185
}
185186
}
186187

187-
fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) {
188+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &hir::Item<'tcx>) {
188189
if item.span.in_external_macro(cx.tcx.sess.source_map()) {
189190
return;
190191
}
@@ -214,7 +215,7 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks {
214215
}
215216
}
216217

217-
fn check_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>, (span, help_span): (Span, Span), is_doc: bool) {
218+
fn check_has_safety_comment<'tcx>(cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, (span, help_span): (Span, Span), is_doc: bool) {
218219
match &item.kind {
219220
ItemKind::Impl(Impl {
220221
of_trait: Some(of_trait),
@@ -234,7 +235,29 @@ fn check_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>, (span, h
234235
},
235236
ItemKind::Impl(_) => {},
236237
// const and static items only need a safety comment if their body is an unsafe block, lint otherwise
237-
&ItemKind::Const(.., body) | &ItemKind::Static(.., body) => {
238+
&ItemKind::Const(.., ct_rhs) => {
239+
if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, ct_rhs.hir_id()) {
240+
let expr = const_item_rhs_to_expr(cx.tcx, ct_rhs);
241+
if let Some(expr) = expr && !matches!(
242+
expr.kind, hir::ExprKind::Block(block, _)
243+
if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)
244+
) {
245+
span_lint_and_then(
246+
cx,
247+
UNNECESSARY_SAFETY_COMMENT,
248+
span,
249+
format!(
250+
"{} has unnecessary safety comment",
251+
cx.tcx.def_descr(item.owner_id.to_def_id()),
252+
),
253+
|diag| {
254+
diag.span_help(help_span, "consider removing the safety comment");
255+
},
256+
);
257+
}
258+
}
259+
}
260+
&ItemKind::Static(.., body) => {
238261
if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, body.hir_id) {
239262
let body = cx.tcx.hir_body(body);
240263
if !matches!(

src/tools/clippy/clippy_lints/src/utils/author.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
320320
self.body(field!(anon_const.body));
321321
},
322322
ConstArgKind::Infer(..) => chain!(self, "let ConstArgKind::Infer(..) = {const_arg}.kind"),
323+
ConstArgKind::Error(..) => chain!(self, "let ConstArgKind::Error(..) = {const_arg}.kind"),
323324
}
324325
}
325326

src/tools/clippy/clippy_utils/src/ast_utils/mod.rs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ pub fn eq_item_kind(l: &ItemKind, r: &ItemKind) -> bool {
374374
&& eq_id(*li, *ri)
375375
&& eq_generics(lg, rg)
376376
&& eq_ty(lt, rt)
377-
&& both(lb.as_deref(), rb.as_deref(), |l, r| eq_anon_const(l, r))
377+
&& both(lb.as_ref(), rb.as_ref(), |l, r| eq_const_item_rhs(l, r))
378378
},
379379
(
380380
Fn(box ast::Fn {
@@ -628,7 +628,7 @@ pub fn eq_assoc_item_kind(l: &AssocItemKind, r: &AssocItemKind) -> bool {
628628
&& eq_id(*li, *ri)
629629
&& eq_generics(lg, rg)
630630
&& eq_ty(lt, rt)
631-
&& both(lb.as_deref(), rb.as_deref(), |l, r| eq_anon_const(l, r))
631+
&& both(lb.as_ref(), rb.as_ref(), |l, r| eq_const_item_rhs(l, r))
632632
},
633633
(
634634
Fn(box ast::Fn {
@@ -786,6 +786,15 @@ pub fn eq_anon_const(l: &AnonConst, r: &AnonConst) -> bool {
786786
eq_expr(&l.value, &r.value)
787787
}
788788

789+
pub fn eq_const_item_rhs(l: &ConstItemRhs, r: &ConstItemRhs) -> bool {
790+
use ConstItemRhs::*;
791+
match (l, r) {
792+
(TypeConst(l), TypeConst(r)) => eq_anon_const(l, r),
793+
(Body(l), Body(r)) => eq_expr(l, r),
794+
(TypeConst(..), Body(..)) | (Body(..), TypeConst(..)) => false,
795+
}
796+
}
797+
789798
pub fn eq_use_tree_kind(l: &UseTreeKind, r: &UseTreeKind) -> bool {
790799
use UseTreeKind::*;
791800
match (l, r) {

src/tools/clippy/clippy_utils/src/consts.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ use rustc_apfloat::Float;
1313
use rustc_apfloat::ieee::{Half, Quad};
1414
use rustc_ast::ast::{LitFloatType, LitKind};
1515
use rustc_hir::def::{DefKind, Res};
16-
use rustc_hir::{BinOpKind, Block, ConstBlock, Expr, ExprKind, HirId, PatExpr, PatExprKind, QPath, TyKind, UnOp};
16+
use rustc_hir::{
17+
BinOpKind, Block, ConstArgKind, ConstBlock, ConstItemRhs, Expr, ExprKind, HirId, PatExpr, PatExprKind, QPath,
18+
TyKind, UnOp,
19+
};
1720
use rustc_lexer::{FrontmatterAllowed, tokenize};
1821
use rustc_lint::LateContext;
1922
use rustc_middle::mir::ConstValue;
@@ -1130,3 +1133,14 @@ pub fn integer_const(cx: &LateContext<'_>, expr: &Expr<'_>, ctxt: SyntaxContext)
11301133
pub fn is_zero_integer_const(cx: &LateContext<'_>, expr: &Expr<'_>, ctxt: SyntaxContext) -> bool {
11311134
integer_const(cx, expr, ctxt) == Some(0)
11321135
}
1136+
1137+
pub fn const_item_rhs_to_expr<'tcx>(tcx: TyCtxt<'tcx>, ct_rhs: ConstItemRhs<'tcx>) -> Option<&'tcx Expr<'tcx>> {
1138+
match ct_rhs {
1139+
ConstItemRhs::Body(body_id) => Some(tcx.hir_body(body_id).value),
1140+
ConstItemRhs::TypeConst(const_arg) => match const_arg.kind {
1141+
ConstArgKind::Path(_) => None,
1142+
ConstArgKind::Anon(anon) => Some(tcx.hir_body(anon.body).value),
1143+
ConstArgKind::Error(..) | ConstArgKind::Infer(..) => None,
1144+
},
1145+
}
1146+
}

src/tools/clippy/clippy_utils/src/hir_utils.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,9 @@ impl HirEqInterExpr<'_, '_, '_> {
481481
(ConstArgKind::Path(..), ConstArgKind::Anon(..))
482482
| (ConstArgKind::Anon(..), ConstArgKind::Path(..))
483483
| (ConstArgKind::Infer(..), _)
484-
| (_, ConstArgKind::Infer(..)) => false,
484+
| (_, ConstArgKind::Infer(..))
485+
| (ConstArgKind::Error(..), _)
486+
| (_, ConstArgKind::Error(..)) => false,
485487
}
486488
}
487489

@@ -1330,7 +1332,7 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> {
13301332
match &const_arg.kind {
13311333
ConstArgKind::Path(path) => self.hash_qpath(path),
13321334
ConstArgKind::Anon(anon) => self.hash_body(anon.body),
1333-
ConstArgKind::Infer(..) => {},
1335+
ConstArgKind::Infer(..) | ConstArgKind::Error(..) => {},
13341336
}
13351337
}
13361338

src/tools/clippy/clippy_utils/src/msrvs.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -192,21 +192,21 @@ fn parse_attrs(sess: &Session, attrs: &[impl AttributeExt]) -> Option<RustcVersi
192192

193193
let msrv_attr = msrv_attrs.next()?;
194194

195-
if let Some(duplicate) = msrv_attrs.next_back() {
196-
sess.dcx()
197-
.struct_span_err(duplicate.span(), "`clippy::msrv` is defined multiple times")
198-
.with_span_note(msrv_attr.span(), "first definition found here")
199-
.emit();
200-
}
195+
if let Some(duplicate) = msrv_attrs.next_back() {
196+
sess.dcx()
197+
.struct_span_err(duplicate.span(), "`clippy::msrv` is defined multiple times")
198+
.with_span_note(msrv_attr.span(), "first definition found here")
199+
.emit();
200+
}
201201

202202
let Some(msrv) = msrv_attr.value_str() else {
203203
sess.dcx().span_err(msrv_attr.span(), "bad clippy attribute");
204204
return None;
205205
};
206206

207207
let Some(version) = parse_version(msrv) else {
208-
sess.dcx()
209-
.span_err(msrv_attr.span(), format!("`{msrv}` is not a valid Rust version"));
208+
sess.dcx()
209+
.span_err(msrv_attr.span(), format!("`{msrv}` is not a valid Rust version"));
210210
return None;
211211
};
212212

src/tools/clippy/tests/ui/needless_doc_main.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,23 @@
99
/// unimplemented!();
1010
/// }
1111
/// ```
12-
///
12+
///
1313
/// With an explicit return type it should lint too
1414
/// ```edition2015
1515
/// fn main() -> () {
1616
//~^ ERROR: needless `fn main` in doctest
1717
/// unimplemented!();
1818
/// }
1919
/// ```
20-
///
20+
///
2121
/// This should, too.
2222
/// ```rust
2323
/// fn main() {
2424
//~^ ERROR: needless `fn main` in doctest
2525
/// unimplemented!();
2626
/// }
2727
/// ```
28-
///
28+
///
2929
/// This one too.
3030
/// ```no_run
3131
/// // the fn is not always the first line

src/tools/clippy/tests/ui/trait_duplication_in_bounds.fixed

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![deny(clippy::trait_duplication_in_bounds)]
22
#![allow(unused)]
3-
#![feature(associated_const_equality, const_trait_impl)]
3+
#![feature(const_trait_impl)]
44

55
use std::any::Any;
66

@@ -194,12 +194,3 @@ where
194194
T: Iterator<Item: Clone> + Iterator<Item: Clone>,
195195
{
196196
}
197-
trait AssocConstTrait {
198-
const ASSOC: usize;
199-
}
200-
fn assoc_const_args<T>()
201-
where
202-
T: AssocConstTrait<ASSOC = 0>,
203-
//~^ trait_duplication_in_bounds
204-
{
205-
}

src/tools/clippy/tests/ui/trait_duplication_in_bounds.rs

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![deny(clippy::trait_duplication_in_bounds)]
22
#![allow(unused)]
3-
#![feature(associated_const_equality, const_trait_impl)]
3+
#![feature(const_trait_impl)]
44

55
use std::any::Any;
66

@@ -194,12 +194,3 @@ where
194194
T: Iterator<Item: Clone> + Iterator<Item: Clone>,
195195
{
196196
}
197-
trait AssocConstTrait {
198-
const ASSOC: usize;
199-
}
200-
fn assoc_const_args<T>()
201-
where
202-
T: AssocConstTrait<ASSOC = 0> + AssocConstTrait<ASSOC = 0>,
203-
//~^ trait_duplication_in_bounds
204-
{
205-
}

0 commit comments

Comments
 (0)