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: prefer showing enum variants as written #131975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link
Contributor

fixes #128347

@rustbot
Copy link
Collaborator

rustbot commented Oct 20, 2024

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 20, 2024
@aDotInTheVoid aDotInTheVoid added T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Oct 20, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:c32c805632780b5c1de330e3f44561b336c2efe163bc0990acb392390157a8e1d9f855d75914a239aa40c49d77f4a837247d05d2f8d46f554b98e1f46712a3e3:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.84s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
---- [rustdoc] tests/rustdoc/enum-variant-value.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/enum-variant-value" "/checkout/tests/rustdoc/enum-variant-value.rs"
--- stderr -------------------------------
16: has check failed
16: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@class="rust item-decl"]/code' 'C = 1_245,'
19: matches check failed
 `XPATH PATTERN` did not match
 //@ matches - '//*[@id="variant.C"]/h3' '^C = 1_245$'
103: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@class="rust item-decl"]/code' 'A = 2,'
104: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@class="rust item-decl"]/code' 'B = 4,'
105: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@class="rust item-decl"]/code' 'C = 9,'
107: matches check failed
 `XPATH PATTERN` did not match
 //@ matches - '//*[@id="variant.A"]/h3' '^A = 2$'
108: matches check failed
 `XPATH PATTERN` did not match
 //@ matches - '//*[@id="variant.B"]/h3' '^B = 4$'
109: matches check failed
 `XPATH PATTERN` did not match
 //@ matches - '//*[@id="variant.C"]/h3' '^C = 9$'
Encountered 8 errors
------------------------------------------


@lolbinarycat
Copy link
Contributor Author

the failing unit test is explicitly expecting the old behavior, i'll update it if we decide we actually want this behavior... i have a feeling i'm just reverting a previous change.

@lolbinarycat
Copy link
Contributor Author

maybe we could shove this behind an attribute, idk

@notriddle
Copy link
Contributor

@lolbinarycat
Copy link
Contributor Author

do new doc attributes have to go through the RFC process, even though they don't have an effect on the language semantics? i know the diagnostics namespace was specifically designed so that new attrs didn't need to go through an rfc.

an attribute really does seem like the best way of doing this.

@fmease
Copy link
Member

fmease commented Oct 21, 2024

do new doc attributes have to go through the RFC process, even though they don't have an effect on the language semantics? i know the diagnostics namespace was specifically designed so that new attrs didn't need to go through an rfc.

If I'm not mistaken, T-rustdoc has virtually full authority over #[doc(...)] and rustdoc-related lints and doesn't need T-lang to get involved for decision making. This is just my observation as a relatively new T-rustdoc member, so I'm lacking some historic context and knowledge.

In any case, the RFC process is a tool we can & arguably should use for such impactful decisions. However, similar to T-lang's alternative processes for feature development like lang experiments (formerly lang MCPs), we (can allow ourselves to) have some wiggle room. For example, an MVP for F-doc_cfg & F-doc_auto_cfg was implemented before an RFC even existed! Note that I don't actually know how this was decided back then.

Personally speaking I'm fine with adding an unstable #[doc(...)] attribute that controls the representation of constant expressions in rustdoc as part of a "T-rustdoc experiment". We can decide if we want to FCP this or at least discuss on Zulip / in a T-rustdoc meeting (FCP the addition of an unstable feature, not the stabilization mind you). An RFC would still be necessary for its stabilization (I don't think we have that in official writing but yeah).

@fmease
Copy link
Member

fmease commented Oct 21, 2024

even though they don't have an effect on the language semantics?

Well, it's tricky nonetheless and we have a lot of users, we should be mindful. Things can be missed at first sight. See for example issue #131625 which regressed due to PR #95316 in which I changed rustdoc to evaluate & print the RHS of associated constants1. It also lead to various other user reports like #99630, #102456 (and to the creation of PR #99688 which in turn kicked off the whole "new const display" topic that notriddle has already linked).

Footnotes

  1. Originally motivated by the fact that technically speaking the value of constants is part of the public API (changes can lead to downstream crates no longer compiling when they use them inside e.g. patterns or types).

@lolbinarycat
Copy link
Contributor Author

how about a #[doc(fold)] attribute that controls constant folding? it could use the same inheritance logic as #[doc(cfg_hide)], and would change the behavior of regular constants, associated constants, and c-style enums.

@camelid
Copy link
Member

camelid commented Nov 1, 2024

FWIW making this change will add yet another inconsistency for cross-crate re-exports. Because HIR is not available when inlining an item from another crate (e.g., when inlining a const from core into the std facade), we will show the original expression when looking at the original crate's version but the evaluated version when looking at the re-export. We may decide this is an acceptable tradeoff since we already have this issue with things like type aliases, but I do want to mention it.

I'm somewhat against having a doc(fold) attribute for this though. Given that we will be forced to always fold inlined constants, it seems confusing to let users control it in other cases.

@camelid
Copy link
Member

camelid commented Nov 1, 2024

Perhaps this is a situation where we could use rustdoc CCI (rust-lang/rfcs#3662) to store the original expression for public constants (that could potentially be inlined by other crates) so that we don't have the cross-crate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc converts byte literal discriminants into decimal numbers
7 participants