Skip to content

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 5, 2020

Between #1402 (regarding to_owned) and #4494 (regarding impl Read), as well as other confusion I've seen hit in my work codebase involving string_lit_as_bytes ("...".as_bytes().into()), I don't think this lint is at a quality to be enabled by default.

I would consider re-enabling this lint after it is updated to understand when the surrounding type information is sufficient to unsize b"..." to &[u8] without causing a type error.

As currently implemented, this lint is pushing people to write &b"_"[..] which is not an improvement over "_".as_bytes() as far as I am concerned.


changelog: Remove string_lit_as_bytes from default set of enabled lints

@rust-highfive
Copy link

r? @matthiaskrgr

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 5, 2020
Copy link
Member Author

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failure appears unrelated.

error[E0061]: this function takes 2 arguments but 3 arguments were supplied
  --> clippy_lints/src/redundant_clone.rs:89:14
   |
89 |             .into_engine(cx.tcx, mir, def_id.to_def_id())
   |              ^^^^^^^^^^^ ------  ---  ------------------ supplied 3 arguments
   |              |
   |              expected 2 arguments

@dtolnay dtolnay force-pushed the string_lit_as_bytes branch from 5e96daf to a5ef305 Compare October 7, 2020 02:44
@dtolnay
Copy link
Member Author

dtolnay commented Oct 7, 2020

@dtolnay
Copy link
Member Author

dtolnay commented Oct 7, 2020

Passed!

@ebroto ebroto assigned ebroto and unassigned matthiaskrgr Oct 8, 2020
Copy link
Contributor

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the suggestion is broken so it seems fair to downgrade it.

Could you please add a small note in the Known problems section explaining why it's in the nursery?

@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 8, 2020
@dtolnay
Copy link
Member Author

dtolnay commented Oct 8, 2020

Added documentation of known problems.


Known Problems:

"str".as_bytes() and the suggested replacement of b"str" are not equivalent because they have different types. The former is &[u8] while the latter is &[u8; 3]. That means in general they will have a different set of methods and different trait implementations.

fn f(v: Vec<u8>) {}

f("...".as_bytes().to_owned()); // works
f(b"...".to_owned()); // does not work, because arg is [u8; 3] not Vec<u8>

fn g(r: impl std::io::Read) {}

g("...".as_bytes()); // works
g(b"..."); // does not work

The actual equivalent of "str".as_bytes() with the same type is not b"str" but &b"str"[..], which is a great deal of punctuation and not more readable than a function call.

@ebroto
Copy link
Contributor

ebroto commented Oct 8, 2020

@bors r+

Thanks for the nice explanation of the problem

@bors
Copy link
Contributor

bors commented Oct 8, 2020

📌 Commit c81bea4 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Oct 8, 2020

⌛ Testing commit c81bea4 with merge 265e484...

@bors
Copy link
Contributor

bors commented Oct 8, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 265e484 to master...

@bors bors merged commit 265e484 into rust-lang:master Oct 8, 2020
@dtolnay dtolnay deleted the string_lit_as_bytes branch October 8, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants