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

Remove in band lifetimes #93845

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 10, 2022

As discussed in t-lang backlog bonanza, the in_band_lifetimes FCP closed in favor for the feature not being stabilized. This PR removes #![feature(in_band_lifetimes)] in its entirety.

Let me know if this PR is too hasty, and if we should instead do something intermediate for deprecate the feature first.

r? @scottmcm (or feel free to reassign, just saw your last comment on #44524)
Closes #44524

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2022
@compiler-errors compiler-errors changed the title In band lifetimes Remove in band lifetimes Feb 10, 2022
@@ -684,7 +680,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
// that collisions are ok here and this shouldn't
// really show up for end-user.
let (str_name, kind) = match hir_name {
ParamName::Plain(ident) => (ident.name, hir::LifetimeParamKind::InBand),
ParamName::Plain(ident) => (ident.name, hir::LifetimeParamKind::Explicit),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only suspicious change.

Copy link
Member Author

@compiler-errors compiler-errors Feb 10, 2022

Choose a reason for hiding this comment

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

Async blocks, for example, declare named elided lifetimes (idk if that's the only one, perhaps closures?). We should give them a category here -- not sure if it's correct to use LifetimeParamKind::Explicit though. Maybe I can add a new LifetimeParamKind like Implicit.

It all funnels into hir::LifetimeDefOrigin::ExplicitOrElided though, so I don't think the distinction matters?

Copy link
Contributor

Choose a reason for hiding this comment

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

This case should only happen when synthetizing lifetimes for an async fn return impl Future type. There should not be a difference, but we got bit before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once LifetimeDefOrigin is removed, LifetimeParamKind is only used in lifetime resolution diagnostics and rustdoc to filter elided lifetimes. Therefore LifetimeParamKind::Explicit is fine.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

Thank you CI -- I'll add those error codes back then!

@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 Feb 10, 2022
@@ -141,13 +141,9 @@ struct LoweringContext<'a, 'hir: 'a> {
/// indicate whether or not we're in a place where new lifetimes will result
/// in in-band lifetime definitions, such a function or an impl header,
/// including implicit lifetimes from `impl_header_lifetime_elision`.
is_collecting_in_band_lifetimes: bool,
is_collecting_anonymous_lifetimes: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed this to anonymous, since this handles both elided and in-band lifetimes, but we now only have the former. Open to calling this something like is_collecting_elided_lifetimes tho!

@compiler-errors compiler-errors force-pushed the in-band-lifetimes branch 2 times, most recently from b6dc969 to 7b9d78a Compare February 10, 2022 04:10
@compiler-errors
Copy link
Member Author

I think this is ready for.. at least an initial review

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2022
@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

scottmcm commented Feb 10, 2022

Thanks for making the PR so quickly! ❤️

I'm not a compiler reviewer, though, so since you mention there are few nuanced bits here, it's probably better than someone who knows it better takes a look.

r? rust-lang/compiler

I could probably review some pieces, if you felt like splitting it up. (It seems like there are some easy bits, like updating tests that aren't about in-band to stop using it. Or maybe removing the special "do you want to try in-band?" hint would be a pure removal.) But since this is almost entirely red, it's not clear to me that it would be worth the effort of splitting. Hopefully the compiler reviewer will just be able to skim it and go "yup, seems fine".

@cjgillot cjgillot self-assigned this Feb 10, 2022
@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors force-pushed the in-band-lifetimes branch 2 times, most recently from e4c51b8 to 9e53b97 Compare February 10, 2022 16:24
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Take a look at some other error explanations not emitted by the compiler anymore to see how they handle the code examples (spoiler alert: use ignore 😉 ).

@bors
Copy link
Contributor

bors commented Feb 24, 2022

📌 Commit 43dbd83 has been approved by cjgillot

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2022
…r=cjgillot

Remove in band lifetimes

As discussed in t-lang backlog bonanza, the `in_band_lifetimes` FCP closed in favor for the feature not being stabilized. This PR removes `#![feature(in_band_lifetimes)]` in its entirety.

Let me know if this PR is too hasty, and if we should instead do something intermediate for deprecate the feature first.

r? `@scottmcm` (or feel free to reassign, just saw your last comment on rust-lang#44524)
Closes rust-lang#44524
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…r=cjgillot

Remove in band lifetimes

As discussed in t-lang backlog bonanza, the `in_band_lifetimes` FCP closed in favor for the feature not being stabilized. This PR removes `#![feature(in_band_lifetimes)]` in its entirety.

Let me know if this PR is too hasty, and if we should instead do something intermediate for deprecate the feature first.

r? ``@scottmcm`` (or feel free to reassign, just saw your last comment on rust-lang#44524)
Closes rust-lang#44524
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 24, 2022
…r=cjgillot

Remove in band lifetimes

As discussed in t-lang backlog bonanza, the `in_band_lifetimes` FCP closed in favor for the feature not being stabilized. This PR removes `#![feature(in_band_lifetimes)]` in its entirety.

Let me know if this PR is too hasty, and if we should instead do something intermediate for deprecate the feature first.

r? ```@scottmcm``` (or feel free to reassign, just saw your last comment on rust-lang#44524)
Closes rust-lang#44524
@Dylan-DPC
Copy link
Member

Dylan-DPC commented Feb 24, 2022

failed in #94328

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2022
@compiler-errors
Copy link
Member Author

@Dylan-DPC wrong PR referenced?

@Dylan-DPC
Copy link
Member

ye fixed now :P

@compiler-errors
Copy link
Member Author

I'll re-bless when I get back to my computer.

@compiler-errors
Copy link
Member Author

Had to bless tests with --compare-mode=nll. That did the trick.

when you have the free time, @cjgillot can you r+ again? thank you!

@Mark-Simulacrum
Copy link
Member

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Feb 25, 2022

📌 Commit 9386ea9 has been approved by cjgillot

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#93845 (Remove in band lifetimes)
 - rust-lang#94155 (Extend toggle GUI test a bit)
 - rust-lang#94252 (don't special case `DefKind::Ctor` in encoding)
 - rust-lang#94305 (Remove an unnecessary restriction in `dest_prop`)
 - rust-lang#94343 (Miri fn ptr check: don't use conservative null check)
 - rust-lang#94344 (diagnostic: suggest parens when users want logical ops, but get closures)
 - rust-lang#94352 (Fix SGX docs build)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ec4fc72 into rust-lang:master Feb 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 25, 2022
@compiler-errors compiler-errors deleted the in-band-lifetimes branch April 7, 2022 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Tracking issue for RFC 2115: In-band lifetime bindings