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

Jump to bottom

Don't display mut in arguments for functions documentation #81831

Merged
merged 3 commits into from Feb 12, 2021
Merged

Don't display mut in arguments for functions documentation #81831

merged 3 commits into from Feb 12, 2021

Conversation

LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Feb 6, 2021 •
edited

Fixes #81289 by reverting #80799, as requested in #81328 (comment).
Supersedes #81328.
r? @camelid cc @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 6, 2021
camelid
camelid reviewed Feb 7, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Can you re-add the tests that you removed?

@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 7, 2021
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Feb 8, 2021 •
edited

Right now it will make rustdoc panic: https://github.com/rust-lang/rust/blob/95acccc32a090937bde0fbe087358721df888629/src/librustdoc/clean/utils.rs#L412-L415

Do you want me to fix this?

@jyn514
Copy link
Member

jyn514 commented Feb 8, 2021

Yes please, take a look at #80220 for suggestions on how.

LeSeulArtichaut
LeSeulArtichaut commented Feb 8, 2021
View reviewed changes
src/librustdoc/clean/utils.rs
Comment on lines +406 to +291
PatKind::Lit(..) => {
warn!(
"tried to get argument name from PatKind::Lit, which is silly in function arguments"
);
return Symbol::intern("()");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, shouldn't this just panic? The case for () is handled through PatKind::Tuple

Copy link
Member

Choose a reason for hiding this comment

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

Where is this warn! macro even coming from?

Copy link
Member

Choose a reason for hiding this comment

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

rust/src/librustdoc/lib.rs

Lines 24 to 25 in 0fc6756

#[macro_use]
extern crate tracing;

Copy link
Member

Choose a reason for hiding this comment

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

It probably should panic, but let's do that in a different PR so we can fix the regression.

@LeSeulArtichaut LeSeulArtichaut removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2021
@LeSeulArtichaut
Copy link
Contributor Author

I took the easy route and replaced the range pattern with _. My reasoning was that range patterns are pretty much never used in the wild and are (AFAICT) semantically equivalent to _.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@LeSeulArtichaut
Copy link
Contributor Author

CI is passing now

camelid
camelid requested changes Feb 9, 2021
View reviewed changes
src/test/rustdoc/mut-params.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/utils.rs
Comment on lines +406 to +291
PatKind::Lit(..) => {
warn!(
"tried to get argument name from PatKind::Lit, which is silly in function arguments"
);
return Symbol::intern("()");
}
Copy link
Member

Choose a reason for hiding this comment

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

It probably should panic, but let's do that in a different PR so we can fix the regression.

src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 9, 2021
@LeSeulArtichaut LeSeulArtichaut removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2021
@camelid
Copy link
Member

camelid commented Feb 12, 2021

Will hopefully get to this tomorrow.

@camelid camelid added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 12, 2021
@camelid
Copy link
Member

camelid commented Feb 12, 2021

Beta-nominating since this bug is now on beta.

@camelid
Copy link
Member

camelid commented Feb 12, 2021

Actually I have time now!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2021

📌 Commit 089ee27 has been approved by camelid

@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 12, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 12, 2021

Backporting sounds good to me - @rust-lang/rustdoc what do you think?

@camelid
Copy link
Member

camelid commented Feb 12, 2021

Note that beta only branched a few days ago, so it's just because it landed a few days late.

@Manishearth
Copy link
Member

Backport sgtm

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 12, 2021
@JohnTitor
Rollup merge of rust-lang#81831 - LeSeulArtichaut:81289-mut-arg, r=ca…
b2c8d7d 
…melid

Don't display `mut` in arguments for functions documentation

Fixes rust-lang#81289 by reverting rust-lang#80799, as requested in rust-lang#81328 (comment).
Supersedes rust-lang#81328.
r? `@camelid` cc `@jyn514`
@JohnTitor JohnTitor mentioned this pull request Feb 12, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Feb 12, 2021
Merged
@GuillaumeGomez
Copy link
Member

Sounds good to m as well.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2021
@bors
Auto merge of rust-lang#82025 - JohnTitor:rollup-cohp0gy, r=JohnTitor
d416093 
Rollup of 16 pull requests

Successful merges:

 - rust-lang#79983 (fix indefinite article in cell.rs)
 - rust-lang#81831 (Don't display `mut` in arguments for functions documentation)
 - rust-lang#81947 (Relax ItemCtxt::to_ty lifetime)
 - rust-lang#81954 (RELEASES.md 1.50: Group platform support notes together)
 - rust-lang#81955 (bootstrap: Locate llvm-dwp based on llvm-config bindir)
 - rust-lang#81959 (Fix assosiated typo)
 - rust-lang#81964 (Fix documentation not showing on localStorage error)
 - rust-lang#81968 (bootstrap: fix wrong docs installation path)
 - rust-lang#81990 (Make suggestion of changing mutability of arguments broader)
 - rust-lang#81994 (Improve long explanation for E0542 and E0546)
 - rust-lang#81997 (dist: include src/build_helper as part of the crate graph for rustc-dev)
 - rust-lang#82003 (Stack probes: fix error message)
 - rust-lang#82004 (clean up clean::Static struct)
 - rust-lang#82011 (Fix private intra-doc warnings on associated items)
 - rust-lang#82013 (Tell user how to fix CI file being not up to date)
 - rust-lang#82017 (Fix typo in mod.rs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@jyn514
Copy link
Member

jyn514 commented Feb 12, 2021

Awesome, that's more than half the team so I'm going to go ahead and mark this as accepted.

@jyn514 jyn514 added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 12, 2021
@bors bors merged commit 327762f into rust-lang:master Feb 12, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 12, 2021
@LeSeulArtichaut LeSeulArtichaut deleted the 81289-mut-arg branch February 12, 2021 17:44
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 13, 2021
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.52.0, 1.51.0 Feb 13, 2021
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Feb 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2021
@bors
Auto merge of rust-lang#82062 - Mark-Simulacrum:beta-next, r=Mark-Sim…
14b2755 
…ulacrum

[beta] backports

This backports some PRs and bumps to the released stable compiler:

* bootstrap: fix wrong docs installation path rust-lang#81968
* parser: Fix panic in 'const impl' recovery rust-lang#81876
* Don't display `mut` in arguments for functions documentation rust-lang#81831

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@camelid camelid camelid requested changes

Assignees

@camelid camelid

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

rustdoc documents mut self as mut self: Self
10 participants
@LeSeulArtichaut @jyn514 @rust-log-analyzer @camelid @bors @Manishearth @GuillaumeGomez @Mark-Simulacrum @rust-highfive @rustbot