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

unnecessary_owned_empty_strings #8660

Merged
merged 2 commits into from Apr 11, 2022
Merged

unnecessary_owned_empty_strings #8660

merged 2 commits into from Apr 11, 2022

Conversation

yoav-lavi
Copy link
Contributor

@yoav-lavi yoav-lavi commented Apr 7, 2022

[unnecessary_owned_empty_strings]

Fixes #8650

  • Followed lint naming conventions
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

changelog: Adds unnecessary_owned_empty_strings, a lint that detects passing owned empty strings to a function expecting &str

@rust-highfive
Copy link

r? @giraffate

(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 Apr 7, 2022
@yoav-lavi yoav-lavi marked this pull request as draft April 7, 2022 18:45
@yoav-lavi
Copy link
Contributor Author

I need to do an additional check which does not currently run due to pulling from source and cargo being missing from the latest nightly, will try again tomorrow

@yoav-lavi
Copy link
Contributor Author

Seems like this currently does not catch "12345".split(&String::new()), I've update the example in the meantime but needs to be checked why that specific example doesn't work

@Jarcho
Copy link
Contributor

Jarcho commented Apr 7, 2022

I wouldn't try to handle str::split right away. It takes a generic type bound by Pattern which both str and String implement. It's doable, but a lot more involved.

@yoav-lavi
Copy link
Contributor Author

Got it, thanks for the tip @Jarcho!

@yoav-lavi yoav-lavi marked this pull request as ready for review April 7, 2022 20:09
@yoav-lavi
Copy link
Contributor Author

The discussion on Reddit brought up that you may want to use &String::new() for type consistency or due to the fact that &str is a fat pointer while &String isn't, but I'm not sure that distinction is relevant when discussing "" vs &String::new().

Making sure that these aren't issues that should prevent this lint from being added

@Jarcho
Copy link
Contributor

Jarcho commented Apr 7, 2022

Irrelevant in this case. This lint is for any time a &str is expected and &String::new() is given. Since &str is expected a fat pointer will be created anyways.

@ghost
Copy link

ghost commented Apr 8, 2022

Tested on a bunch of crates. 3 warnings in yaml-rust. Fix worked.

---> yaml-rust-0.4.5/src/scanner.rs:700:60                            
warning: detected a usage of `&String::new()` for a function expecting a `&str` argument
   --> src/scanner.rs:700:60
    |
700 |         let prefix = self.scan_tag_uri(true, is_secondary, &String::new(), mark)?;
    |                                                            ^^^^^^^^^^^^^^ help: try: `""`
    |
    = note: requested on the command line with `-W clippy::unnecessary-string-new`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_string_new


---> yaml-rust-0.4.5/src/scanner.rs:736:54                            
warning: detected a usage of `&String::new()` for a function expecting a `&str` argument
   --> src/scanner.rs:736:54
    |
736 |             suffix = self.scan_tag_uri(false, false, &String::new(), &start_mark)?;
    |                                                      ^^^^^^^^^^^^^^ help: try: `""`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_string_new


---> yaml-rust-0.4.5/src/scanner.rs:754:62                            
warning: detected a usage of `&String::new()` for a function expecting a `&str` argument
   --> src/scanner.rs:754:62
    |
754 |                 suffix = self.scan_tag_uri(false, secondary, &String::new(), &start_mark)?;
    |                                                              ^^^^^^^^^^^^^^ help: try: `""`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_string_new

clippy_lints/src/unnecessary_string_new.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_string_new.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_string_new.rs Outdated Show resolved Hide resolved
@yoav-lavi yoav-lavi changed the title unnecessary_string_new unnecessary_owned_empty_string Apr 8, 2022
@yoav-lavi yoav-lavi requested a review from flip1995 April 8, 2022 10:43
@yoav-lavi
Copy link
Contributor Author

r? @flip1995

@rust-highfive rust-highfive assigned flip1995 and unassigned giraffate Apr 8, 2022
rust-toolchain Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks, impl LGTM, just 2 more NITs

clippy_lints/src/unnecessary_owned_empty_string.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_owned_empty_string.rs Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks, please squash your commits, so we can merge this 🚀

@yoav-lavi
Copy link
Contributor Author

Thanks, please squash your commits, so we can merge this 🚀

Done

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

📌 Commit a4d1837 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

⌛ Testing commit a4d1837 with merge 53c9f68...

bors added a commit that referenced this pull request Apr 11, 2022
`unnecessary_owned_empty_string`

[`unnecessary_owned_empty_string`]

Fixes #8650

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

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

changelog: Adds `unnecessary_owned_empty_string`, a lint that detects passing owned empty strings to a function expecting `&str`
@flip1995
Copy link
Member

@bors r- retry

/// vec!["1", "2", "3"].join("");
/// ```
#[clippy::version = "1.62.0"]
pub UNNECESSARY_OWNED_EMPTY_STRING,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub UNNECESSARY_OWNED_EMPTY_STRING,
pub UNNECESSARY_OWNED_EMPTY_STRINGS,

Lint names are usually better in the plural form, when they aren't describing a specific function name. This usually reads better in combination with allow.

This change will require cargo dev update_lints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yoav-lavi yoav-lavi changed the title unnecessary_owned_empty_string unnecessary_owned_empty_strings Apr 11, 2022
@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

📌 Commit 1020137 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

⌛ Testing commit 1020137 with merge 5c19ae9...

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 5c19ae9 to master...

@bors bors merged commit 5c19ae9 into rust-lang:master Apr 11, 2022
@yoav-lavi yoav-lavi deleted the squashed-master branch April 12, 2022 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary_string_new
6 participants