rustdoc: clean up and test macro visibility print #84074
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes the overly-complex invariant mentioned in <rust-lang#83237 (comment)>, where the macro source can't have any links in it only because the cache hasn't been populated yet.
|
r? @CraftSpider (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
the
T-rustdoc
jyn514
reviewed
Apr 16, 2021
Comment on lines
-1215
to
+1217
| let first_name = | ||
| path.data[0].data.get_opt_name().expect("modules are always named"); | ||
| // modified from `resolved_path()` to work with `DefPathData` | ||
| let last_name = path.data.last().unwrap().data.get_opt_name().unwrap(); | ||
| let anchor = anchor(vis_did, &last_name.as_str(), cache).to_string(); | ||
|
|
||
| let mut s = "pub(".to_owned(); | ||
| if path.data.len() != 1 | ||
| || (first_name != kw::SelfLower && first_name != kw::Super) | ||
| { | ||
| s.push_str("in "); | ||
| } | ||
| let mut s = "pub(in ".to_owned(); |
There was a problem hiding this comment.
It seemed impossible for this conditional to evaluate to false. It's checking if the path is exactly equal to self or super, but there are already conditionals above that check if it's equal to the current module or the parent module.
Removing it also didn't break any test cases, supporting my dead-code theory.
jyn514
added
S-waiting-on-author
|
@notriddle I have a question about how this interacts with #83237 - you add an assert there that |
That assert is actually outdated. The |
|
I guess it no longer matters one way or the other, yeah - all the assert is checking is that to_source_with_space is consistent with the previous behavior, but the previous behavior was wrong if the cache wasn't empty. Removing the assert sounds fine, since the test suite passed with it once already, that gives me confidence it was at least correct most of the time. |
|
@bors r+ |
|
📌 Commit 2dfd0bf has been approved by |
bors
added
S-waiting-on-bors
jyn514
pushed a commit
to jyn514/rust
that referenced
this pull request
Apr 16, 2021
… r=jyn514 rustdoc: clean up and test macro visibility print This fixes the overly-complex invariant mentioned in <rust-lang#83237 (comment)>, where the macro source can't have any links in it only because the cache hasn't been populated yet.
|
Will this fix #83000 or is it a pure cleanup with no (expected) behavior changes? |
|
It should have no visible behaviour changes. Just pure cleanup. |
|
☀️ Test successful - checks-actions |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Nov 13, 2024
…o-decl, r=notriddle rustdoc: Treat declarative macros more like other item kinds Apparently at some time in the past we were unable to generate an href for the module path inside the visibility of decl macros 2.0 (`pub(in ...)`). As a result of this, a whole separate function was introduced specifically for printing the visibility of decl macros that didn't attempt to generate any links. The description of PR rust-lang#84074 states: > This fixes the overly-complex invariant mentioned in rust-lang#83237 (comment), where the macro source can't have any links in it only because the cache hasn't been populated yet. I can no longer reproduce the original issue. Reusing the existing visibility rendering logic *seems* to work just fine (I couldn't come up with any counterexamples, though I invite you to prove me wrong). * Fixes rust-lang#83000 * Fixes the visibility showing up "twice" in rustdoc-JSON output: Once as the `visibility` field, once baked into the source[^1] * Fixes `#[doc(hidden)]` not getting rendered on doc(hidden) decl macros 2.0 under `--document-hiden-items` (for decl macros 1.2 the issue remains; I will address this separately when fixing rust-lang#132304). --- <details><summary>Outdated Section</summary> NOTE: The current version of this PR is committing a UI crime, I'd like to receive feedback on that. Maybe you have a satisfactory solution for how to remedy it. Namely, as you know we have two different ways of / modes for highlighting code with color: 1. Only highlighting links / item paths and avoiding to highlight tokens by kind like keywords (to reduce visual noise and maybe also artifact size). Used for item declarations(\*). 2. Highlighting tokens by kind. Used for code blocks written by the user. (\*): With the notable exception being macro declarations! Well, since this PR reuses the same function for rendering the item visibility (which only makes sense), we have a clash of modes: We now use both ways of highlighting code for decl macros: №1 for the visibility, №2 for the rest. This awkward. See for yourself: * On master:  * On this branch:  </details> Furthermore, we now no longer syntax-highlight declarative macros (be it `macro_rules!` or `macro`) since that was inconsistent with the way we render all other item kinds. See (collapsed) *Outdated Section* above. See also rust-lang#132302 (comment). | On master | On this branch | |---|---| |  |  | [^1]: E.g., `"visibility":{"restricted":{"parent":1,"path":"::a"}},/*OMITTED*/,"inner":{"macro":"pub(in a) macro by_example_vis_named($foo:expr) {\n ...\n}"}`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 14, 2024
Rollup merge of rust-lang#132302 - fmease:rustdoc-better-vis-for-macro-decl, r=notriddle rustdoc: Treat declarative macros more like other item kinds Apparently at some time in the past we were unable to generate an href for the module path inside the visibility of decl macros 2.0 (`pub(in ...)`). As a result of this, a whole separate function was introduced specifically for printing the visibility of decl macros that didn't attempt to generate any links. The description of PR rust-lang#84074 states: > This fixes the overly-complex invariant mentioned in rust-lang#83237 (comment), where the macro source can't have any links in it only because the cache hasn't been populated yet. I can no longer reproduce the original issue. Reusing the existing visibility rendering logic *seems* to work just fine (I couldn't come up with any counterexamples, though I invite you to prove me wrong). * Fixes rust-lang#83000 * Fixes the visibility showing up "twice" in rustdoc-JSON output: Once as the `visibility` field, once baked into the source[^1] * Fixes `#[doc(hidden)]` not getting rendered on doc(hidden) decl macros 2.0 under `--document-hiden-items` (for decl macros 1.2 the issue remains; I will address this separately when fixing rust-lang#132304). --- <details><summary>Outdated Section</summary> NOTE: The current version of this PR is committing a UI crime, I'd like to receive feedback on that. Maybe you have a satisfactory solution for how to remedy it. Namely, as you know we have two different ways of / modes for highlighting code with color: 1. Only highlighting links / item paths and avoiding to highlight tokens by kind like keywords (to reduce visual noise and maybe also artifact size). Used for item declarations(\*). 2. Highlighting tokens by kind. Used for code blocks written by the user. (\*): With the notable exception being macro declarations! Well, since this PR reuses the same function for rendering the item visibility (which only makes sense), we have a clash of modes: We now use both ways of highlighting code for decl macros: №1 for the visibility, №2 for the rest. This awkward. See for yourself: * On master:  * On this branch:  </details> Furthermore, we now no longer syntax-highlight declarative macros (be it `macro_rules!` or `macro`) since that was inconsistent with the way we render all other item kinds. See (collapsed) *Outdated Section* above. See also rust-lang#132302 (comment). | On master | On this branch | |---|---| |  |  | [^1]: E.g., `"visibility":{"restricted":{"parent":1,"path":"::a"}},/*OMITTED*/,"inner":{"macro":"pub(in a) macro by_example_vis_named($foo:expr) {\n ...\n}"}`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the overly-complex invariant mentioned in #83237 (comment), where the macro source can't have any links in it only because the cache hasn't been populated yet.