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 lint for unnecessary lifetime bounded &str return #13395

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

GnomedDev
Copy link
Contributor

Closes #305.

Currently implemented with a pretty strong limitation that it can only see the most basic implicit return, but this should be fixable by something with more time and brain energy than me. Cavets from #13388 apply, as I have not had a review on my clippy lints yet so am pretty new to this.

changelog: [`unnecessary_literal_bound`]:  Add lint for unnecessary lifetime bounded &str return. 

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 13, 2024
Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

I think maybe tcx.named_bound_var should work for this? I'm not sure whether this would be before inference or not, but if it is a Ty should give you all the info you need to generalize this to work outside of just &str. Regions are erased in the typeck results, however tcx.fn_sig() should still work.

Unfortunately, I don't think there would be any way to know if it's inferred nor can you get the span of a Ty easily. Might be annoying. My first idea to get around that is to recurse over both the HIR Ty and Ty at once but that seems incredibly finicky; however, the solution should be regions regardless of how it's implemented.

clippy_lints/src/unnecessary_literal_bound.rs Outdated Show resolved Hide resolved
@GnomedDev
Copy link
Contributor Author

I don't know how I would use named_bound_var and it seems out of scope for the idea/issue this is closing. I feel like this is letting perfect be the enemy of good, as a followup PR can generalize or improve this lint however it wants.

@Centri3
Copy link
Member

Centri3 commented Sep 16, 2024

Well, at the end of the day regions are practically lowered lifetimes (and scopes). It's not at all out of scope when the original issue cannot reliably be solved & generalized by parsing the source code alone, and by tracking lifetimes you're doing exactly what the compiler grants to you in such queries.

@Centri3
Copy link
Member

Centri3 commented Sep 16, 2024

Also now that I think about it, this could apply to b"thing" and c"thing" as well

@GnomedDev
Copy link
Contributor Author

GnomedDev commented Sep 16, 2024

Okay, I'll look into using named_bound_var and if I can't figure it out close this PR as it doesn't sound wanted with this implementation.

@Centri3
Copy link
Member

Centri3 commented Sep 16, 2024

Note, I'd definitely be fine with it being added if it's done this way, but it'll be harder to update + it'd be easier to rewrite it anyway

named_bound_var when used on TyKind::Ref's containing type's id practically just returns a Region, there's better documentation on the RegionKind

@GnomedDev
Copy link
Contributor Author

I have tried both

dbg!(cx.tcx.named_bound_var(ret_hir_ty.hir_id));

and

dbg!(cx.tcx.named_bound_var(inner_hir_ty.hir_id));

and they both always return none by the time we get past the "is str" check.

@xFrednet
Copy link
Member

@Centri3 since you already looked at this, do you want to take over the review?

@Centri3
Copy link
Member

Centri3 commented Sep 18, 2024

I have tried both

dbg!(cx.tcx.named_bound_var(ret_hir_ty.hir_id));

and

dbg!(cx.tcx.named_bound_var(inner_hir_ty.hir_id));

and they both always return none by the time we get past the "is str" check.

In specific I'm fairly sure you'll need the HirId of the LifeTime contained in ret_hir_ty.kind

@Centri3
Copy link
Member

Centri3 commented Sep 18, 2024

r? Centri3

@rustbot rustbot assigned Centri3 and unassigned xFrednet Sep 18, 2024
@Centri3
Copy link
Member

Centri3 commented Sep 18, 2024

Unfortunately looking closely at the implementation of it I don't know if named_bound_var will work. Maybe the fn_sig can be done, but probably not for now. I'll look for some way to deal with stuff like Result<(), &str>

@GnomedDev
Copy link
Contributor Author

How about we land this, then if you want you can improve it via lifetime stuff? As is, this doesn't add any false positives (to my knowledge) and is simply a little limited in what it lints (which is better than the not linting at all currently)

@bors
Copy link
Collaborator

bors commented Sep 22, 2024

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

@GnomedDev
Copy link
Contributor Author

Hey, @Centri3, just double checking you saw the comment above? Nearly been a month.

@Centri3
Copy link
Member

Centri3 commented Oct 12, 2024

I don't have the time at the moment unfortunately. I'll reroll
r? rust-lang/clippy

@rustbot rustbot assigned Manishearth and unassigned Centri3 Oct 12, 2024
@bors
Copy link
Collaborator

bors commented Oct 13, 2024

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

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

📌 Commit b62d262 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

⌛ Testing commit b62d262 with merge f328623...

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing f328623 to master...

@bors bors merged commit f328623 into rust-lang:master Oct 15, 2024
11 checks passed
@GnomedDev GnomedDev deleted the unnecessary-lit-bound branch October 15, 2024 16:49
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.

Idea: Unintentional borrowing with &str return.
7 participants