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: Change all 'optflag' arguments to 'optflagmulti' #87039

Closed
wants to merge 1 commit into from

Conversation

zachlute
Copy link
Contributor

@zachlute zachlute commented Jul 10, 2021

Because specifying these flags multiple times will never be discernibly different in functionality from specifying them a single time, there is no reason to fail and report an error to the user.

This is a new PR to replace PR #73936, because as usual I messed up that branch somehow and this change is small enough that it's easier to redo it than figure out how to fix that.

Copying my original comments from that PR:

This might be a slightly controversial change. it's tough to say, but it's hard to imagine a case where somebody was depending on this behavior, and doing this seem actively better for the user.

This originally came up in discussion of a fix for Cargo #8373, in Cargo PR #8422.

The issue is that Cargo will automatically add things like --document-private-items to binaries, because it's the only thing that makes sense there. Then some poor user comes along and adds --document-private-items to their rustdoc flags for the project and suddenly they're getting errors for specifying a flag twice and need to track down which targets to actually add it to without getting duplicates for reasons they won't understand without deep understanding of Cargo behavior.

We're apparently hesitant to inspect rustdoc flags provided by the user directly in Cargo, because they're supposed to be opaque, so looking to see if it's already provided before adding it is evidently a non-starter. In trying to resolve that, one suggestion I came up with was to just change rustdoc to support passing the flag multiple times, because the user's intent should be clear and it's not really an error, so maybe this is a case of 'be permissive in what you accept'.

This PR is an attempt to do that in a straightforward manner for purposes of discussion.

Because specifying these flags multiple times will never be discernibly different in functionality from specifying them a single time, there is no reason to fail and report an error to the user.
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/src/librustdoc/lib.rs at line 598:
                 "[unversioned-shared-resources,toolchain-shared-resources,invocation-specific]",
         }),
         }),
-        unstable("no-run", |o| o.optflagmulti("", "no-run", "Compile doctests without running them")),
+        unstable("no-run", |o| {
+            o.optflagmulti("", "no-run", "Compile doctests without running them")
+        }),
         unstable("show-type-layout", |o| {
             o.optflagmulti("", "show-type-layout", "Include the memory layout of types in the docs")
         }),
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/src/librustdoc/lib.rs" "/checkout/src/librustdoc/passes/collect_trait_impls.rs" "/checkout/src/librustdoc/doctree.rs" "/checkout/src/librustdoc/passes/bare_urls.rs" "/checkout/src/librustdoc/html/render/cache.rs" "/checkout/src/librustdoc/docfs.rs" "/checkout/src/librustdoc/json/mod.rs" "/checkout/src/librustdoc/passes/unindent_comments/tests.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2021

Closing this, please use the existing PR so all the discussion is in one place. I can force-push to that branch if you like, or you should be able to do it yourself like so:

git checkout rustdoc-optflagmulti
git reset --hard rustdoc-optflagmulti2
git push -f

@jyn514 jyn514 closed this Jul 10, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 10, 2021

@zachlute zachlute deleted the rustdoc-optflagmulti2 branch July 10, 2021 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants