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

Add new lint string_as_str #9624

Closed
wants to merge 1 commit into from
Closed

Add new lint string_as_str #9624

wants to merge 1 commit into from

Conversation

guerinoni
Copy link
Contributor

@guerinoni guerinoni commented Oct 10, 2022

Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
changelog: none. Otherwise, please write a short comment
explaining your change.

It's also helpful for us that the lint name is put within backticks (` `),
and then encapsulated by square brackets ([]), for example:

changelog: [`string_as_str`]: Add new lint

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
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev 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.


Please write a short comment explaining your change (or "none" for internal only changes)

changelog: Add new lint [string_as_str]

Closes #874

@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 Oct 10, 2022
@guerinoni
Copy link
Contributor Author

Thanks to @flip1995 for some hints :)

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.

Some suggestions on how to clean up the lint code.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/string_as_str.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/string_as_str.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/string_as_str.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/internal_lints.rs Outdated Show resolved Hide resolved
Comment on lines 18 to 19
LL | let a = s.as_str().chars().map(|c| c.is_digit(2)).collect::<Vec<_>>();
| ^^^^^^^^^^ help: replace it with: `&s`
Copy link
Member

Choose a reason for hiding this comment

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

This suggestion won't work. It should suggest (&s). You can use clippy_utils::Sugg with maybe_paren to automatically produce the parens.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should lint where parenthesis are needed. This feels like the canonical use of as_str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid to lint this

)
}

fn is_within_borrowed_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this function is supposed to do. It looks like it checks for really specific things in the tests you've written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know how to not lint here

Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be linted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because using &snip there are different type on those two branch, one is &str and other &String, don't know if we can change the code in other way

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a blocking issue since it's not a machine applicable lint.

This seems like a wrong inference from rustc because explicit typing fixes the issue

let a: Option<&str> = match snip.split_once(" as ") {
        None => Some(&snip),
        Some((import, rename)) => {
            if rename.trim() == name {
                None
            } else {
                Some(import.trim())
            }
        },
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly... Don't sure next step to complete this lint :( 🤔

clippy_lints/src/methods/string_as_str.rs Outdated Show resolved Hide resolved
@guerinoni guerinoni requested review from flip1995 and removed request for giraffate October 15, 2022 12:37
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.

Took a closer look at this lint. The suggestion of changing this to &s only applies when the String can be coerced to str. This is the case in calls where the argument is of type &str. In every other place the suggestion has to be &* I can see two paths forward here:

  1. Check if the expression parent is some kind of call and the parameter of that function/method/closure has type &str
  2. Just lint everything and suggest &*. But in this case, this lint should be restriction IMO, because I don't think that &*s is better than s.as_str(). Even for a pedantic lint, I think this is a questionable suggestion.

@bors
Copy link
Contributor

bors commented Oct 16, 2022

☔ The latest upstream changes (presumably #9566) made this pull request unmergeable. Please resolve the merge conflicts.

@guerinoni
Copy link
Contributor Author

@flip1995 Do you have any suggestion for checking parameter of fn, I prefer to implement the option 1 because the impact can be more useful for a codebase

@flip1995
Copy link
Member

I think we have a expr_fn_sig utils function, that you can use to get the method/function signature and with that the type of the argument.

@bors
Copy link
Contributor

bors commented Oct 25, 2022

☔ The latest upstream changes (presumably #9667) made this pull request unmergeable. Please resolve the merge conflicts.

@guerinoni guerinoni closed this Nov 8, 2022
@guerinoni guerinoni deleted the string-as-str branch November 8, 2022 09:19
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.

lint String::as_str()
6 participants