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

[ebkalderon]

r? @jyn514

Fixes #76517.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[ebkalderon]

Would you like me to prepare any tests for this fix?

[jyn514]

Yes please; add them to src/test/rustdoc. There are examples there and instructions at https://rustc-dev-guide.rust-lang.org/rustdoc-internals.html#dotting-is-and-crossing-ts.

[jyn514]

 error[E0596]: cannot borrow `vec` as mutable, as it is not declared as mutable
  --> /checkout/src/test/ui/async-await/argument-patterns.rs:12:20
   |
LL | async fn b(n: u32, ref mut vec: A) {
   |                    ^^^^^^^^^^^
   |                    |
   |                    cannot borrow as mutable
   |                    help: consider changing this to be mutable: `mut vec`

error: aborting due to previous error

Welp, looks like this broke something. Not sure why.

r? @ecstatic-morse for review or re-assignment

[jyn514]

Actually, I wonder if this would work if you changed it to Unannotated | Mutable instead; that would avoid changing the logic for ref mut and I don't think that's valid in function parameters anyway.

Suggested change

	hir::PatKind::Binding(_, _, ident, _) => (ident, true),
	hir::PatKind::Binding(hir::BindingAnnotation::Unannotated | hir::BindingAnnotation::Mutable, _, ident, _) => (ident, true),

[jyn514]

and I don't think that's valid in function parameters anyway

It clearly is, see the error message above. So yeah, I'm not sure how to fix this.

[ebkalderon]

The issue, it seems, is as described in the comments above:

// If `<pattern>` is a simple ident, then it is lowered to a single
// `let <pattern> = <pattern>;` statement as an optimization.

I don't think that this is considered valid:

let mut bad1 = mut bad1;
let ref mut bad2 = ref mut bad2;

[ebkalderon]

This comment a little further down the page elaborates on the reasoning:

rust/compiler/rustc_ast_lowering/src/item.rs

Lines 1129 to 1132 in c101116

	if is_simple_parameter {
	// If this is the simple case, then we only insert one statement that is
	// `let <pat> = <pat>;`. We re-use the original argument's pattern so that
	// `HirId`s are densely assigned.

It seems that not reusing the original argument's pattern will mean that the HirIds will no longer be densely assigned. I wonder if this is still a pressing concern that must be addressed before considering this PR for merging, the current CI failures aside.

Is setting aside a special case for mutable assignment patterns even a good idea? Sure, it would fix the rendering issue in rustdoc, but perhaps this change to the AST lowering logic would have other implications elsewhere in the compiler?

[ebkalderon]

It seems that the tests now pass with the suggested change from @jyn514, but this is not yet ready to merge. I'm assuming that although, for example, let ref mut foo = ref mut foo; is invalid Rust, CI is passing because there are no tests in place to catch this. Once I add tests in place, we can check whether this change is safe or not.

[jyn514]

perhaps this change to the AST lowering logic would have other implications elsewhere in the compiler?

Right, this is why I asked for feedback from ecstatic-morse. Rustdoc doesn't really have a way to fix this on its end.

[ecstatic-morse]

r? @cramertj maybe? I don't know this code.

I would guess that there's no correct "quick fix" here. If that's correct, we would need to preserve a mapping from async fn parameters to their patterns for use by rustdoc to fix #76517.

[Dylan-DPC-zz]

r? @Mark-Simulacrum

[tmandry]

In the simple case we aren't actually inserting let <pat> = <pat>; as the comment says. We're inserting the statement let <pat> = <ident>; where ident is the original identifier named in the function signature. That means we were effectively doing the following in the ref mut case:

fn foo(ref mut x: i32) {
    let ref mut y = x;
}

i.e. trying to take a mutable reference to x, which itself is a &mut i32. But x is not a mutable binding, i.e. the integer x points to could be changed, but which integer x points to cannot be changed. Taking a mutable reference to x would violate that, so this is a compiler error.

I think we don't want this is_simple_parameter behavior anytime ref is involved. Which means that changing to hir::BindingAnnotation::Unannotated | hir::BindingAnnotation::Mutable was partly right.

However, we do actually want to preserve the original identifier for rustdoc to use, anytime the pattern is a binding. For ref cases, I don't see any harm in preserving the identifier and setting is_simple_parameter to false.

P.S. Keeping HirId's dense is just an optimization, not as important as correctness.

[jyn514]

Ok, that makes a lot of sense! So something like this would work:

                let (ident, is_simple_parameter) = match parameter.pat.kind {
                    hir::PatKind::Binding(
                        hir::BindingAnnotation::Unannotated | hir::BindingAnnotation::Mutable,
                        _,
                        ident,
                        _,
                    ) => (ident, true),
                    hir::PatKind::Binding(_, _, ident, _) => (ident, false),

@ebkalderon do you mind implementing that?

[jyn514]

Also, please add a test for both of these cases: async fn f(ref mut x: i32) and async fn f(mut x: i32).

[ebkalderon]

Yep, I'm on it!

[tmandry]

Ping from triage. @ebkalderon, are you still working on this?

[jyn514]

@ebkalderon do you mind if I take over this PR? It's most of the way there, I'd hate for it to be lost in limbo.

[tmandry]

@bors r+ rollup

Thanks all!

[bors]

📌 Commit 96793d3 has been approved by tmandry

[GuillaumeGomez]

It failed CI: https://github.com/rust-lang-ci/rust/runs/1389783331

@bors: r-

[jyn514]

Heh, the test @lcnr added for the wrong behavior in #78916 failed. Just needs a rebase.

[jyn514]

@bors r=tmandry rollup

[bors]

📌 Commit 38127ca has been approved by tmandry