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

rustdoc renames parameters with mut in async fn #76517

Closed
ebkalderon opened this issue Sep 9, 2020 · 7 comments · Fixed by #76730
Closed

rustdoc renames parameters with mut in async fn #76517

ebkalderon opened this issue Sep 9, 2020 · 7 comments · Fixed by #76730
Assignees
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@ebkalderon
Copy link
Contributor

ebkalderon commented Sep 9, 2020

Original:

pub struct Foo;

impl Foo {
    pub async fn good(self, first: usize, second: usize) {}
    pub async fn bad(mut self, mut first: usize, mut second: usize) {}
}

Rendered:

rustdoc output screenshot

This example was tested on Rust 1.46.0, but it has been broken for quite a while. For example, see Server::serve() in tower-lsp version 0.7.0 (built with Rust 1.43.0-nightly, see docs.rs build info).

@ebkalderon ebkalderon added the C-bug Category: This is a bug. label Sep 9, 2020
@jonas-schievink jonas-schievink added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 9, 2020
@jyn514 jyn514 changed the title rustdoc doesn't infer correct type with mut args in async fn rustdoc renames parameters with mut in async fn Sep 14, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 14, 2020

Relevant code:

if let Some(selfty) = input.to_self() {

Not sure what's going wrong.

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Sep 15, 2020

Thanks for the helpful hint, @jyn514! However, I'm not convinced this is the root cause because it's happening to all mutable by-value function arguments, not just to async functions with self as the first argument. For example, this free function emits similar results:

pub async fn foo(mut first: usize, mut second: usize) {}

Screen Shot 2020-09-15 at 9 56 26 AM

I think the actual root cause may be deeper than the interpretation of Self types, but I can't tell for sure.

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Sep 15, 2020

This example confirms that the behavior appears to be per-argument instead of per-function. It's not like one bad argument ruins the formatting of the entire function.

pub async fn foo(good: usize, mut bad: usize) {}

Screen Shot 2020-09-15 at 10 04 39 AM

Perhaps we could investigate where in the code the __argN text is being generated and work our way backwards to see what is triggering it?

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Sep 15, 2020

Hmm, the only relevant location I could find through GitHub search is in rustc_ast_lowering in item.rs:

let name = format!("__arg{}", index);

Is rustdoc operating on a desugared form of the AST? I don't think this is the case, since async fn is being successfully displayed here. Is there something else I'm missing?

@jyn514
Copy link
Member

jyn514 commented Sep 15, 2020

Here's the issue:

// Check if this is a binding pattern, if so, we can optimize and avoid adding a
// `let <pat> = __argN;` statement. In this case, we do not rename the parameter.
let (ident, is_simple_parameter) = match parameter.pat.kind {
hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, _, ident, _) => {
(ident, true)
}
_ => {
// Replace the ident for bindings that aren't simple.
let name = format!("__arg{}", index);
let ident = Ident::from_str(&name);
(ident, false)
}
};

Looks like it should use hir::PatKind::Binding(_, _, ident, _) => { instead. Are you interested in making a PR? ;)

@jyn514
Copy link
Member

jyn514 commented Sep 15, 2020

Is rustdoc operating on a desugared form of the AST? I don't think this is the case, since async fn is being successfully displayed here. Is there something else I'm missing?

Yes, rustdoc works on a desugared version and has to reconstruct the async by hand.

@ebkalderon
Copy link
Contributor Author

ebkalderon commented Sep 15, 2020

Sure, @jyn514! I will prepare one right away. Thanks for investigating with me. 😄

@camelid camelid added the A-async-await Area: Async & Await label Oct 5, 2020
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 6, 2020
@tmandry tmandry added this to On deck in wg-async work via automation Oct 6, 2020
@tmandry tmandry moved this from On deck to In progress in wg-async work Oct 6, 2020
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 12, 2020
…nc-fn, r=tmandry

Fix rustdoc rendering of by-value mutable arguments in async fn

r? `@jyn514`

Fixes rust-lang#76517.
@bors bors closed this as completed in 4b0b42a Nov 13, 2020
wg-async work automation moved this from In progress to Done Nov 13, 2020
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 AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
Development

Successfully merging a pull request may close this issue.

5 participants