Skip to content
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.

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

Lint string_lit_as_bytes should not be reported for string literal of length 33 and more #1208

Closed
antoyo opened this issue Sep 1, 2016 · 13 comments · Fixed by #4233
Closed
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types

Comments

@antoyo
Copy link

antoyo commented Sep 1, 2016

Hello.
The lint string_lit_as_bytes reports a warning even if the string literal has a length of 33 or more.
However, the AsRef<[T]> for array is only defined for array of length up to 32.

A code example triggering a false positive is here.
More specifically, it is this code:

    let request = tls_handshake.and_then(|socket| {
        tokio_core::io::write_all(socket, "\
            GET / HTTP/1.0\r\n\
            Host: www.rust-lang.org\r\n\
            \r\n\
        ".as_bytes())
    });

Since the string has a length of 43, the the trait bound '[u8; 43]: std::convert::AsRef<[u8]>' is not satisfied (compiler error), so we cannot use as_bytes().

I believe this issue happens only when a trait like AsRef<[T]> is used.

Thanks to fix this issue.

@mcarton
Copy link
Member

mcarton commented Sep 1, 2016

fn foo(a: &[u8]) {
    println!("{}", a.len());
}

fn main() {
    foo(b"azertyuiopqsdfghjklmwxcvbnazertyuiopqsdfghjklmwxcvbn");
}

prints 52.

@antoyo
Copy link
Author

antoyo commented Sep 1, 2016

@mcarton While the following:

fn foo<A: AsRef<[u8]>>(a: A) {
    println!("{}", a.as_ref().len());
}

fn main() {
    foo(b"azertyuiopqsdfghjklmwxcvbnazertyuiopqsdfghjklmwxcvbn");
}

gives a the above compiler error.

And using:

    foo("azertyuiopqsdfghjklmwxcvbnazertyuiopqsdfghjklmwxcvbn".as_bytes());

triggers the clippy warning.

@mcarton
Copy link
Member

mcarton commented Sep 1, 2016

Ah, I only looked at std::io::Write::write_all which takes &[u8].

@mcarton mcarton added C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types labels Sep 1, 2016
@antoyo
Copy link
Author

antoyo commented Sep 19, 2016

I am not sure it is related to the size of the string.
The following code produces the same issue:

let cstring = CString::new("Hello".as_bytes()).unwrap();

because using b"Hello" gives:

the trait bound `std::vec::Vec<u8>: std::convert::From<&[u8; 5]>` is not satisfied

@jens1o
Copy link
Contributor

jens1o commented Mar 8, 2019

I'm also affected by this problem. Is there somewhere I could start contributing to fix this?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 10, 2019

I believe all that is needed is an if lit_content.as_str().len() < 32 around

let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#);
let expanded = if let StrStyle::Raw(n) = style {
let term = (0..n).map(|_| '#').collect::<String>();
format!("r{0}\"{1}\"{0}", term, lit_content.as_str())
} else {
format!("\"{}\"", lit_content.as_str())
};
let mut applicability = Applicability::MachineApplicable;
if callsite.starts_with("include_str!") {
span_lint_and_sugg(
cx,
STRING_LIT_AS_BYTES,
e.span,
"calling `as_bytes()` on `include_str!(..)`",
"consider using `include_bytes!(..)` instead",
snippet_with_applicability(cx, args[0].span, r#""foo""#, &mut applicability).replacen(
"include_str",
"include_bytes",
1,
),
applicability,
);
} else if callsite == expanded
&& lit_content.as_str().chars().all(|c| c.is_ascii())
&& !in_macro(args[0].span)
{
span_lint_and_sugg(
cx,
STRING_LIT_AS_BYTES,
e.span,
"calling `as_bytes()` on a string literal",
"consider using a byte string literal instead",
format!(
"b{}",
snippet_with_applicability(cx, args[0].span, r#""foo""#, &mut applicability)
),
applicability,
);
}

Also a rustfix test ensuring that everything works correctly should be added so we don't regress this.

@oli-obk oli-obk added the good-first-issue These issues are a good way to get started with Clippy label Mar 10, 2019
@jens1o
Copy link
Contributor

jens1o commented Mar 10, 2019

I'd like to take this! :)

@jens1o
Copy link
Contributor

jens1o commented Mar 10, 2019

@oli-obk Before opening a PR: I wonder what you exactly mean with a rustfix test? I've removed the test expectations and any error that should be added should change the stderr, shouldn't it?

jens1o added a commit to jens1o/rust-clippy that referenced this issue Mar 10, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Mar 10, 2019

Adding //run-rustfix to the top of the .rs file, should automatically generate a new file with all suggestions applied. If the resulting file does not compile successfully, the test fails.

This allows us to detect cases like this one, since the suggested change doesn't compile.

@phansch
Copy link
Member

phansch commented Apr 19, 2019

@jens1o I believe you forgot to open a PR with your fix?

@jens1o
Copy link
Contributor

jens1o commented Apr 19, 2019

Yeah. I don't know whether there is missing something. If so, please, @phansch, take it over, as I'll be unavailable over the next week. :)

@thiagoarrais
Copy link
Contributor

@jens1o: are you working on this? I'm interested in porting this to master.

@flip1995
Copy link
Member

@thiagoarrais It seems you can take this issue. :)

thiagoarrais added a commit to thiagoarrais/rust-clippy that referenced this issue Jun 25, 2019
Port of @jens1o code ([b76f939][jens1o_commit])

Fixes rust-lang#1208

[jens1o_commit]: jens1o@b76f939

Co-authored-by: Thiago Arrais <[email protected]>
thiagoarrais added a commit to thiagoarrais/rust-clippy that referenced this issue Jul 8, 2019
Port of @jens1o code ([b76f939][jens1o_commit])

Fixes rust-lang#1208

[jens1o_commit]: jens1o@b76f939

Co-authored-by: Thiago Arrais <[email protected]>
bors added a commit that referenced this issue Jul 9, 2019
Avoid reporting string_lit_as_bytes for long strings

Port of @jens1o code ([b76f939][jens1o_commit])

Fixes #1208

[jens1o_commit]: jens1o@b76f939

<!--
Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only updates to the latest nightly, you can leave the
`changelog` entry as `none`. Otherwise, please write a short comment
explaining your change.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- [ ] Followed [lint naming conventions][lint_naming]
- [ ] Added passing UI tests (including committed `.stderr` file)
- [ ] `cargo test` passes locally
- [ ] Executed `util/dev update_lints`
- [ ] Added lint documentation
- [ ] Run `cargo fmt`

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR -->

changelog: bugfix for long strings as bytes
@bors bors closed this as completed in #4233 Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants