Skip to content

Point at signature on unused lint #44847

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

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Point at signature on unused lint #44847

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
28 changes: 21 additions & 7 deletions src/librustc/middle/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,9 +553,22 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> {

fn visit_item(&mut self, item: &'tcx hir::Item) {
if self.should_warn_about_item(item) {
// For items that have a definition with a signature followed by a
// block, point only at the signature.
let span = match item.node {
hir::ItemFn(..) |
hir::ItemMod(..) |
hir::ItemEnum(..) |
hir::ItemStruct(..) |
hir::ItemUnion(..) |
hir::ItemTrait(..) |
hir::ItemDefaultImpl(..) |
hir::ItemImpl(..) => self.tcx.sess.codemap().def_span(item.span),
_ => item.span,
};
self.warn_dead_code(
item.id,
item.span,
span,
item.name,
item.node.descriptive_variant()
);
Expand All @@ -570,8 +583,7 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> {
g: &'tcx hir::Generics,
id: ast::NodeId) {
if self.should_warn_about_variant(&variant.node) {
self.warn_dead_code(variant.node.data.id(), variant.span,
variant.node.name, "variant");
self.warn_dead_code(variant.node.data.id(), variant.span, variant.node.name, "variant");
} else {
intravisit::walk_variant(self, variant, g, id);
}
Expand All @@ -596,15 +608,17 @@ impl<'a, 'tcx> Visitor<'tcx> for DeadVisitor<'a, 'tcx> {
match impl_item.node {
hir::ImplItemKind::Const(_, body_id) => {
if !self.symbol_is_live(impl_item.id, None) {
self.warn_dead_code(impl_item.id, impl_item.span,
impl_item.name, "associated const");
self.warn_dead_code(impl_item.id,
impl_item.span,
impl_item.name,
"associated const");
}
self.visit_nested_body(body_id)
}
hir::ImplItemKind::Method(_, body_id) => {
if !self.symbol_is_live(impl_item.id, None) {
self.warn_dead_code(impl_item.id, impl_item.span,
impl_item.name, "method");
let span = self.tcx.sess.codemap().def_span(impl_item.span);
self.warn_dead_code(impl_item.id, span, impl_item.name, "method");
}
self.visit_nested_body(body_id)
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui-fulldeps/lint-plugin-cmdline-allow.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ warning: function is never used: `lintme`
--> $DIR/lint-plugin-cmdline-allow.rs:20:1
|
20 | fn lintme() { }
| ^^^^^^^^^^^^^^^
| ^^^^^^^^^^^
|
note: lint level defined here
--> $DIR/lint-plugin-cmdline-allow.rs:17:9
Expand Down
12 changes: 4 additions & 8 deletions src/test/ui/path-lookahead.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ warning: unnecessary parentheses around `return` value
warning: function is never used: `with_parens`
--> $DIR/path-lookahead.rs:17:1
|
17 | / fn with_parens<T: ToString>(arg: T) -> String { //~WARN function is never used: `with_parens`
18 | | return (<T as ToString>::to_string(&arg)); //~WARN unnecessary parentheses around `return` value
19 | | }
| |_^
17 | fn with_parens<T: ToString>(arg: T) -> String { //~WARN function is never used: `with_parens`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: lint level defined here
--> $DIR/path-lookahead.rs:13:9
Expand All @@ -24,8 +22,6 @@ note: lint level defined here
warning: function is never used: `no_parens`
--> $DIR/path-lookahead.rs:21:1
|
21 | / fn no_parens<T: ToString>(arg: T) -> String { //~WARN function is never used: `no_parens`
22 | | return <T as ToString>::to_string(&arg);
23 | | }
| |_^
21 | fn no_parens<T: ToString>(arg: T) -> String { //~WARN function is never used: `no_parens`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

40 changes: 40 additions & 0 deletions src/test/ui/span/unused-warning-point-at-signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// run-pass

#![warn(unused)]

enum Enum {
A,
B,
C,
D,
}

struct Struct {
a: usize,
b: usize,
c: usize,
d: usize,
}

fn func() -> usize {
3
}

fn
func_complete_span()
-> usize
{
3
}

fn main() {}
36 changes: 36 additions & 0 deletions src/test/ui/span/unused-warning-point-at-signature.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
warning: enum is never used: `Enum`
--> $DIR/unused-warning-point-at-signature.rs:15:1
|
15 | enum Enum {
| ^^^^^^^^^
|
note: lint level defined here
--> $DIR/unused-warning-point-at-signature.rs:13:9
|
13 | #![warn(unused)]
| ^^^^^^
= note: #[warn(dead_code)] implied by #[warn(unused)]

warning: struct is never used: `Struct`
--> $DIR/unused-warning-point-at-signature.rs:22:1
|
22 | struct Struct {
| ^^^^^^^^^^^^^

warning: function is never used: `func`
--> $DIR/unused-warning-point-at-signature.rs:29:1
|
29 | fn func() -> usize {
| ^^^^^^^^^^^^^^^^^^

warning: function is never used: `func_complete_span`
--> $DIR/unused-warning-point-at-signature.rs:33:1
|
33 | / fn
34 | | func_complete_span()
35 | | -> usize
36 | | {
37 | | 3
38 | | }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this span including the body? Just because it's multiline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oli-obk it was originally proposed that given that in cases where a multiline span was already being shown, we might as well point at the entire body. This decision can be revised if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

For RLS this is a major annoyance.

Copy link
Member

Choose a reason for hiding this comment

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

@oli-obk For RLS indeed it'd be very handy to only point at the symbol name, but I think from the CLI ergonomics point of view, displaying multiline diagnostics might still be superior? (gives more context, CLIs are often run out-of-the-editor etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would still be multiline, it just wouldn't show the function body. I don't see much use in showing function bodies at all for such diagnostics

Copy link
Member

Choose a reason for hiding this comment

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

If that would work, I guess we could. However, aren't there other def_span users who still rely on the full multiline when a signature is also spanning across multiple lines? I'm not very well versed in rustc itself, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xanewok there're are only two users of def_span at the moment, including this one, and neither rely on it showing the full span when multiline.

Copy link
Member

Choose a reason for hiding this comment

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

@estebank ah, my bad. Must've looked up wrong def_span, sorry. The one from the codemap seems to only be used for lints (if I'm not mistaken?) so I think it'd be great to go ahead and limit the multiline spans only to signatures, without bodies, in lints.

Copy link
Member

@Xanewok Xanewok Oct 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@estebank @oli-obk with the change now also ui\span\multiline-span-E0072.rs fails as follows:
after change:

error[E0072]: recursive type `ListNode` has infinite size
  --> $DIR/multiline-span-E0072.rs:12:1
   |
12 | / struct
13 | | ListNode
   | |________^ recursive type has infinite size
...
16 |       tail: Option<ListNode>,
   |       ---------------------- recursive without indirection
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable

expected:

error[E0072]: recursive type `ListNode` has infinite size
  --> $DIR/multiline-span-E0072.rs:12:1
   |
12 | / struct
13 | | ListNode
14 | | {
15 | |     head: u8,
16 | |     tail: Option<ListNode>,
   | |     ---------------------- recursive without indirection
17 | | }
   | |_^ recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable

Is this desired? On one hand it's handy to highlight the faulty (recursive here) bit within the whole span, on the other highlighting entire definition may be an overkill, especially for RLS (iirc right now RLS can't even highlight primary/secondary spans properly per a single diagnostic, only the primary one).
cc @GuillaumeGomez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xanewok I also prefer the current output, but understand that we're bound by the limitations of VSCode/RLS here.

@nikomatsakis would you have a comment? I'm inclined to move ahead with the change.

| |_^