Don't warn on unused field on union #43397

GuillaumeGomez · 2017-07-21T23:18:19Z

rust-highfive · 2017-07-21T23:18:30Z

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2017-07-21T23:54:09Z

src/librustc/middle/dead.rs

+            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,


This is throwing out the baby with the bathwater, union fields can be legitimately unused.

Ideally, use of a union field in a struct expression (let u = U { a: 10 }) should mark it as "potentially used", and uses in field accesses/patterns (u.b/let U { b } = u) should mark the field as "used" and additionally mark all its "potentially used" siblings as "used".

Just marking all union fields used in struct expressions as "used" in hope that at least one field of the same union will be read somewhere would be a good approximation as well.

kennytm · 2017-07-26T15:55:19Z

compile-fail/union/union-lint-dead-code.rs failed to fail.

[00:40:48] failures:
[00:40:48] 
[00:40:48] ---- [compile-fail] compile-fail/union/union-lint-dead-code.rs stdout ----
[00:40:48] 	
[00:40:48] error: compile-fail test compiled successfully!
[00:40:48] status: exit code: 0
[00:40:48] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/compile-fail/union/union-lint-dead-code.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail --target=x86_64-unknown-linux-gnu --error-format json -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/union/union-lint-dead-code.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux -C prefer-dynamic -o /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/union/union-lint-dead-code.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers
[00:40:48] stdout:
[00:40:48] ------------------------------------------
[00:40:48] 
[00:40:48] ------------------------------------------
[00:40:48] stderr:
[00:40:48] ------------------------------------------
[00:40:48] 
[00:40:48] ------------------------------------------
[00:40:48] 
[00:40:48] thread '[compile-fail] compile-fail/union/union-lint-dead-code.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2479:8
[00:40:48] 
[00:40:48] 
[00:40:48] failures:
[00:40:48]     [compile-fail] compile-fail/union/union-lint-dead-code.rs
[00:40:48] 
[00:40:48] test result: FAILED. 2704 passed; 1 failed; 13 ignored; 0 measured; 0 filtered out

The test case

#![deny(dead_code)]

union Foo {
    x: usize,
    b: bool, //~ ERROR: field is never used
    _unused: u16,
}

fn field_read(f: Foo) -> usize {
    unsafe { f.x }
}

fn main() {
    let _ = field_read(Foo { x: 2 });
}

GuillaumeGomez · 2017-07-26T17:49:35Z

@kennytm: It seems normal to me?

kennytm · 2017-07-26T17:59:28Z

@GuillaumeGomez Well Travis is failing because of this. And as @petrochenkov said, the b field is legitimately unused in this test case, so it should fail to compile.

GuillaumeGomez · 2017-07-26T18:02:51Z

That's an union. If any of its fields is used, then shouldn't we consider all its fields used as well?

kennytm · 2017-07-26T19:23:33Z

Intuitively,

When a union field is read, then it is used
The data of the field comes from writes that happens before the read, so those writes are used as well.
Anything else makes the field unused

union U {
    x: u32,
    y: f32,
    z: [u16; 2],
    w: [u8; 4],
}

unsafe {
    let mut u = U { x: 1 };
    // x: written, y: -, z: -, w: -
    let y = u.y; 
    // x: written, y: read, z: -, w: -
    // -> x: used, y: used, z: -, w: - 
    // `y` is used because it is read.
    // `x` is used because the write to `x` happens before read of `y`.
    u.z = [5, 6];
    // x: used, y: used, z: written, w: -
    drop(u);
    // w is entirely untouched, so unused.
    // z is written, but the union is no longer read, so also unused.
    // (current Rust treats `z` is used, but that's fine.)
}

GuillaumeGomez · 2017-07-30T17:09:38Z

I think the new code respects what you wanted @kennytm?

GuillaumeGomez · 2017-08-01T11:55:25Z

Any news in here?

cc @petrochenkov @kennytm

petrochenkov · 2017-08-01T12:01:14Z

I will review today or tomorrow.

petrochenkov · 2017-08-01T21:32:54Z

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


Could you avoid changing this infrastructure? It's used everywhere.

petrochenkov · 2017-08-01T21:34:07Z

src/librustc/middle/dead.rs

+                        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,


The lint should not depend on the field definition order in any way, so this must be incorrect.

I suggest to implement #43397 (comment), e.g. visit ~~hir::ExprPath~~ hir::ExprStruct in MarkSymbolVisitor::visit_expr and mark all fields in it as used if they are union fields.

The only way I see to mark a field as used is to add the "used" attr into its attr list. However, I can't do it in the visit_expr method since the received parameter isn't mutable. Or maybe is there another way I didn't see?

By "marking as used" I mean inserting the field's id into live_symbols set in MarkSymbolVisitor.

One tiny tweak: "mark all fields in it as used if they are union fields" -> "mark all fields in it as used if they are union fields and the union has >1 fields". Test for this:

union NoDropLike { a: u8 } // should be reported as unused let u = NoDropLike { a: 10 };

petrochenkov · 2017-08-01T21:39:09Z

src/test/ui/union-fields.stderr

+  --> $DIR/union-fields.rs:20:5
+   |
+20 |     y: u32,
+   |     ^^^^^^


Diagnostics for unions are not actually tested here.
Could you remove the struct V (it's not especially relevant to the test) and test this instead:

union U1 { a: u8, // should not be reported b: u8, // should not be reported c: u8, // should be reported } union U2 { a: u8, // should be reported b: u8, // should not be reported c: u8, // should not be reported } let u = U1 { a: 0 }; let a = u.b; let u = U2 { c: 0 }; let b = u.b;

GuillaumeGomez · 2017-08-06T15:20:40Z

src/librustc/middle/dead.rs

+    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 {


@petrochenkov: For now it still doesn't work and I think it's because of here: it never enter in here. Any idea what I did wrong?

Oh, sorry, I meant hir::ExprStruct in #43397 (comment) of course, not hir::ExprPath.
mark_as_used_if_union is good except that it should add only fields used in ExprStruct to the live_symbols set, not all fields from the union.
Nit: mark_as_used_if_union could also use some if lets.

Ah! That makes more sense!

GuillaumeGomez · 2017-08-06T16:50:52Z

@petrochenkov: Works way better with the correct check. ;) I think it's all good now. Last check?

kennytm · 2017-08-06T17:46:57Z

ICE when compiling rustc (librustc_errors) at stage1.

[00:22:33] error: internal compiler error: /checkout/src/librustc/hir/def.rs:162: attempted .def_id() on invalid def: SelfTy(None, Some(DefId { krate: CrateNum(0), node: DefIndex(3530) => rustc/3eeda9a::middle[0]::free_region[0]::{{impl}}[0] }))
[00:22:33] 
[00:22:34] note: the compiler unexpectedly panicked. this is a bug.
[00:22:34] 
[00:22:34] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:22:34] 
[00:22:34] note: rustc 1.21.0-dev (0bd7b6f4b 2017-08-06) running on x86_64-unknown-linux-gnu
[00:22:34] 
[00:22:34] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc_errors/lib.rs:486:8

GuillaumeGomez · 2017-08-06T17:57:07Z

Strange. Checking what's wrong.

petrochenkov · 2017-08-06T18:02:57Z

src/librustc/middle/dead.rs

@@ -231,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);


The last thing is to make it work for type aliases (it'll also fix the ICE with Self).

You can use self.tcx.expr_ty(expr) to get the struct expression's type and then check that it's a union.

Test:

union U { a: u8, // should not be reported b: u8, // should not be reported c: u8, // should be reported } type A = U; let u = A { a: 0 }; let b = unsafe { u.b };

GuillaumeGomez · 2017-08-06T18:46:53Z

And done as well!

petrochenkov · 2017-08-06T18:57:53Z

@bors r+

bors · 2017-08-06T18:57:53Z

📌 Commit f94157e has been approved by petrochenkov

bors · 2017-08-06T18:58:03Z

⌛ Testing commit f94157e with merge ba1d065...

Don't warn on unused field on union Fixes #43393.

petrochenkov · 2017-08-06T18:59:29Z

@GuillaumeGomez
If this fails CI, could you squash commits before resubmitting?

GuillaumeGomez · 2017-08-06T19:02:10Z

Sure.

bors · 2017-08-06T21:23:27Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing ba1d065 to master...

joshtriplett · 2017-08-09T14:51:08Z

The original test case in #43393 caused the compiler to warn about an unused field for a field that got used in initialization, which definitely seems like a bug. With this change, does the compiler still warn about a field that nothing initializes or reads from or writes to, or does this disable the warning entirely?

GuillaumeGomez · 2017-08-10T09:14:50Z

Do you have a code that could allow us to check if it's fixed?

petrochenkov · 2017-08-10T09:17:54Z

@joshtriplett

With this change, does the compiler still warn about a field that nothing initializes or reads from or writes to, or does this disable the warning entirely?

Yes, the warning is still issued for completely unused fields, see examples in the tests.

rust-highfive assigned pnkfelix Jul 21, 2017

GuillaumeGomez force-pushed the unused-union-field branch from d425fa8 to b24a625 Compare July 21, 2017 23:20

petrochenkov reviewed Jul 21, 2017

View reviewed changes

petrochenkov assigned petrochenkov and unassigned pnkfelix Jul 21, 2017

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 24, 2017

GuillaumeGomez force-pushed the unused-union-field branch from b24a625 to 1729c2e Compare July 30, 2017 17:08

petrochenkov reviewed Aug 1, 2017

View reviewed changes

GuillaumeGomez added 3 commits August 5, 2017 22:02

Don't warn on unused field on union

46fe8e9

Improve dead code detection for unions

59fcac6

Improve union unused field detection

90f54d0

GuillaumeGomez force-pushed the unused-union-field branch from 1729c2e to 90f54d0 Compare August 6, 2017 15:19

GuillaumeGomez commented Aug 6, 2017

View reviewed changes

Fix union unused fields check

09420fc

petrochenkov reviewed Aug 6, 2017

View reviewed changes

Handle type aliases as well

f94157e

bors added a commit that referenced this pull request Aug 6, 2017

Auto merge of #43397 - GuillaumeGomez:unused-union-field, r=petrochenkov

ba1d065

Don't warn on unused field on union Fixes #43393.

bors merged commit f94157e into rust-lang:master Aug 6, 2017

GuillaumeGomez deleted the unused-union-field branch August 6, 2017 21:56

Don't warn on unused field on union #43397

Don't warn on unused field on union #43397

Conversation

GuillaumeGomez commented Jul 21, 2017

rust-highfive commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm commented Jul 26, 2017

GuillaumeGomez commented Jul 26, 2017

kennytm commented Jul 26, 2017

GuillaumeGomez commented Jul 26, 2017

kennytm commented Jul 26, 2017 • edited Loading

GuillaumeGomez commented Jul 30, 2017

GuillaumeGomez commented Aug 1, 2017

petrochenkov commented Aug 1, 2017

Choose a reason for hiding this comment

petrochenkov Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

petrochenkov Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 6, 2017

kennytm commented Aug 6, 2017 • edited Loading

GuillaumeGomez commented Aug 6, 2017

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 6, 2017

petrochenkov commented Aug 6, 2017

bors commented Aug 6, 2017

bors commented Aug 6, 2017

petrochenkov commented Aug 6, 2017

GuillaumeGomez commented Aug 6, 2017

bors commented Aug 6, 2017

joshtriplett commented Aug 9, 2017

GuillaumeGomez commented Aug 10, 2017

petrochenkov commented Aug 10, 2017

kennytm commented Jul 26, 2017 •

edited

Loading

petrochenkov Aug 1, 2017 •

edited

Loading

petrochenkov Aug 1, 2017 •

edited

Loading

kennytm commented Aug 6, 2017 •

edited

Loading