From 46fe8e99665d6a210f6dc16590127ee808e60366 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 22 Jul 2017 01:17:53 +0200 Subject: [PATCH 1/5] Don't warn on unused field on union --- src/librustc/middle/dead.rs | 18 +++++++++++++++--- src/test/ui/union-fields.rs | 29 +++++++++++++++++++++++++++++ src/test/ui/union-fields.stderr | 14 ++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/union-fields.rs create mode 100644 src/test/ui/union-fields.stderr diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index 2238e464cbcd5..91d25c680d87a 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -13,7 +13,7 @@ // from live codes are live, and everything else is dead. use hir::map as hir_map; -use hir::{self, PatKind}; +use hir::{self, Item_, PatKind}; use hir::intravisit::{self, Visitor, NestedVisitorMap}; use hir::itemlikevisit::ItemLikeVisitor; @@ -558,8 +558,20 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> { fn visit_struct_field(&mut self, field: &'tcx hir::StructField) { if self.should_warn_about_field(&field) { - self.warn_dead_code(field.id, field.span, - field.name, "field"); + let did = self.tcx.hir.get_parent_did(field.id); + if if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { + match self.tcx.hir.find(node_id) { + Some(hir_map::NodeItem(item)) => match item.node { + Item_::ItemUnion(_, _) => false, + _ => true, + }, + _ => true, + } + } else { + true + } { + self.warn_dead_code(field.id, field.span, field.name, "field"); + } } intravisit::walk_struct_field(self, field); diff --git a/src/test/ui/union-fields.rs b/src/test/ui/union-fields.rs new file mode 100644 index 0000000000000..87a617301336c --- /dev/null +++ b/src/test/ui/union-fields.rs @@ -0,0 +1,29 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![deny(dead_code)] + +union U { + x: u32, + y: f32, +} + +struct V { + x: u32, + y: u32, +} + +fn main() { + let u = U { x: 0x3f800000 }; + let _f = unsafe { u.y }; + let v = V { x: 0, y: 0 }; + println!("{}", v.x); +} + diff --git a/src/test/ui/union-fields.stderr b/src/test/ui/union-fields.stderr new file mode 100644 index 0000000000000..d0f1a9214255b --- /dev/null +++ b/src/test/ui/union-fields.stderr @@ -0,0 +1,14 @@ +error: field is never used: `y` + --> $DIR/union-fields.rs:20:5 + | +20 | y: u32, + | ^^^^^^ + | +note: lint level defined here + --> $DIR/union-fields.rs:11:9 + | +11 | #![deny(dead_code)] + | ^^^^^^^^^ + +error: aborting due to previous error + From 59fcac6fa977630640fb79c4c97486b14ca66cee Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 30 Jul 2017 19:08:26 +0200 Subject: [PATCH 2/5] Improve dead code detection for unions --- src/librustc/hir/intravisit.rs | 2 +- src/librustc/middle/dead.rs | 46 +++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 57198d8ca0b77..e751241669dae 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -877,7 +877,7 @@ pub fn walk_impl_item_ref<'v, V: Visitor<'v>>(visitor: &mut V, impl_item_ref: &' pub fn walk_struct_def<'v, V: Visitor<'v>>(visitor: &mut V, struct_definition: &'v VariantData) { visitor.visit_id(struct_definition.id()); - walk_list!(visitor, visit_struct_field, struct_definition.fields()); + walk_list!(visitor, visit_struct_field, struct_definition.fields().iter().rev()); } pub fn walk_struct_field<'v, V: Visitor<'v>>(visitor: &mut V, struct_field: &'v StructField) { diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index 91d25c680d87a..2f0ee8d8f2fb2 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -422,6 +422,7 @@ fn get_struct_ctor_id(item: &hir::Item) -> Option { struct DeadVisitor<'a, 'tcx: 'a> { tcx: TyCtxt<'a, 'tcx, 'tcx>, live_symbols: Box>, + need_check_next_union_field: bool, } impl<'a, 'tcx> DeadVisitor<'a, 'tcx> { @@ -537,6 +538,16 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> { } } + fn visit_variant_data(&mut self, + s: &'tcx hir::VariantData, + _: ast::Name, + _: &'tcx hir::Generics, + _parent_id: ast::NodeId, + _: syntax_pos::Span) { + self.need_check_next_union_field = true; + intravisit::walk_struct_def(self, s) + } + fn visit_variant(&mut self, variant: &'tcx hir::Variant, g: &'tcx hir::Generics, @@ -557,23 +568,24 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> { } fn visit_struct_field(&mut self, field: &'tcx hir::StructField) { - if self.should_warn_about_field(&field) { - let did = self.tcx.hir.get_parent_did(field.id); - if if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { - match self.tcx.hir.find(node_id) { - Some(hir_map::NodeItem(item)) => match item.node { - Item_::ItemUnion(_, _) => false, - _ => true, - }, - _ => true, - } - } else { - true - } { + if self.need_check_next_union_field { + if self.should_warn_about_field(&field) { self.warn_dead_code(field.id, field.span, field.name, "field"); + } else { + let did = self.tcx.hir.get_parent_did(field.id); + if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { + match self.tcx.hir.find(node_id) { + Some(hir_map::NodeItem(item)) => match item.node { + // If this is an union's field, it means all previous fields + // have been used as well so no need to check further. + Item_::ItemUnion(_, _) => self.need_check_next_union_field = false, + _ => {} + }, + _ => {} + } + } } } - intravisit::walk_struct_field(self, field); } @@ -615,6 +627,10 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let access_levels = &tcx.privacy_access_levels(LOCAL_CRATE); let krate = tcx.hir.krate(); let live_symbols = find_live(tcx, access_levels, krate); - let mut visitor = DeadVisitor { tcx: tcx, live_symbols: live_symbols }; + let mut visitor = DeadVisitor { + tcx: tcx, + live_symbols: live_symbols, + need_check_next_union_field: true, + }; intravisit::walk_crate(&mut visitor, krate); } From 90f54d00d313178a6246566a5f09cd6af1f7a099 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 6 Aug 2017 17:19:15 +0200 Subject: [PATCH 3/5] Improve union unused field detection --- src/librustc/hir/intravisit.rs | 2 +- src/librustc/middle/dead.rs | 55 ++++++++++++++++------------------ src/test/ui/union-fields.rs | 28 +++++++++-------- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index e751241669dae..57198d8ca0b77 100644 --- a/src/librustc/hir/intravisit.rs +++ b/src/librustc/hir/intravisit.rs @@ -877,7 +877,7 @@ pub fn walk_impl_item_ref<'v, V: Visitor<'v>>(visitor: &mut V, impl_item_ref: &' pub fn walk_struct_def<'v, V: Visitor<'v>>(visitor: &mut V, struct_definition: &'v VariantData) { visitor.visit_id(struct_definition.id()); - walk_list!(visitor, visit_struct_field, struct_definition.fields().iter().rev()); + walk_list!(visitor, visit_struct_field, struct_definition.fields()); } pub fn walk_struct_field<'v, V: Visitor<'v>>(visitor: &mut V, struct_field: &'v StructField) { diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index 2f0ee8d8f2fb2..c82cfb3449671 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -189,6 +189,24 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> { self.struct_has_extern_repr = had_extern_repr; self.inherited_pub_visibility = had_inherited_pub_visibility; } + + fn mark_as_used_if_union(&mut self, did: DefId) { + if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { + match self.tcx.hir.find(node_id) { + Some(hir_map::NodeItem(item)) => match item.node { + Item_::ItemUnion(ref variant, _) => { + if variant.fields().len() > 1 { + for field in variant.fields() { + self.live_symbols.insert(field.id); + } + } + } + _ => {} + }, + _ => {} + } + } + } } impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> { @@ -221,6 +239,11 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> { hir::ExprPath(ref qpath @ hir::QPath::TypeRelative(..)) => { let def = self.tables.qpath_def(qpath, expr.id); self.handle_definition(def); + self.mark_as_used_if_union(def.def_id()); + } + hir::ExprPath(ref qpath @ hir::QPath::Resolved(..)) => { + let def = self.tables.qpath_def(qpath, expr.id); + self.mark_as_used_if_union(def.def_id()); } hir::ExprMethodCall(..) => { self.lookup_and_handle_method(expr.id); @@ -422,7 +445,6 @@ fn get_struct_ctor_id(item: &hir::Item) -> Option { struct DeadVisitor<'a, 'tcx: 'a> { tcx: TyCtxt<'a, 'tcx, 'tcx>, live_symbols: Box>, - need_check_next_union_field: bool, } impl<'a, 'tcx> DeadVisitor<'a, 'tcx> { @@ -538,16 +560,6 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> { } } - fn visit_variant_data(&mut self, - s: &'tcx hir::VariantData, - _: ast::Name, - _: &'tcx hir::Generics, - _parent_id: ast::NodeId, - _: syntax_pos::Span) { - self.need_check_next_union_field = true; - intravisit::walk_struct_def(self, s) - } - fn visit_variant(&mut self, variant: &'tcx hir::Variant, g: &'tcx hir::Generics, @@ -568,23 +580,9 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> { } fn visit_struct_field(&mut self, field: &'tcx hir::StructField) { - if self.need_check_next_union_field { - if self.should_warn_about_field(&field) { - self.warn_dead_code(field.id, field.span, field.name, "field"); - } else { - let did = self.tcx.hir.get_parent_did(field.id); - if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { - match self.tcx.hir.find(node_id) { - Some(hir_map::NodeItem(item)) => match item.node { - // If this is an union's field, it means all previous fields - // have been used as well so no need to check further. - Item_::ItemUnion(_, _) => self.need_check_next_union_field = false, - _ => {} - }, - _ => {} - } - } - } + if self.should_warn_about_field(&field) { + self.warn_dead_code(field.id, field.span, + field.name, "field"); } intravisit::walk_struct_field(self, field); } @@ -630,7 +628,6 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let mut visitor = DeadVisitor { tcx: tcx, live_symbols: live_symbols, - need_check_next_union_field: true, }; intravisit::walk_crate(&mut visitor, krate); } diff --git a/src/test/ui/union-fields.rs b/src/test/ui/union-fields.rs index 87a617301336c..7b39a548fe9d2 100644 --- a/src/test/ui/union-fields.rs +++ b/src/test/ui/union-fields.rs @@ -10,20 +10,24 @@ #![deny(dead_code)] -union U { - x: u32, - y: f32, +union U1 { + a: u8, // should not be reported + b: u8, // should not be reported + c: u8, // should be reported } - -struct V { - x: u32, - y: u32, +union U2 { + a: u8, // should be reported + b: u8, // should not be reported + c: u8, // should not be reported } +union NoDropLike { a: u8 } // should be reported as unused fn main() { - let u = U { x: 0x3f800000 }; - let _f = unsafe { u.y }; - let v = V { x: 0, y: 0 }; - println!("{}", v.x); -} + let u = U1 { a: 0 }; + let _a = unsafe { u.b }; + let u = U2 { c: 0 }; + let _b = unsafe { u.b }; + + let _u = NoDropLike { a: 10 }; +} From 09420fc2060e08e332efd00098cda6447285290d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 6 Aug 2017 18:49:33 +0200 Subject: [PATCH 4/5] Fix union unused fields check --- src/librustc/middle/dead.rs | 25 +++++++++++-------------- src/test/ui/union-fields.stderr | 22 +++++++++++++++++----- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index c82cfb3449671..6532cde9715b0 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -190,20 +190,18 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> { self.inherited_pub_visibility = had_inherited_pub_visibility; } - fn mark_as_used_if_union(&mut self, did: DefId) { + fn mark_as_used_if_union(&mut self, did: DefId, fields: &hir::HirVec) { if let Some(node_id) = self.tcx.hir.as_local_node_id(did) { - match self.tcx.hir.find(node_id) { - Some(hir_map::NodeItem(item)) => match item.node { - Item_::ItemUnion(ref variant, _) => { - if variant.fields().len() > 1 { - for field in variant.fields() { + if let Some(hir_map::NodeItem(item)) = self.tcx.hir.find(node_id) { + if let Item_::ItemUnion(ref variant, _) = item.node { + if variant.fields().len() > 1 { + for field in variant.fields() { + if fields.iter().find(|x| x.name.node == field.name).is_some() { self.live_symbols.insert(field.id); } } } - _ => {} - }, - _ => {} + } } } } @@ -239,11 +237,6 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> { hir::ExprPath(ref qpath @ hir::QPath::TypeRelative(..)) => { let def = self.tables.qpath_def(qpath, expr.id); self.handle_definition(def); - self.mark_as_used_if_union(def.def_id()); - } - hir::ExprPath(ref qpath @ hir::QPath::Resolved(..)) => { - let def = self.tables.qpath_def(qpath, expr.id); - self.mark_as_used_if_union(def.def_id()); } hir::ExprMethodCall(..) => { self.lookup_and_handle_method(expr.id); @@ -254,6 +247,10 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> { hir::ExprTupField(ref lhs, idx) => { self.handle_tup_field_access(&lhs, idx.node); } + hir::ExprStruct(ref qpath, ref fields, _) => { + let def = self.tables.qpath_def(qpath, expr.id); + self.mark_as_used_if_union(def.def_id(), fields); + } _ => () } diff --git a/src/test/ui/union-fields.stderr b/src/test/ui/union-fields.stderr index d0f1a9214255b..5c47ba388a452 100644 --- a/src/test/ui/union-fields.stderr +++ b/src/test/ui/union-fields.stderr @@ -1,8 +1,8 @@ -error: field is never used: `y` - --> $DIR/union-fields.rs:20:5 +error: field is never used: `c` + --> $DIR/union-fields.rs:16:5 | -20 | y: u32, - | ^^^^^^ +16 | c: u8, // should be reported + | ^^^^^ | note: lint level defined here --> $DIR/union-fields.rs:11:9 @@ -10,5 +10,17 @@ note: lint level defined here 11 | #![deny(dead_code)] | ^^^^^^^^^ -error: aborting due to previous error +error: field is never used: `a` + --> $DIR/union-fields.rs:19:5 + | +19 | a: u8, // should be reported + | ^^^^^ + +error: field is never used: `a` + --> $DIR/union-fields.rs:23:20 + | +23 | union NoDropLike { a: u8 } // should be reported as unused + | ^^^^^ + +error: aborting due to 3 previous errors From f94157eb616d18655809ea60af870e1888476c9a Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 6 Aug 2017 20:46:32 +0200 Subject: [PATCH 5/5] Handle type aliases as well --- src/librustc/middle/dead.rs | 9 ++++++--- src/test/ui/union-fields.rs | 9 +++++++++ src/test/ui/union-fields.stderr | 8 +++++++- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs index 6532cde9715b0..a525b4e13b78d 100644 --- a/src/librustc/middle/dead.rs +++ b/src/librustc/middle/dead.rs @@ -247,9 +247,12 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> { hir::ExprTupField(ref lhs, idx) => { self.handle_tup_field_access(&lhs, idx.node); } - hir::ExprStruct(ref qpath, ref fields, _) => { - let def = self.tables.qpath_def(qpath, expr.id); - self.mark_as_used_if_union(def.def_id(), fields); + hir::ExprStruct(_, ref fields, _) => { + if let ty::TypeVariants::TyAdt(ref def, _) = self.tables.expr_ty(expr).sty { + if def.is_union() { + self.mark_as_used_if_union(def.did, fields); + } + } } _ => () } diff --git a/src/test/ui/union-fields.rs b/src/test/ui/union-fields.rs index 7b39a548fe9d2..021f57e3eee0a 100644 --- a/src/test/ui/union-fields.rs +++ b/src/test/ui/union-fields.rs @@ -22,6 +22,13 @@ union U2 { } union NoDropLike { a: u8 } // should be reported as unused +union U { + a: u8, // should not be reported + b: u8, // should not be reported + c: u8, // should be reported +} +type A = U; + fn main() { let u = U1 { a: 0 }; let _a = unsafe { u.b }; @@ -30,4 +37,6 @@ fn main() { let _b = unsafe { u.b }; let _u = NoDropLike { a: 10 }; + let u = A { a: 0 }; + let _b = unsafe { u.b }; } diff --git a/src/test/ui/union-fields.stderr b/src/test/ui/union-fields.stderr index 5c47ba388a452..f3a2702d5aefa 100644 --- a/src/test/ui/union-fields.stderr +++ b/src/test/ui/union-fields.stderr @@ -22,5 +22,11 @@ error: field is never used: `a` 23 | union NoDropLike { a: u8 } // should be reported as unused | ^^^^^ -error: aborting due to 3 previous errors +error: field is never used: `c` + --> $DIR/union-fields.rs:28:5 + | +28 | c: u8, // should be reported + | ^^^^^ + +error: aborting due to 4 previous errors