Skip to content

Commit

Permalink
Auto merge of #95977 - FabianWolff:issue-92790-dead-tuple, r=estebank
Browse files Browse the repository at this point in the history 
Warn about dead tuple struct fields

Continuation of #92972. Fixes #92790.

The language team has already commented on this in #92972 (comment); I have incorporated their requests here. Specifically, there is now a new allow-by-default `unused_tuple_struct_fields` lint (name bikesheddable), and fields of unit type are ignored (#92972 (comment)), so error messages look like this:
```
error: field is never read: `1`
  --> $DIR/tuple-struct-field.rs:6:21
   |
LL | struct Wrapper(i32, [u8; LEN], String);
   |                     ^^^^^^^^^
   |
help: change the field to unit type to suppress this warning while preserving the field numbering
   |
LL | struct Wrapper(i32, (), String);
   |                     ~~
```
r? `@joshtriplett`
  • Loading branch information
@bors
bors committed Aug 5, 2022
2 parents cdfd675 + e3c7e04 commit 9bbbf60
Show file tree
Hide file tree
Showing 168 changed files with 451 additions and 246 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_apfloat/src/ppc.rs
View file
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
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
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 @@ -2020,12 +2020,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
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?
No,
}

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(
&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;
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
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

0 comments on commit 9bbbf60

Please sign in to comment.