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

Stop errorring for elided lifetimes in path. #96957

Closed
wants to merge 2 commits into from

Conversation

cjgillot
Copy link
Contributor

This special case is used to ban elided lifetimes in path in async fns and impl block headers.
It does the same thing as the elided-lifetimes-in-paths lint, excepts as a hard error.

Having both flavours does not really bring anything but complexity.
This PR proposes to remove the special case.

This will allow to always use the CreateParameter mode for anonymous lifetimes in a later PR.

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

r? @wesleywiser

(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 May 11, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 20, 2022

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

@wesleywiser
Copy link
Member

@jackh726 would you mind taking a look at this? Thanks!

r? @jackh726

@@ -1,3 +1,4 @@
// check-pass
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that this change is only picked up in this test. I would have expected it to be picked up in a parsing or lowering test or something.

Regardless, this change will need a lang (probably) FCP. FWIW, I personally don't think allowing elided lifetimes in impl headers is the right move. It's 1) inconsistent with the general trend of not allow elided lifetime parameters and 2) unspecified on if it would be 'static (as is assumed today under error conditions or some new lifetime (which would be the case if you did '_.

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 also maybe a bit confused here. I guess the ELIDED_LIFETIMES_IN_PATHS lint is allow by default, but was erroring. Is that what you're cleaning up?

@cjgillot
Copy link
Contributor Author

ELIDED_LIFETIMES_IN_PATHS is an allow-by-default lint, but some cases (impl headers and async fns) are hardcoded to be a (slightly different) error instead. As I understood, ELIDED_LIFETIMES_IN_PATHS is not close to being warn-by-default any time soon: too much of mechanical changes each time a lifetime is added/removed somewhere.

The goal of this PR is to simplify the implementation by having only one flavour of error. If relaxing the behaviour is not desirable, I will attempt to refactor the code to only have one implementation and one message.

@jackh726
Copy link
Member

If relaxing the behaviour is not desirable

I think that is the case, yeah. We could poll the lang team, but given that the general trend seems to be more explicitness when there could be ambiguity (this error, dyn Trait instead of Trait, etc.), I would be surprised if this is direction we want to take.

It definitely does seem like there's some opportunity to clean this up though, even if we error or (maybe) lint depending on the context.

@cjgillot cjgillot closed this May 25, 2022
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants