lint against using a binding whose name is prefixed with an underscore

[oli-obk]

Prefixing an underscore to a binding is done to silence unused warnings. This makes it easy to recognize explicitly unused bindings by their name, and still have a name that tells you something about the binding. In case you start using the binding, you should remove the underscore.

[Manishearth]

I like this idea, but I don't know how to do it without duplicating liveness. Perhaps exprusevisitor.

[oli-obk]

Maybe there's another way. Look for ExprPath where the name starts with an underscore?

[Manishearth]

I'm an idiot. Thanks.

[oli-obk]

glad I'm not alone ;)

[llogiq]

Welcome to the club. 😜

[devonhollowood]

I could do this if someone is willing to mentor.

[oli-obk]

@devonhollowood I'll try. What do you need? I'm reachable on irc if you need real-time-feedback

For variables you probably just need an EarlyLintPass that implements check_expr and in case the Expr is a path or a struct field indexing, checks whether the last element of that path starts with an underscore.

[Manishearth]

Or a local variable.

You can do these checks with the def_map that comes with the cx.tcx. But it needs to be a LateLintPass (stick it in misc.rs I guess). Feel free to ping me if you need more help!

[oli-obk]

do we need the definition? Maybe for ensuring that the error is only reported once?

[Manishearth]

No, for ensuring that it is indeed a variable, not a function or something 😄

[devonhollowood]

Okay so I'm finally getting a chance to dig into this (work suddenly got very busy over the past couple days). I'm a little confused by the discussion here--is my understanding (below) correct?

• This lint should be implemented as a LateLintPass, which would implement check_expr
• Lint if both of the following are true:
  • The Expr's node is an ExprField, an ExprTupField, an ExprIndex, or an ExprPath, and the last element of that path is starts with an underscore
  • The LateContext's tcx's def_map's PathResolutions's base_def is a DefLocal

[Manishearth]

Well, more like:

• ExprPath, with DefLocal
• ExprField, with no def checking

ExprTupField and ExprIndex don't have paths that can start with an underscore

the rest is correct.

[devonhollowood]

Here's what I've got so far. This seems to pick up a lot of false positives though, so I must be doing something wrong. For example, it lints all of the following:

<std macros>:3 print ! ( concat ! ( $ fmt , "\n" ) , $ ( $ arg ) * ) ) ;
                         ^~~~~~~~~~~~~~~~~~~~~~~~~

tests/compile-fail/while_loop.rs:54         println!("{}", x);
                                                           ^

<std macros>:2 $ crate:: io:: _print ( format_args ! ( $ ( $ arg ) * ) ) ) ;
               ^~~~~~~~~~~~~~~~~~~~~

I think the name.as_str().chars().next() == Some('_') checks must be failing, but I'm not sure why that would be.

[llogiq]

This could certainly use a macro check. Look at utils::in_macro

[devonhollowood]

Okay, so I think you were right @llogiq in that the issue was in the macros themselves. I tried printing out the matched Paths, and they were respectively __STATIC_FMTSTR, __arg0, and ::std::io::_print. However, I don't think we want to just get rid of everything in a macro, which is what utils::in_macro seems to guard against. For example, we want to lint

fn foo(_x: Foo) {
    println!("{}", _x);
}

because that is using an underscore-prefixed variable. So how would one check that that the definition of _x is not in a macro? I've been digging around the Def documentation for a while now and I can't figure out how to get the span of a definition, otherwise I would just use in_macro.

[Manishearth]

Well, just check that the ident for _x does not have the same name and hygenic name. This will ensure that it is a user-defined ident, not a macro-defined ident. Neat trick 😄

(might not work, not sure how the new hygiene stuff works)

[devonhollowood]

@Manishearth I went ahead and added that, but it doesn't seem to work as expected. For example, it will still lint this:

tests/compile-fail/box_vec.rs:7     println!("{:?}", foo.get(0))                
                                                     ^~~~~~~~~~

(it's matching the __arg0 from the println!() macro)

[devonhollowood]

I'm thinking it might make sense to not lint in the case of >1 leading underscore, since I think that has a different traditional meaning than a single leading underscore. I tried out only linting for cases with exactly one leading underscore, and it seems to fix the built-in macros problem. If you're okay with that solution, I'll run dogfood, clean up any offending code code and submit the pull request.

[Manishearth]

in the case of >1 leading underscore, since I think that has a different traditional meaning than a single leading underscore

Usually used to mark private things which are public due to unavoidable reasons, but not always the case.

[untitaker]

I don't understand why this lint was added. What am I supposed to use instead for lock guards that I don't call any methods on or pass around? The wiki doesn't mention this new lint, only the README does.

[Manishearth]

What am I supposed to use instead for lock guards that I don't call any methods on or pass around?

This doesn't affect that?

This is when you use a variable tagged _something, not create one, i.e. when naming it something would NOT have triggered the unused_variables lint (which makes the underscore unnecessary and misleading).

An unused lock guard is fine. If it didn't have the underscore, it would have triggered unused_variables, so with the underscore it goes right past this lint.

(We don't immediately update the wiki)

[untitaker]

The errors are indeed not for the guard code I've written (my bad), but the code generated by #[derive(RustcDecodable)]. See https://travis-ci.org/untitaker/mysteryshack/jobs/97893563#L305

[Manishearth]

(fixed, needed an external macro check)

[llogiq]

The lint still gets triggered on btreemap!(..). I think we'll need a better macro check. Or better, make it an early lint pass (pre expansion).

[Manishearth]

Early lints passes are pre desugaring, but macros are still expanded. We could add another "veryearlylintpass" to the compiler for pre-expansion since the ast is the same.

[llogiq]

OK, that probably makes sense. Otherwise, we could create a check to see if some span originates from the macro expansion or the expanded code (if that is somehow possible).

[torfsen]

As far as I can see, this lint has been updated a few times since the last comments. In particular, it now has tests that make sure that code generated by macros is not linted.

In 0.0.69, used_underscore_binding was made allow-by-default in 2a5416d without further explanation (that later resulted in the lint being put into the pedantic category when categories were introduced). Assuming that all outstanding issues with the lint have been fixed I think suspicious would be a better place, because in my opinion it catches a definite code smell.

[blyxyas]

Shouldn't this issue be closed? Is there something else to handle related directly to this issue?

[Manishearth]

Yeah, we should close this with a separate issue opened about making this non-pedantic if people care about that.