Uh oh!
There was an error while loading. Please reload this page.
don't elide lifetimes in paths in librustc/ - #53816
Merged
bors merged 1 commit intoSep 30, 2018
Merged
Conversation
There was a problem hiding this comment.
I think I would prefer this as '_ since they're not used.
Contributor
There was a problem hiding this comment.
I too would prefer to use '_ — I like to know when the names are important. I also just don't like our convention of using name 'a, though I guess that's neither here nor there =)
Contributor
There was a problem hiding this comment.
Are we using in-band lifetimes here, or are these names in scope?
nikomatsakis
approved these changes
Aug 30, 2018
nikomatsakis
left a comment
Contributor
There was a problem hiding this comment.
Wow, this is awesome, and kind of unexpected. I think I would have preferred to see TyCtxt,'_, '_, '_>, but actually I could go either way on that point, and we could always tweak later (guided by the "single-use lifetime" lint).
nikomatsakis
commented
Aug 30, 2018
Contributor
@bors r+ I think I'm going to r+ this as is because "bitrot" |
bors
commented
Aug 30, 2018
Collaborator
📌 Commit c9a67858ecb9c686bfbbd7e2929e3e86e62bd571 has been approved by |
Contributor
There was a problem hiding this comment.
This exact function confuses me every time — I always think "how come there is no lifetime here?!"
bors
commented
Sep 1, 2018
Collaborator
⌛ Testing commit c9a67858ecb9c686bfbbd7e2929e3e86e62bd571 with merge 0c4c20043bfe7c2069c8cd5f4969449feb9cfc66... |
bors
commented
Sep 1, 2018
Collaborator
💔 Test failed - status-travis |
rust-highfive
commented
Sep 1, 2018
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
commented
Sep 1, 2018
Member
Missed one |
bors
commented
Sep 1, 2018
Collaborator
☔ The latest upstream changes (presumably #53884) made this pull request unmergeable. Please resolve the merge conflicts. |
Closed
BatmanAoD
commented
Sep 10, 2018
Contributor
Ping from triage, @zackmdavis! Any update on fixing the missing lifetime? |
zackmdavis
commented
Sep 10, 2018
ContributorAuthor
@BatmanAoD yes, thanks, I have a note on my to-do list to rebase this tonight. |
zackmdavis
commented
Sep 11, 2018
ContributorAuthor
or at least "soon" if not literally tonight (I hope I may be forgiven for the phenomenon where a task that only takes 10 minutes of wall time costs disproportionately more in subjective "activiation energy") |
zackmdavisforce-pushed
the
elided_lifetimes_in_paths_field_day
branch
from
September 14, 2018 05:03
c9a6785 to
54cc45cComparebors
commented
Sep 14, 2018
Collaborator
☔ The latest upstream changes (presumably #54032) made this pull request unmergeable. Please resolve the merge conflicts. |
zackmdavisforce-pushed
the
elided_lifetimes_in_paths_field_day
branch
from
September 14, 2018 14:43
54cc45c to
91a3cb1Comparerust-highfive
commented
Sep 14, 2018
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
commented
Sep 15, 2018
Collaborator
☔ The latest upstream changes (presumably #54151) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
commented
Sep 30, 2018
Collaborator
⌛ Testing commit 5b22d9b with merge 2bafac082a2573db4df4917533e419e54d7e63b9... |
bors
commented
Sep 30, 2018
Collaborator
💔 Test failed - status-travis |
rust-highfive
commented
Sep 30, 2018
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
zackmdavis
commented
Sep 30, 2018
ContributorAuthor
@bors retry |
bors
commented
Sep 30, 2018
Collaborator
bors added a commit
that referenced
this pull request
Sep 30, 2018
…, r=nikomatsakis don't elide lifetimes in paths in librustc/ In light of the "Apply to rustc" checkbox on #44524 and @nikomatsakis's [recent comment about regularly wanting visual indication of elided lifetimes in types](#44524 (comment)), I was curious to see what it would look like if we turned the `elided_lifetimes_in_path` lint on in at least one crate in the codebase (I chose librustc). Given that I couldn't figure out how to get `cargo fix` work with the build system, this arguably wasn't a very efficient use of my time, but once I started, the conjunction of moral law and the sunk cost fallacy forced me to continue. This is mostly applying the `<'_>` suggestions issued by the lint, but there were a few places where I named the lifetimes (_e.g._, `<'a, 'gcx, 'tcx>` on `TyCtxt`) in order to match style with surrounding code. r? @nikomatsakis
bors
commented
Sep 30, 2018
Collaborator
☀️ Test successful - status-appveyor, status-travis |
This was referenced Sep 30, 2018
JonathanBrouwer added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 10, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
JonathanBrouwer added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 10, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 10, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 11, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 11, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 11, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 11, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
jhpratt added a commit
to jhpratt/rust
that referenced
this pull request
Jun 11, 2026
…escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixesrust-lang#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor rust-lang#53816, and the `// FIXME(rust-lang#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
rust-timer added a commit
that referenced
this pull request
Jun 11, 2026
Rollup merge of #157280 - Dnreikronos:existential_trait_ref_escaping_bound_vars_assert, r=lcnr traits: Allow escaping self types in ExistentialTraitRef::with_self_ty Fixes#157122 WF-checking recurses through higher-ranked binders without instantiating them, so a `dyn Trait` nested inside a `for<'a>` binder reaches the `ty::Dynamic` arm of `WfPredicates::visit_ty` while still carrying escaping bound vars. Building the principal trait ref there via `ExistentialTraitRef::with_self_ty` hands that escaping self type to a `debug_assert!(!self_ty.has_escaping_bound_vars())`, which ICEs once the assertion is enabled. The assert was accidentally disabled in 2018 refactor #53816, and the `// FIXME(#157122)` left in its place asks whether to remove it or fix the fallout. An escaping bound var is expected for trait objects, and we already catch trait refs with escaping bound vars at the places that actually use them — so creating one here is fine. Rather than work around the assert in WF, this removes the assert and its stale FIXME from `with_self_ty`. Two regression tests pin the behavior: - `wf-dyn-in-hrtb-bound-issue-157122.rs` reproduces the original ICE (`dyn` nested in an HRTB bound). - `wf-dyn-in-hrtb-bound-const-mismatch.rs` checks that the `ConstArgHasType` obligation this arm reads off still fires, so an ill-typed const argument on such a `dyn` keeps erroring instead of compiling clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In light of the "Apply to rustc" checkbox on #44524 and @nikomatsakis's recent comment about regularly wanting visual indication of elided lifetimes in types, I was curious to see what it would look like if we turned the
elided_lifetimes_in_pathlint on in at least one crate in the codebase (I chose librustc). Given that I couldn't figure out how to getcargo fixwork with the build system, this arguably wasn't a very efficient use of my time, but once I started, the conjunction of moral law and the sunk cost fallacy forced me to continue.This is mostly applying the
<'_>suggestions issued by the lint, but there were a few places where I named the lifetimes (e.g.,<'a, 'gcx, 'tcx>onTyCtxt) in order to match style with surrounding code.r? @nikomatsakis