Fix string_lit_as_bytes lint for macros

[yaahc]

Prior to this change, string_lit_as_bytes would trigger for constructs
like include_str!("filename").as_bytes() and would recommend fixing it
by rewriting as binclude_str!("filename").

This change updates the lint to act as an EarlyLintPass lint. It then
differentiates between string literals and macros that have bytes
yielding alternatives.

Closes #3205

[yaahc]

This is a WIP that doesn't work and I could use some help / mentoring on it. I moved the check in question to a new file because it seemed like failing an EarlyLintPass lint causes LateLintPass lints not to be run.

Also I'm not sure if im going about this entirely the wrong way. I expected to be able to use ExprKind::Mac to identify macros rather than string literals that precede a as_bytes() call, but all the macros seem to be expanded still even though its early_pass. I have a few things left to try still but I'm going to bed for now and thought I'd push what I've got. My current best guess is that I need to use something other than check_expr to get an actual pre macro check. Alternatively I'll probably look for other uses of ExprKind::Mac in the code base to see if I can't pinpoint where I'm messing up.

[yaahc]

For what its worth, the debug traces that I added, which are included in this initial commit, produced the following output

error: MethodCall(PathSegment { ident: as_bytes#0, args: None }, [expr(9: "hello there")])

error: calling `as_bytes()` on a string literal
 --> tests/ui/strings_early.rs:6:14
  |
6 |     let bs = "hello there".as_bytes();
  |              ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `b"hello there"`
  |
  = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings`

error: MethodCall(PathSegment { ident: as_bytes#0, args: None }, [expr(13: "☃")])

error: MethodCall(PathSegment { ident: as_bytes#0, args: None }, [expr(24: "foobar")])

error: calling `as_bytes()` on a string literal
  --> tests/ui/strings_early.rs:11:18
   |
11 |     let strify = stringify!(foobar).as_bytes();
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `bstringify!(foobar)`

error: MethodCall(PathSegment { ident: as_bytes#0, args: None }, [expr(25: "#![feature(tool_lints)]\n\n#![allow(unused, clippy::needless_
nsert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {\n    if !m.contains_key(&k) { m.insert(k, v); }\n}\n\nfn insert_if
absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {\n    if !m.contains_key(&k) { m.insert(k, v) } else { None };\n}\n\nfn inse
n insert_if_absent3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {\n    if !m.contains_key(&k) { foo(); m.insert(k, v) } else { No
 m.insert(k, v) };\n}\n\nfn insert_in_btreemap<K: Ord, V>(m: &mut BTreeMap<K, V>, k: K, v: V) {\n    if !m.contains_key(&k) { foo(); m.in
ns_key(&k) { m.insert(o, v); }\n}\n\nfn main() {\n}\n")])

error: calling `as_bytes()` on a string literal
  --> tests/ui/strings_early.rs:13:22
   |
13 |     let includestr = include_str!("entry.rs").as_bytes();
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `binclude_str!("entry.r

error: aborting due to 7 previous errors

I was hoping the stringify or include_str would indicate that it was a macro :/

[flip1995]

PreExpansionPass lints have to be registered here:
https://github.com/rust-lang-nursery/rust-clippy/blob/1be78b90ac60e0416c0289d39093d5f811e8057b/clippy_lints/src/lib.rs#L200-L206
and not in the register_plugins function. Just move it there and the macros should not get expanded anymore.

EarlyLintPass lints will always suppress LateLintPass lints (if they're deny), it doesn't matter where you define them: rust-lang/rust#36161. You can leave this code in strings.rs.

[yaahc]

re: moving it back to strings.rs. At the very least I think I'd still have to split up the strings.rs test case because there are still other late pass lints in there. What ended up happening was I moved the lint from late to early and it changed the number of lints in the stderr output from 11 to 2. Does it make more sense to have one string.rs clippy lint source file and two test case files or should I aim to keep it 1 to 1.

[flip1995]

Multiple test files are always better. We want to keep the size of the *.stderr files short. But for the code we want to keep things together, if they're doing similar things.

[yaahc]

K I think we're getting close, I'm getting a strange failure on run-pass/used_underscore_binding_macro.rs. Also should I try to add span_suggestion_with_applicability to this lint?

[flip1995]

No, we need to add Applicability levels to all occurrences of span_lint_and_sugg anyway at some point in the future (new version of #2943). One occurrence more or less doesn't make a difference.

No run_pass fail on travis.

strings.stderr needs an update

NIT: To make the life of the reviewer (me) easier, it would be great if you could add a new commit when you change something. It is hard to keep up with git commit -a --amend --no-edit && git push -f 😄 If necessary these commits can be squashed before merging.

[yaahc]

Why does this need the self::... path segment? is that what this is called? The type for e in the function args doesn't need it.

[flip1995]

The type in the function call just looks for the use statements of the file and can use them directly. use-statements on the other hand also need to be able to import stuff from outside the current file, so you have to specify where it should start looking.

• Crate level: crate::foo in 2018 edition, ::foo in 2015 edition
• Module level: self::foo
• Parent-module level: super::foo (parent-parent-module: super::super::foo, and so on)

If you just write use foo in the 2018 edition (extern crate got removed) it will look for a crate named foo, whereas use self::foo looks for a module/function/... inside the current module.

Playground

[ghost]

I found that a big problem with pre-expansion lints is that they won't lint code "passed through" macros.

Example:

use if_chain::if_chain;

fn main() {

    // Warns
    let _ = "test".as_bytes();

    if_chain! {
        if true;
        then {
            // Doesn't warn
            let _ = "test".as_bytes();
        }
    }
}

After this lint is changed to pre-expansion, it won't check inside the if_chain! macro. It can't see into the macro because it hasn't been expanded. I think we need to be very careful about making anymore pre-expansion lints. Maybe we should even revisit the other lints we converted.

For fixing this bug, I found a possible alternative. In the original code, snippet(cx, args[0].span.source_callsite(), "") it will return the source before expansion. You can check if that string starts with "include_str!" when you create the suggestion.

[flip1995]

Oh yeah, you're right, I never thought about that. The code inside the macro call isn't even parsed at the point where the PreExpansionPass lints get executed. We should limit PreExpansionPass to lints that really only look for a specific macro call.

Another option would be to split this up in an EarlyLintPass for the original lint and a PreExpansionPass for the include_str part. But that would mean, that the same lint needs to be run twice. Thinking about it, I like the source_callsite() variant better.

@yaahallo Could you make this change? Sorry for the back and forth.

[yaahc]

@flip1995 of course :D

[yaahc]

Also, I'm sorry this took so long :/ I had it mostly ready and just never set aside the time to finish the last 5% and push it.

[flip1995]

Also, I'm sorry this took so long :/ I had it mostly ready and just never set aside the time to finish the last 5% and push it.

No problem, we've all been there :D

I'll try to take a closer look at this in the next two days.

[flip1995]

Now I'm sorry this took so long :/ I took last week off and couldn't look over this last weekend.

Everything except the 4 comments I left LGTM

(Needs a rebase)

[phansch]

looks good to me, too!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push
• continuous-integration/appveyor/branch