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

False Positive: needless_raw_string_hashes #11402

Closed
FintanH opened this issue Aug 25, 2023 · 9 comments
Closed

False Positive: needless_raw_string_hashes #11402

FintanH opened this issue Aug 25, 2023 · 9 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@FintanH
Copy link

FintanH commented Aug 25, 2023

Summary

On our radicle-surf project, the lint job failed due to this clippy warning. This is the job and the offending code is:

#[test]
fn test_both_missing_eof_newline() {
    let buf = r#"
diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
\ No newline at end of file
+hello=1234
\ No newline at end of file
"#;
    let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
    let diff = Diff::try_from(diff).unwrap();
    assert_eq!(
        diff.modified().next().unwrap().diff.eof(),
        Some(EofNewLine::BothMissing)
    );
}

What's strange is there is a very similar test below that does not set off the clippy warning:

#[test]
fn test_none_missing_eof_newline() {
    let buf = r#"
diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
+hello=1234
"#;
    let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
    let diff = Diff::try_from(diff).unwrap();
    assert_eq!(
        diff.modified().next().unwrap().diff.eof(),
        Some(EofNewLine::NoneMissing)
    );
}

Lint Name

needless_raw_string_hashes

Reproducer

I tried this code:

#[test]
fn test_both_missing_eof_newline() {
    let buf = r#"
diff --git a/.env b/.env
index f89e4c0..7c56eb7 100644
--- a/.env
+++ b/.env
@@ -1 +1 @@
-hello=123
\ No newline at end of file
+hello=1234
\ No newline at end of file
"#;
    let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
    let diff = Diff::try_from(diff).unwrap();
    assert_eq!(
        diff.modified().next().unwrap().diff.eof(),
        Some(EofNewLine::BothMissing)
    );
}

I saw this happen:

error: unnecessary hashes around raw string literal
   --> radicle-surf/t/src/diff.rs:363:15
    |
363 |       let buf = r#"
    |  _______________^
364 | | diff --git a/.env b/.env
365 | | index f89e4c0..7c56eb7 100644
366 | | --- a/.env
...   |
372 | | \ No newline at end of file
373 | | "#;
    | |__^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes
    = note: `-D clippy::needless-raw-string-hashes` implied by `-D warnings`
help: try
    |
363 ~     let buf = r"
364 + diff --git a/.env b/.env
365 + index f89e4c0..7c56eb7 100644
366 + --- a/.env
367 + +++ b/.env
368 + @@ -1 +1 @@
369 + -hello=123
370 + \ No newline at end of file
371 + +hello=1234
372 + \ No newline at end of file
373 ~ ";
    |

error: could not compile `radicle-surf-test` (lib test) due to previous error

I expected for the lint job to pass. It can be reproduced in the git repository by running scripts/ci/lint.

Version

rustc 1.72.0 (5680fa18f 2023-08-23)
binary: rustc
commit-hash: 5680fa18feaa87f3ff04063800aec256c3d4b4be
commit-date: 2023-08-23
host: x86_64-unknown-linux-gnu
release: 1.72.0
LLVM version: 16.0.5

Additional Labels

No response

@FintanH FintanH added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Aug 25, 2023
FintanH added a commit to radicle-dev/radicle-git that referenced this issue Aug 25, 2023
Update the toolchain to rust-1.72.0. This brought with it a new clippy
check for needless '#'s for string literals. However, this is a false
positive and is being tracked here[[0]].

[0]: rust-lang/rust-clippy#11402

Signed-off-by: Fintan Halpenny <[email protected]>
X-Clacks-Overhead: GNU Terry Pratchett
@cemoktra
Copy link

cemoktra commented Aug 25, 2023

Another false positive is: : r#"escaped single quote \' is false positive"#:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=521e929a73e99c463e92216af74f58c9

@Centri3
Copy link
Member

Centri3 commented Aug 25, 2023

I fail to see how either of these are false positives. You can remove the hashes just fine.

needless_raw_string_hashes concerns the number of hashes, not the raw string itself.

The test that doesn't emit anything looks like a FN.

@cemoktra
Copy link

I fail to see how either of these are false positives. You can remove the hashes just fine.

needless_raw_string_hashes concerns the number of hashes, not the raw string itself.

The test that doesn't emit anything looks like a FN.

No. Removing the hashes caused unit tests to start failing. So something is changed in the string

@y21
Copy link
Member

y21 commented Aug 27, 2023

I'm also not entirely sure what the problem here is. That suggestion looks good to me, and the snippet it suggests also compares equal to your original string, so I don't know how that caused tests to fail.

fn main() {
    let a = r#"escaped single quote \' is false positive"#;
    let b = r"escaped single quote \' is false positive"; // clippy's suggestion
    dbg!(a == b);
}
[src/main.rs:4] a == b = true

@Centri3
Copy link
Member

Centri3 commented Aug 27, 2023

Removing the hashes should only change the string if there is a " with the amount of suggested hashes following it (or more), but would also be a compile error rather than a test failure. So can you give an example of a test that fails? Because the demonstrative examples work perfectly fine for me.

@cemoktra
Copy link

We are parsing the string in this test, more i can't say. It seems to make a difference. and i just checked, the string also contains double quotes

@FintanH
Copy link
Author

FintanH commented Aug 28, 2023

I fail to see how either of these are false positives. You can remove the hashes just fine.

needless_raw_string_hashes concerns the number of hashes, not the raw string itself.

The test that doesn't emit anything looks like a FN.

I see, I didn't realise that the #'s weren't needed in my examples above. I guess I'm reporting a false negative then :)

FintanH added a commit to radicle-dev/radicle-git that referenced this issue Aug 30, 2023
Update the toolchain to rust-1.72.0. This brought with it a new clippy
check for needless '#'s for string literals. However, this is a false
positive and is being tracked here[[0]].

[0]: rust-lang/rust-clippy#11402

Signed-off-by: Fintan Halpenny <[email protected]>
X-Clacks-Overhead: GNU Terry Pratchett
bors added a commit that referenced this issue Sep 28, 2023
… r=flip1995

Move `needless_raw_string_hashes` to `pedantic`

IMO it doesn't improve code enough to be warn by default. [It seems to be unclear to some also](#11402), but that can probably be remedied separately

changelog: Moved [`needless_raw_string_hashes`] to `pedantic` (Now allow-by-default)
[#11415](#11415)

r? `@flip1995`
@torfsen
Copy link
Contributor

torfsen commented Dec 27, 2023

@FintanH: I've tried reproducing this, but so far couldn't. For example, this reduced playground snippet with your two test cases produces the needless_raw_string_hashes for both, as expected.

Do you have a minimal, standalone example that shows the issue?

@FintanH
Copy link
Author

FintanH commented Dec 29, 2023

@FintanH: I've tried reproducing this, but so far couldn't. For example, this reduced playground snippet with your two test cases produces the needless_raw_string_hashes for both, as expected.

Do you have a minimal, standalone example that shows the issue?

Well I did report it with 1.72 whereas that playground is running 1.75, so maybe that's playing a factor? :)

Judging from the conversation, it seems like the fix was to remove the # anyway, but I guess it wasn't clear why some cases passed and others didn't. I'm happy to consider this as resolved if it can't be reproduced.

@Alexendoo Alexendoo closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

6 participants