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.

Sign up for GitHub

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

Jump to bottom

Convert a hard-warning about named static lifetimes into a lint #98079

Closed
wants to merge 4 commits into from
Closed

Convert a hard-warning about named static lifetimes into a lint #98079

wants to merge 4 commits into from

Conversation

jeremydavis519
Copy link

Fixes #96956.

This change is pretty simple, but any feedback is appreciated.

Thanks to cjgillot for the clear instructions in the issue's thread. They made this so much easier. :)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 14, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

Perhaps a UNUSED_LIFETIMES lint can be reused instead?
It fits pretty well, and this case looks too niche for introducing a new separate lint.

petrochenkov
petrochenkov reviewed Jun 14, 2022
View reviewed changes
compiler/rustc_resolve/Cargo.toml Outdated
@@ -21,6 +21,7 @@ rustc_expand = { path = "../rustc_expand" }
rustc_feature = { path = "../rustc_feature" }
rustc_hir = { path = "../rustc_hir" }
rustc_index = { path = "../rustc_index" }
rustc_lint_defs = { path = "../rustc_lint_defs" }
Copy link
Contributor

Choose a reason for hiding this comment

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

rustc_resolve typically uses lints through rustc_session::lint, so adding this shouldn't be necessary.

@jeremydavis519
Copy link
Author

I agree that this is a very niche case, but I hesitate to lump this together with UNUSED_LIFETIMES because the lifetime isn't necessarily unused in this case. The problem is that it's unnecessary. And, maybe more importantly, this problem has a different solution: replacing the lifetime with 'static instead of just removing the lifetime.

@estebank
Copy link
Contributor

@jeremydavis519 I think @petrochenkov's proposal is to reuse the unused lifetimes name for the lint so that these two are effectively a "family" of errors in the same lint, the user visible output would have the same amount of context as it does today so people wouldn't be confused about what they should do. It would remove some granularity on allowing one and not the other, but that should be... rare.

estebank
estebank reviewed Jun 14, 2022
View reviewed changes
compiler/rustc_resolve/src/late/lifetimes.rs
Comment on lines +1268 to +1273
let help = &format!(
"you can use the `'static` lifetime directly, in place of `{}`",
lifetime.name.ident(),
);
lint.build(msg)
.help(help)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily something to do on this PR, but we have enough context to turn the help into a structured suggestion, by inspecting the bounds and suggesting:

LL -     type Y<'a: 'static>;
LL +     type Y<'static>;
   |            ~~~~~~~

@jeremydavis519
Copy link
Author

Alright, I don't mind grouping the lints together. My main concern is that I would be mildly confused if I saw #[allow(unused_lifetimes)] but all of the lifetimes were used (even if some were used unnecessarily). But I can't think of any situations where someone would actually selectively allow or deny this particular warning, so it probably doesn't matter.

@apiraino
Copy link
Contributor

What's the status here? I'll switch to waiting on author since I think there are pending comments. Feel free to update the status of this PR with @rustbot ready when ready, thanks!

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2022
@bors
Copy link
Contributor

bors commented Jul 25, 2022

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

@mkatychev
Copy link

@jeremydavis519 what's the timeline on moving forward with the proposal? I'd be happy to address feedback

@mkatychev
Copy link

@estebank PR contributor hasn't been active in this PR for over 2 months, is there a process for taking over the feature ask?

@estebank
Copy link
Contributor

@mkatychev you can pull from @jeremydavis519's branch, build upon it and publish a new PR referencing this one.

@JohnCSimon
Copy link
Member

@jeremydavis519
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Oct 2, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 2, 2022
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 3, 2022
@LittleFall LittleFall mentioned this pull request Feb 19, 2023
Merged
4 tasks
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 21, 2023
@compiler-errors
Rollup merge of rust-lang#108230 - LittleFall:enhance/warning, r=este…
f9d71d2 
…bank

Convert a hard-warning about named static lifetimes into lint "unused_lifetimes"

Fixes rust-lang#96956.

Some changes are ported from rust-lang#98079, thanks to jeremydavis519.

r? `@estebank` `@petrochenkov`

Any feedback is appreciated!

## Actions
- [x] resolve conflicts
- [x] fix build
- [x] address review comments in last pr
- [x] update tests
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 22, 2023
@GuillaumeGomez
Rollup merge of rust-lang#108230 - LittleFall:enhance/warning, r=este…
a32c500 
…bank

Convert a hard-warning about named static lifetimes into lint "unused_lifetimes"

Fixes rust-lang#96956.

Some changes are ported from rust-lang#98079, thanks to jeremydavis519.

r? `@estebank` `@petrochenkov`

Any feedback is appreciated!

## Actions
- [x] resolve conflicts
- [x] fix build
- [x] address review comments in last pr
- [x] update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@estebank estebank estebank left review comments

Assignees

@estebank estebank

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"warning: unnecessary lifetime parameter" unsilenceable lint