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

Warn about dead tuple struct fields #95977

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Warn about dead tuple struct fields #95977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions compiler/rustc_apfloat/src/ppc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub type DoubleDouble = DoubleFloat<ieee::Double>;
// FIXME: Implement all operations in DoubleDouble, and delete these
// semantics.
// FIXME(eddyb) This shouldn't need to be `pub`, it's only used in bounds.
pub struct FallbackS<F>(F);
pub struct FallbackS<F>(#[allow(unused)] F);
type Fallback<F> = ieee::IeeeFloat<FallbackS<F>>;
impl<F: Float> ieee::Semantics for FallbackS<F> {
// Forbid any conversion to/from bits.
Expand All @@ -45,7 +45,7 @@ impl<F: Float> ieee::Semantics for FallbackS<F> {
// truncate the mantissa. The result of that second conversion
// may be inexact, but should never underflow.
// FIXME(eddyb) This shouldn't need to be `pub`, it's only used in bounds.
pub struct FallbackExtendedS<F>(F);
pub struct FallbackExtendedS<F>(#[allow(unused)] F);
type FallbackExtended<F> = ieee::IeeeFloat<FallbackExtendedS<F>>;
impl<F: Float> ieee::Semantics for FallbackExtendedS<F> {
// Forbid any conversion to/from bits.
Expand Down
27 changes: 27 additions & 0 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 @@ -630,6 +630,32 @@ declare_lint! {
"detects attributes that were not used by the compiler"
}

declare_lint! {
/// The `unused_tuple_struct_fields` lint detects fields of tuple structs
/// that are never read.
///
/// ### Example
///
/// ```
/// #[warn(unused_tuple_struct_fields)]
/// struct S(i32, i32, i32);
/// let s = S(1, 2, 3);
/// let _ = (s.0, s.2);
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Tuple struct fields that are never read anywhere may indicate a
/// mistake or unfinished code. To silence this warning, consider
/// removing the unused field(s) or, to preserve the numbering of the
/// remaining fields, change the unused field(s) to have unit type.
pub UNUSED_TUPLE_STRUCT_FIELDS,
Allow,
"detects tuple struct fields that are never read"
}

declare_lint! {
/// The `unreachable_code` lint detects unreachable code paths.
///
Expand Down Expand Up @@ -3281,6 +3307,7 @@ declare_lint_pass! {
UNSUPPORTED_CALLING_CONVENTIONS,
BREAK_WITH_LABEL_AND_LOOP,
UNUSED_ATTRIBUTES,
UNUSED_TUPLE_STRUCT_FIELDS,
NON_EXHAUSTIVE_OMITTED_PATTERNS,
TEXT_DIRECTION_CODEPOINT_IN_COMMENT,
DEREF_INTO_DYN_SUPERTRAIT,
Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_passes/src/check_attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub(crate) fn target_from_impl_item<'tcx>(
#[derive(Clone, Copy)]
enum ItemLike<'tcx> {
Item(&'tcx Item<'tcx>),
ForeignItem(&'tcx ForeignItem<'tcx>),
ForeignItem,
}

struct CheckAttrVisitor<'tcx> {
Expand Down Expand Up @@ -1995,12 +1995,7 @@ impl<'tcx> Visitor<'tcx> for CheckAttrVisitor<'tcx> {

fn visit_foreign_item(&mut self, f_item: &'tcx ForeignItem<'tcx>) {
let target = Target::from_foreign_item(f_item);
self.check_attributes(
f_item.hir_id(),
f_item.span,
target,
Some(ItemLike::ForeignItem(f_item)),
);
self.check_attributes(f_item.hir_id(), f_item.span, target, Some(ItemLike::ForeignItem));
intravisit::walk_foreign_item(self, f_item)
}

Expand Down
129 changes: 111 additions & 18 deletions compiler/rustc_passes/src/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use itertools::Itertools;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{pluralize, MultiSpan};
use rustc_errors::{pluralize, Applicability, MultiSpan};
use rustc_hir as hir;
use rustc_hir::def::{CtorOf, DefKind, Res};
use rustc_hir::def_id::{DefId, LocalDefId};
Expand Down Expand Up @@ -42,6 +42,7 @@ struct MarkSymbolVisitor<'tcx> {
maybe_typeck_results: Option<&'tcx ty::TypeckResults<'tcx>>,
live_symbols: FxHashSet<LocalDefId>,
repr_has_repr_c: bool,
repr_has_repr_simd: bool,
in_pat: bool,
ignore_variant_stack: Vec<DefId>,
// maps from tuple struct constructors to tuple struct items
Expand Down Expand Up @@ -220,6 +221,32 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}
}

fn handle_tuple_field_pattern_match(
&mut self,
lhs: &hir::Pat<'_>,
res: Res,
pats: &[hir::Pat<'_>],
dotdot: Option<usize>,
) {
let variant = match self.typeck_results().node_type(lhs.hir_id).kind() {
ty::Adt(adt, _) => adt.variant_of_res(res),
_ => span_bug!(lhs.span, "non-ADT in tuple struct pattern"),
};
let first_n = pats.iter().enumerate().take(dotdot.unwrap_or(pats.len()));
let missing = variant.fields.len() - pats.len();
let last_n = pats
.iter()
.enumerate()
.skip(dotdot.unwrap_or(pats.len()))
.map(|(idx, pat)| (idx + missing, pat));
for (idx, pat) in first_n.chain(last_n) {
if let PatKind::Wild = pat.kind {
continue;
}
self.insert_def_id(variant.fields[idx].did);
}
}

fn mark_live_symbols(&mut self) {
let mut scanned = FxHashSet::default();
while let Some(id) = self.worklist.pop() {
Expand Down Expand Up @@ -274,12 +301,15 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}

let had_repr_c = self.repr_has_repr_c;
let had_repr_simd = self.repr_has_repr_simd;
self.repr_has_repr_c = false;
self.repr_has_repr_simd = false;
match node {
Node::Item(item) => match item.kind {
hir::ItemKind::Struct(..) | hir::ItemKind::Union(..) => {
let def = self.tcx.adt_def(item.def_id);
self.repr_has_repr_c = def.repr().c();
self.repr_has_repr_simd = def.repr().simd();

intravisit::walk_item(self, &item)
}
Expand Down Expand Up @@ -315,6 +345,7 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}
_ => {}
}
self.repr_has_repr_simd = had_repr_simd;
self.repr_has_repr_c = had_repr_c;
}

Expand Down Expand Up @@ -347,9 +378,10 @@ impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> {
) {
let tcx = self.tcx;
let has_repr_c = self.repr_has_repr_c;
let has_repr_simd = self.repr_has_repr_simd;
let live_fields = def.fields().iter().filter_map(|f| {
let def_id = tcx.hir().local_def_id(f.hir_id);
if has_repr_c {
if has_repr_c || (f.is_positional() && has_repr_simd) {
return Some(def_id);
}
if !tcx.visibility(f.hir_id.owner).is_public() {
Expand Down Expand Up @@ -408,6 +440,10 @@ impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> {
let res = self.typeck_results().qpath_res(qpath, pat.hir_id);
self.handle_res(res);
}
PatKind::TupleStruct(ref qpath, ref fields, dotdot) => {
let res = self.typeck_results().qpath_res(qpath, pat.hir_id);
self.handle_tuple_field_pattern_match(pat, res, fields, dotdot);
}
_ => (),
}

Expand Down Expand Up @@ -440,7 +476,11 @@ impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> {
}
}

fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
fn has_allow_dead_code_or_lang_attr_helper(
tcx: TyCtxt<'_>,
id: hir::HirId,
lint: &'static lint::Lint,
) -> bool {
let attrs = tcx.hir().attrs(id);
if tcx.sess.contains_name(attrs, sym::lang) {
return true;
Expand Down Expand Up @@ -470,7 +510,11 @@ fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
}
}

tcx.lint_level_at_node(lint::builtin::DEAD_CODE, id).0 == lint::Allow
tcx.lint_level_at_node(lint, id).0 == lint::Allow
}

fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
has_allow_dead_code_or_lang_attr_helper(tcx, id, lint::builtin::DEAD_CODE)
}

// These check_* functions seeds items that
Expand Down Expand Up @@ -623,6 +667,7 @@ fn live_symbols_and_ignored_derived_traits<'tcx>(
maybe_typeck_results: None,
live_symbols: Default::default(),
repr_has_repr_c: false,
repr_has_repr_simd: false,
in_pat: false,
ignore_variant_stack: vec![],
struct_constructors,
Expand All @@ -644,32 +689,46 @@ struct DeadVisitor<'tcx> {
ignored_derived_traits: &'tcx FxHashMap<LocalDefId, Vec<(DefId, DefId)>>,
}

enum ShouldWarnAboutField {
Yes(bool), // positional?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Yes(bool), // positional?
Named,
Positional,

This will probably be clearer.

No,
}
Comment on lines +692 to +695
Copy link
Contributor

Choose a reason for hiding this comment

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

I would normally represent this as

enum ShouldWarnAboutField {
    Positional,
    Named,
    No,
}

but I don't care strongly either way.


impl<'tcx> DeadVisitor<'tcx> {
fn should_warn_about_field(&mut self, field: &ty::FieldDef) -> bool {
fn should_warn_about_field(&mut self, field: &ty::FieldDef) -> ShouldWarnAboutField {
if self.live_symbols.contains(&field.did.expect_local()) {
return false;
return ShouldWarnAboutField::No;
}
let field_type = self.tcx.type_of(field.did);
if field_type.is_phantom_data() {
return ShouldWarnAboutField::No;
}
let is_positional = field.name.as_str().starts_with(|c: char| c.is_ascii_digit());
if is_positional {
return false;
if is_positional
&& self
.tcx
.layout_of(self.tcx.param_env(field.did).and(field_type))
.map_or(true, |layout| layout.is_zst())
{
return ShouldWarnAboutField::No;
}
let field_type = self.tcx.type_of(field.did);
!field_type.is_phantom_data()
ShouldWarnAboutField::Yes(is_positional)
}

fn warn_multiple_dead_codes(
&self,
dead_codes: &[LocalDefId],
participle: &str,
parent_item: Option<LocalDefId>,
is_positional: bool,
) {
if let Some(&first_id) = dead_codes.first() {
let tcx = self.tcx;
let names: Vec<_> = dead_codes
.iter()
.map(|&def_id| tcx.item_name(def_id.to_def_id()).to_string())
.collect();
let spans = dead_codes
let spans: Vec<_> = dead_codes
.iter()
.map(|&def_id| match tcx.def_ident_span(def_id) {
Some(s) => s.with_ctxt(tcx.def_span(def_id).ctxt()),
Expand All @@ -678,9 +737,13 @@ impl<'tcx> DeadVisitor<'tcx> {
.collect();

tcx.struct_span_lint_hir(
lint::builtin::DEAD_CODE,
if is_positional {
lint::builtin::UNUSED_TUPLE_STRUCT_FIELDS
} else {
lint::builtin::DEAD_CODE
},
tcx.hir().local_def_id_to_hir_id(first_id),
MultiSpan::from_spans(spans),
MultiSpan::from_spans(spans.clone()),
|lint| {
let descr = tcx.def_kind(first_id).descr(first_id.to_def_id());
let span_len = dead_codes.len();
Expand All @@ -702,6 +765,21 @@ impl<'tcx> DeadVisitor<'tcx> {
are = pluralize!("is", span_len),
));

if is_positional {
err.multipart_suggestion(
Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion seems a bit convoluted, and may be more confusing that helping. Since we don't give a suggestion in the non-positional case, do we need a suggestion?

&format!(
"consider changing the field{s} to be of unit type to \
suppress this warning while preserving the field \
numbering, or remove the field{s}",
s = pluralize!(span_len)
),
spans.iter().map(|sp| (*sp, "()".to_string())).collect(),
// "HasPlaceholders" because applying this fix by itself isn't
// enough: All constructor calls have to be adjusted as well
Applicability::HasPlaceholders,
);
}

if let Some(parent_item) = parent_item {
let parent_descr = tcx.def_kind(parent_item).descr(parent_item.to_def_id());
err.span_label(
Expand Down Expand Up @@ -743,6 +821,7 @@ impl<'tcx> DeadVisitor<'tcx> {
def_id: LocalDefId,
participle: &str,
dead_codes: Vec<DeadVariant>,
is_positional: bool,
) {
let mut dead_codes = dead_codes
.iter()
Expand All @@ -758,12 +837,13 @@ impl<'tcx> DeadVisitor<'tcx> {
&group.map(|v| v.def_id).collect::<Vec<_>>(),
participle,
Some(def_id),
is_positional,
);
}
}

fn warn_dead_code(&mut self, id: LocalDefId, participle: &str) {
self.warn_multiple_dead_codes(&[id], participle, None);
self.warn_multiple_dead_codes(&[id], participle, None, false);
}

fn check_definition(&mut self, def_id: LocalDefId) {
Expand Down Expand Up @@ -829,24 +909,37 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
continue;
}

let mut is_positional = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Positional fields are all or nothing. Can is_positional be computed only once, and passed around?

let dead_fields = variant
.fields
.iter()
.filter_map(|field| {
let def_id = field.did.expect_local();
let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
if visitor.should_warn_about_field(&field) {
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;
if let ShouldWarnAboutField::Yes(is_pos) =
visitor.should_warn_about_field(&field)
{
let level = tcx
.lint_level_at_node(
if is_pos {
is_positional = true;
lint::builtin::UNUSED_TUPLE_STRUCT_FIELDS
} else {
lint::builtin::DEAD_CODE
},
hir_id,
)
.0;
Some(DeadVariant { def_id, name: field.name, level })
} else {
None
}
})
.collect();
visitor.warn_dead_fields_and_variants(def_id, "read", dead_fields)
visitor.warn_dead_fields_and_variants(def_id, "read", dead_fields, is_positional)
}

visitor.warn_dead_fields_and_variants(item.def_id, "constructed", dead_variants);
visitor.warn_dead_fields_and_variants(item.def_id, "constructed", dead_variants, false);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/codegen-units/item-collection/generic-drop-glue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ enum EnumNoDrop<T1, T2> {
}


struct NonGenericNoDrop(i32);
struct NonGenericNoDrop(#[allow(unused_tuple_struct_fields)] i32);

struct NonGenericWithDrop(i32);
struct NonGenericWithDrop(#[allow(unused_tuple_struct_fields)] i32);
//~ MONO_ITEM fn std::ptr::drop_in_place::<NonGenericWithDrop> - shim(Some(NonGenericWithDrop)) @@ generic_drop_glue-cgu.0[Internal]

impl Drop for NonGenericWithDrop {
Expand Down
Loading