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

Warn about dead tuple struct fields #92972

Closed
wants to merge 1 commit into from
Closed

Warn about dead tuple struct fields #92972

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(dead_code)] F);
Copy link
Member

Choose a reason for hiding this comment

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

I wish we had a better name for this than "dead code". I'm actually surprised it's not "unused" instead.

I suppose this change is small enough, and just a lint attribute, that it's probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you could use unused as well, but that is the lint group containing the dead_code lint, so the latter is more precise (not that it makes a difference here). Also, just to be clear: I didn't come up with the name here, it is also called dead_code for named fields.

In any case: Thanks for having a look!

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(dead_code)] F);
type FallbackExtended<F> = ieee::IeeeFloat<FallbackExtendedS<F>>;
impl<F: Float> ieee::Semantics for FallbackExtendedS<F> {
// Forbid any conversion to/from bits.
Expand Down
25 changes: 11 additions & 14 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2377,9 +2377,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
#[derive(Debug)]
enum SubOrigin<'hir> {
GAT(&'hir hir::Generics<'hir>),
Impl(&'hir hir::Generics<'hir>),
Trait(&'hir hir::Generics<'hir>),
Fn(&'hir hir::Generics<'hir>),
Impl,
Trait,
Fn,
Unknown,
}
let sub_origin = 'origin: {
Expand All @@ -2397,31 +2397,28 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}) => SubOrigin::GAT(generics),
Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Fn(..),
generics,
..
}) => SubOrigin::Fn(generics),
}) => SubOrigin::Fn,
Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Type(..),
generics,
..
}) => SubOrigin::GAT(generics),
Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(..),
generics,
..
}) => SubOrigin::Fn(generics),
}) => SubOrigin::Fn,
Node::Item(hir::Item {
kind: hir::ItemKind::Trait(_, _, generics, _, _),
kind: hir::ItemKind::Trait(..),
..
}) => SubOrigin::Trait(generics),
}) => SubOrigin::Trait,
Node::Item(hir::Item {
kind: hir::ItemKind::Impl(hir::Impl { generics, .. }),
kind: hir::ItemKind::Impl(hir::Impl { .. }),
..
}) => SubOrigin::Impl(generics),
}) => SubOrigin::Impl,
Node::Item(hir::Item {
kind: hir::ItemKind::Fn(_, generics, _),
..
}) => SubOrigin::Fn(generics),
kind: hir::ItemKind::Fn(..), ..
}) => SubOrigin::Fn,
_ => continue,
};
}
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 @@ -51,7 +51,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 @@ -1889,12 +1889,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
69 changes: 64 additions & 5 deletions compiler/rustc_passes/src/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,11 @@ 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,
inherited_pub_visibility: bool,
pub_visibility: bool,
allow_dead_field: bool,
ignore_variant_stack: Vec<DefId>,
// maps from tuple struct constructors to tuple struct items
struct_constructors: FxHashMap<LocalDefId, LocalDefId>,
Expand Down Expand Up @@ -221,6 +223,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 @@ -269,19 +297,38 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}

let had_repr_c = self.repr_has_repr_c;
let had_repr_simd = self.repr_has_repr_simd;
let had_inherited_pub_visibility = self.inherited_pub_visibility;
let had_pub_visibility = self.pub_visibility;
let had_allow_dead_field = self.allow_dead_field;
self.repr_has_repr_c = false;
self.repr_has_repr_simd = false;
self.inherited_pub_visibility = false;
self.pub_visibility = false;
self.allow_dead_field = false;
match node {
Node::Item(item) => {
self.pub_visibility = item.vis.node.is_pub();

match item.kind {
hir::ItemKind::Struct(..) | hir::ItemKind::Union(..) => {
match &item.kind {
hir::ItemKind::Struct(vd, ..) | hir::ItemKind::Union(vd, ..) => {
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();

// A single non-public field of unit type in a public tuple struct
// can be used to make the tuple struct constructor private. This
// is allowed and shouldn't yield a "field is never read" warning.
if let hir::VariantData::Tuple([field_def], _) = vd {
match field_def.vis.node {
hir::VisibilityKind::Public => {}
_ => {
if let hir::TyKind::Tup([]) = field_def.ty.kind {
self.allow_dead_field = true;
}
}
}
}

intravisit::walk_item(self, &item);
}
Expand All @@ -307,8 +354,10 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}
_ => {}
}
self.allow_dead_field = had_allow_dead_field;
self.pub_visibility = had_pub_visibility;
self.inherited_pub_visibility = had_inherited_pub_visibility;
self.repr_has_repr_simd = had_repr_simd;
self.repr_has_repr_c = had_repr_c;
}

Expand Down Expand Up @@ -346,10 +395,15 @@ impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> {
_: rustc_span::Span,
) {
let has_repr_c = self.repr_has_repr_c;
let has_repr_simd = self.repr_has_repr_simd;
let inherited_pub_visibility = self.inherited_pub_visibility;
let pub_visibility = self.pub_visibility;
let allow_dead_field = self.allow_dead_field;
let live_fields = def.fields().iter().filter(|f| {
has_repr_c || (pub_visibility && (inherited_pub_visibility || f.vis.node.is_pub()))
has_repr_c
|| (pub_visibility && (inherited_pub_visibility || f.vis.node.is_pub()))
|| (f.is_positional() && has_repr_simd)
|| allow_dead_field
});
let hir = self.tcx.hir();
self.live_symbols.extend(live_fields.map(|f| hir.local_def_id(f.hir_id)));
Expand Down Expand Up @@ -403,6 +457,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 @@ -585,9 +643,11 @@ fn find_live<'tcx>(
maybe_typeck_results: None,
live_symbols: Default::default(),
repr_has_repr_c: false,
repr_has_repr_simd: false,
in_pat: false,
inherited_pub_visibility: false,
pub_visibility: false,
allow_dead_field: false,
ignore_variant_stack: vec![],
struct_constructors,
};
Expand Down Expand Up @@ -618,8 +678,7 @@ impl<'tcx> DeadVisitor<'tcx> {
fn should_warn_about_field(&mut self, field: &hir::FieldDef<'_>) -> bool {
let def_id = self.tcx.hir().local_def_id(field.hir_id);
let field_type = self.tcx.type_of(def_id);
!field.is_positional()
&& !self.symbol_is_live(def_id)
!self.symbol_is_live(def_id)
&& !field_type.is_phantom_data()
&& !has_allow_dead_code_or_lang_attr(self.tcx, field.hir_id)
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
let crate_root = self.r.resolve_crate_root(source.ident);
let crate_name = match crate_root.kind {
ModuleKind::Def(.., name) => name,
ModuleKind::Block(..) => unreachable!(),
ModuleKind::Block => unreachable!(),
};
// HACK(eddyb) unclear how good this is, but keeping `$crate`
// in `source` breaks `src/test/ui/imports/import-crate-var.rs`,
Expand Down Expand Up @@ -927,7 +927,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
if self.block_needs_anonymous_module(block) {
let module = self.r.new_module(
Some(parent),
ModuleKind::Block(block.id),
ModuleKind::Block,
expansion.to_expn_id(),
block.span,
parent.no_implicit_prelude,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_resolve/src/late/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1307,7 +1307,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
// Items from this module
self.r.add_module_candidates(module, &mut names, &filter_fn);

if let ModuleKind::Block(..) = module.kind {
if let ModuleKind::Block = module.kind {
// We can see through blocks
} else {
// Items from the prelude
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ enum ModuleKind {
/// f(); // Resolves to (1)
/// }
/// ```
Block(NodeId),
Block,
/// Any module with a name.
///
/// This could be:
Expand All @@ -482,7 +482,7 @@ impl ModuleKind {
/// Get name of the module.
pub fn name(&self) -> Option<Symbol> {
match self {
ModuleKind::Block(..) => None,
ModuleKind::Block => None,
ModuleKind::Def(.., name) => Some(*name),
}
}
Expand Down Expand Up @@ -558,7 +558,7 @@ impl<'a> ModuleData<'a> {
) -> Self {
let is_foreign = match kind {
ModuleKind::Def(_, def_id, _) => !def_id.is_local(),
ModuleKind::Block(_) => false,
ModuleKind::Block => false,
};
ModuleData {
parent,
Expand Down Expand Up @@ -1987,7 +1987,7 @@ impl<'a> Resolver<'a> {
};

match module.kind {
ModuleKind::Block(..) => {} // We can see through blocks
ModuleKind::Block => {} // We can see through blocks
_ => break,
}

Expand Down Expand Up @@ -2026,7 +2026,7 @@ impl<'a> Resolver<'a> {
return Some((self.expn_def_scope(ctxt.remove_mark()), None));
}

if let ModuleKind::Block(..) = module.kind {
if let ModuleKind::Block = module.kind {
return Some((module.parent.unwrap().nearest_item_scope(), None));
}

Expand Down Expand Up @@ -3029,7 +3029,7 @@ impl<'a> Resolver<'a> {

let container = match parent.kind {
ModuleKind::Def(kind, _, _) => kind.descr(parent.def_id()),
ModuleKind::Block(..) => "block",
ModuleKind::Block => "block",
};

let old_noun = match old_binding.is_import() {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_serialize/tests/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,8 @@ struct DecodeStruct {
}
#[derive(Decodable)]
enum DecodeEnum {
A(f64),
B(string::String),
A(#[allow(dead_code)] f64),
B(#[allow(dead_code)] string::String),
}
fn check_err<T: Decodable<Decoder>>(to_parse: &'static str, expected: DecoderError) {
let res: DecodeResult<T> = match from_str(to_parse) {
Expand Down
1 change: 1 addition & 0 deletions library/alloc/src/boxed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
//!
//! ```
//! #[derive(Debug)]
//! # #[allow(dead_code)]
//! enum List<T> {
//! Cons(T, Box<List<T>>),
//! Nil,
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/collections/btree/set/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ fn test_extend_ref() {
#[test]
fn test_recovery() {
#[derive(Debug)]
struct Foo(&'static str, i32);
struct Foo(&'static str, #[allow(dead_code)] i32);

impl PartialEq for Foo {
fn eq(&self, other: &Self) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/collections/vec_deque/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ fn test_clone_from() {
fn test_vec_deque_truncate_drop() {
static mut DROPS: u32 = 0;
#[derive(Clone)]
struct Elem(i32);
struct Elem(#[allow(dead_code)] i32);
impl Drop for Elem {
fn drop(&mut self) {
unsafe {
Expand Down
6 changes: 3 additions & 3 deletions library/alloc/tests/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ fn test_cmp() {
#[test]
fn test_vec_truncate_drop() {
static mut DROPS: u32 = 0;
struct Elem(i32);
struct Elem(#[allow(dead_code)] i32);
impl Drop for Elem {
fn drop(&mut self) {
unsafe {
Expand Down Expand Up @@ -1077,7 +1077,7 @@ fn test_from_iter_specialization_panic_during_drop_leaks() {

#[derive(Debug)]
enum Droppable {
DroppedTwice(Box<i32>),
DroppedTwice(#[allow(dead_code)] Box<i32>),
PanicOnDrop,
}

Expand Down Expand Up @@ -2251,7 +2251,7 @@ fn test_vec_dedup_multiple_ident() {
#[test]
fn test_vec_dedup_partialeq() {
#[derive(Debug)]
struct Foo(i32, i32);
struct Foo(i32, #[allow(dead_code)] i32);

impl PartialEq for Foo {
fn eq(&self, other: &Foo) -> bool {
Expand Down
Loading