Reduce span highlighted code in unused_variables lint

[killercup]

Let's say I wrote this critical piece of code:

fn main() {
    let mut i = 42;
}

rustc gives me the following output:

warning: unused variable: `i`
 --> src/main.rs:2:9
  |
2 |     let mut i = 42;
  |         ^^^^^ help: consider using `_i` instead
  |
  = note: #[warn(unused_variables)] on by default

warning: variable does not need to be mutable
 --> src/main.rs:2:9
  |
2 |     let mut i = 42;
  |         ----^
  |         |
  |         help: remove this `mut`
  |
  = note: #[warn(unused_mut)] on by default

Which is correct -- except for one little detail. Which is crucial, if you are not just any developer but actually rustfix.

You see, the mut in mut i is highlighted, but the replacement is just for the binding name, not the mutability. (Why not the mutability? Because the mutability is covered by the next lint we trigger, unused_mut.) And rustfix will rightfully complain if we try to replace parts of the code that we have already replaced.

tl;dr We need to change the span in unused_variables to only cover the i

[11Takanori]

I'd like to take this issue.

[killercup]

Gladly! This seems to be the place where the lint gets triggered:

rust/src/librustc/middle/liveness.rs

Lines 1542 to 1562 in 6f721f5

	let suggest_underscore_msg = format!("consider using `_{}` instead",
	name);
	if is_assigned {
	self.ir.tcx
	.lint_node_note(lint::builtin::UNUSED_VARIABLES, id, sp,
	&format!("variable `{}` is assigned to, but never used",
	name),
	&suggest_underscore_msg);
	} else if name != "self" {
	let msg = format!("unused variable: `{}`", name);
	let mut err = self.ir.tcx
	.struct_span_lint_node(lint::builtin::UNUSED_VARIABLES, id, sp, &msg);
	if self.ir.variable_is_shorthand(var) {
	err.span_suggestion(sp, "try ignoring the field",
	format!("{}: _", name));
	} else {
	err.span_suggestion_short(sp, &suggest_underscore_msg,
	format!("_{}", name));
	}
	err.emit()
	}

and the sp parameter should be the span we care about. I'd probably try and see if it's a binding with a mutability specifier and try to get the sub-span of that. (No idea what the actual API for that is, though, sorry.)

[csmoe]

@11Takanori Do you need any help?
some tips: the sp points to mut var or var, and the index for Span is BytePos:

let mut var = 1;
    ^^^^^^^  = origin-sp
        ^^^  = sub-sp

So you can get the sub-span's beginning BytePos with length of name, then check the methods of Span to generate the sub-span with the bytepos.

[killercup]

@csmoe I'd prefer to somehow get the original span if possible, instead of creating new spans. Sadly, this is not as trivial. A short code dive revealed that the name we use here

rust/src/librustc/middle/liveness.rs

Lines 1530 to 1531 in 6f721f5

	let r = self.should_warn(var);
	if let Some(name) = r {

is from

rust/src/librustc/middle/liveness.rs

Line 1490 in 6f721f5

let name = self.ir.variable_name(var);

which calls

rust/src/librustc/middle/liveness.rs

Lines 330 to 337 in 6f721f5

	fn variable_name(&self, var: Variable) -> String {
	match self.var_kinds[var.get()] {
	Local(LocalInfo { name, .. }) | Arg(_, name) => {
	name.to_string()
	},
	CleanExit => "<clean-exit>".to_string()
	}
	}

so, our "variable name" might in fact be <clean-exit>! Not sure how deep you want to go here, but it might make sense to refactor this a bit.

We should also try to add an applicability marker to this lint (landed in #50204, you can see it in use in #50454).

[csmoe]

@killercup ~~~I tried your sub-span suggestion just now, it works.~~~

I'd prefer to somehow get the original span if possible, instead of creating new spans.

The sounds more plausible than deriving from mix-span, I'll explore more into your advice later since it's midnight.

[killercup]

Not necessarily something we need to fix in this exact issue but I just wanted to mention that we should probably also add a macro-check to this lint. For example, this code:

macro_rules! foo {
    ($x:ident) => {{ let $x = 42; }}
}

fn main() {
    foo!(y);
}

tells me this (playground):

warning: unused variable: `y`
 --> src/main.rs:2:26
  |
2 |     ($x:ident) => {{ let $x = 42; }}
  |                          ^^ help: consider using `_y` instead
...
6 |     foo!(y);
  |     -------- in this macro invocation
  |
  = note: #[warn(unused_variables)] on by default

This is somewhat confusing, but should also never be applied by rustfix as suggested.

[11Takanori]

Is this issue difficult for a beginner like me? It will take me time to understand scope we need to fix.

[killercup]

@11Takanori hard to say, I'm neither a regular contributor nor mentor for rustc. Other people did diagnostics adjustments as first contributions to the compiler. Maybe you and @csmoe can figure something out together?

cc @Manishearth and @oli-obk who both know more on this than I do

[csmoe]

@11Takanori Are you still here? If you are interested in this issue, this is the instruction:
All the unused_local-checking are initiated by check_local:

rust/src/librustc/middle/liveness.rs

Lines 1394 to 1404 in 8ff4b42

	fn check_local<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, local: &'tcx hir::Local) {
	match local.init {
	Some(_) => {
	this.warn_about_unused_or_dead_vars_in_pat(&local.pat);
	},
	None => {
	this.pat_bindings(&local.pat, |this, ln, var, sp, id| {
	this.warn_about_unused(sp, id, ln, var);
	})
	}
	}

So the issue is caused by the wrong span(sp) which represents a Pat, but what we want is the REAL name span.

---

rust/src/librustc/hir/mod.rs

Lines 1125 to 1129 in 8ff4b42

	/// Local represents a `let` statement, e.g., `let <pat>:<ty> = <expr>;`
	#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
	pub struct Local {
	pub pat: P<Pat>,
	pub ty: Option<P<Ty>>,

We can search all the info of a variable from a let statement(represented by Local), as indicated by the comment of struct Local, those info might live in <pat>. Finally, we get it, all the info are wrapped in the Spanned struct:

rust/src/librustc/hir/mod.rs

Line 877 in 8ff4b42

Binding(BindingAnnotation, NodeId, Spanned<Name>, Option<P<Pat>>),

rust/src/libsyntax/codemap.rs

Lines 51 to 54 in 8ff4b42

	pub struct Spanned<T> {
	pub node: T,
	pub span: Span,
	}

---

Now we can derive an approcah from the analysis above:

1. Add a method to get the span of the varibale. the simple_name already implements a way to unwrap variable name, so you can create a simple_span method, its definition is almost same as simple_name.
2. Correct the sp in check_local with the real span we get from step 1.
3. Clean up the remaining trival error.

Use ./x.py check in between iterations to ensure everything builds at each step. Then, when an error occurs, you can bisect to see why.
It seems your first PR to rustc, so https://forge.rust-lang.org/ and ./configure --help are highly recommended before coding, for example, ./configure --llvm-root=<path> and ./x.py test --test-args=ui can save your time.
Feel free to ping me if you have any problem. :)

---

We should also try to add an applicability marker to this lint.

We'll step to applicability after span lint is done.