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

Fix elided lifetimes shown as '_ on async functions #80319

Merged
merged 2 commits into from
Dec 25, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 23, 2020

Closes #63037.

r? @tmandry on the implementation, @Darksonn on the test cases.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-lifetimes Area: lifetime related A-async-await Area: Async & Await labels Dec 23, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 23, 2020

impl Foo {
// @has async_fn/struct.Foo.html
// @has - '//h4[@class="method"]' 'pub async fn complicated_lifetimes( &self, context: &impl Bar) -> impl Iterator<Item = &usize>'
Copy link
Member Author

Choose a reason for hiding this comment

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

The test suite normalizes this really strangely; the original is
image

@rust-log-analyzer

This comment has been minimized.

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/mod.rs Show resolved Hide resolved
src/test/rustdoc/async-fn.rs Show resolved Hide resolved
// Turning `fn f(&'_ self)` into `fn f(&self)` isn't the worst thing in the world, though;
// there's no case where it could cause the function to fail to compile.
let elided =
l.is_elided() || matches!(l.name, LifetimeName::Param(ParamName::Fresh(_)));
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 surprised that there are two possible ways we need to handle, when is it one versus the other? (Which test cases fail without one of these?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fresh params aren't considered elided in rustc itself: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_hir/hir.rs.html#130-143. Almost every test fails without this, the only thing GenericParam changes is removing the bogus fn f<'_>.

If you notice above, it was already checking is_elided, just not Fresh.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. That makes me wonder if we should push this change up to is_elided(), but that might have other side effects.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

The test-cases look good, although I note that there are very few cases with &self or &mut self, which is where I typically see the issue.

@tmandry
Copy link
Member

tmandry commented Dec 24, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 24, 2020

📌 Commit ceb66ad has been approved by tmandry

@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 Dec 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79213 (Stabilize `core::slice::fill`)
 - rust-lang#79999 (Refactored verbose print into a function)
 - rust-lang#80160 (Implemented a compiler diagnostic for move async mistake)
 - rust-lang#80274 (Rename rustc_middle::lint::LintSource)
 - rust-lang#80280 (Add installation commands to `x` tool README)
 - rust-lang#80319 (Fix elided lifetimes shown as `'_` on async functions)
 - rust-lang#80327 (Updated the match with the matches macro)
 - rust-lang#80330 (Fix typo in simplify_try.rs)
 - rust-lang#80340 (Don't unnecessarily override attrs for Module)
 - rust-lang#80342 (Fix typo)
 - rust-lang#80352 (BTreeMap: make test cases more explicit on failure)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d837407 into rust-lang:master Dec 25, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 25, 2020
@jyn514 jyn514 deleted the async-lifetimes branch December 25, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-lifetimes Area: lifetime related S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc shows anonymous elided lifetimes in async fn signature
7 participants