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

Stabilize custom_code_classes_in_docs feature #124577

Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 1, 2024

Fixes #79483.

This feature has been around for quite some time now, I think it's fine to stabilize it now.

Summary

What is the feature about?

In short, this PR changes two things, both related to codeblocks in doc comments in Rust documentation:

  • Allow to disable generation of language-* CSS classes with the custom attribute.
  • Add your own CSS classes to a code block so that you can use other tools to highlight them.

The custom attribute

Let's start with the new custom attribute: it will disable the generation of the language-* CSS class on the generated HTML code block. For example:

/// ```custom,c
/// int main(void) {
///     return 0;
/// }
/// ```

The generated HTML code block will not have class="language-c" because the custom attribute has been set. The custom attribute becomes especially useful with the other thing added by this feature: adding your own CSS classes.

Adding your own CSS classes

The second part of this feature is to allow users to add CSS classes themselves so that they can then add a JS library which will do it (like highlight.js or prism.js), allowing to support highlighting for other languages than Rust without increasing burden on rustdoc. To disable the automatic language-* CSS class generation, you need to use the custom attribute as well.

This allow users to write the following:

/// Some code block with `{class=language-c}` as the language string.
///
/// ```custom,{class=language-c}
/// int main(void) {
///     return 0;
/// }
/// ```
fn main() {}

This will notably produce the following HTML:

<pre class="language-c">
int main(void) {
    return 0;
}</pre>

Instead of:

<pre class="rust rust-example-rendered">
<span class="ident">int</span> <span class="ident">main</span>(<span class="ident">void</span>) {
    <span class="kw">return</span> <span class="number">0</span>;
}
</pre>

To be noted, we could have written {.language-c} to achieve the same result. . and class= have the same effect.

One last syntax point: content between parens ((like this)) is now considered as comment and is not taken into account at all.

In addition to this, I added an unknown field into LangString (the parsed code block "attribute") because of cases like this:

/// ```custom,class:language-c
/// main;
/// ```
pub fn foo() {}

Without this unknown field, it would generate in the DOM: <pre class="language-class:language-c language-c">, which is quite bad. So instead, it now stores all unknown tags into the unknown field and use the first one as "language". So in this case, since there is no unknown tag, it'll simply generate <pre class="language-c">. I added tests to cover this.

EDIT(camelid): This description is out-of-date. Using custom,class:language-c will generate the output <pre class="language-class:language-c"> as would be expected; it treats class:language-c as just the name of a language (similar to the langstring c or js or what have you) since it does not use the designed class syntax.

Finally, I added a parser for the codeblock attributes to make it much easier to maintain. It'll be pretty easy to extend.

As to why this syntax for adding attributes was picked: it's Pandoc's syntax. Even if it seems clunkier in some cases, it's extensible, and most third-party Markdown renderers are smart enough to ignore Pandoc's brace-delimited attributes (from this comment).

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 1, 2024
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the stabilize-custom_code_classes_in_docs branch from 23d200e to f2118b8 Compare May 1, 2024 13:32
@GuillaumeGomez
Copy link
Member Author

Wow, didn't pay attention but that's a lot of removed lines. :D

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the stabilize-custom_code_classes_in_docs branch from f2118b8 to 2f6abd1 Compare May 1, 2024 14:45
@GuillaumeGomez
Copy link
Member Author

Ok this is ready for review now.

@notriddle
Copy link
Contributor

@rfcbot fcp merge

@rfcbot

This comment was marked as resolved.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 1, 2024
@GuillaumeGomez
Copy link
Member Author

Hum, why is the compiler team part of this? Because of the feature in the compiler source maybe?

@compiler-errors
Copy link
Member

compiler-errors commented May 1, 2024

@rfcbot fcp cancel

Autotagged. I don't think the compiler team needs to participate in this.

@rfcbot
Copy link

rfcbot commented May 1, 2024

@compiler-errors proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 1, 2024
@compiler-errors compiler-errors added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 1, 2024
@compiler-errors
Copy link
Member

Please start a new one @GuillaumeGomez or @notriddle

@GuillaumeGomez
Copy link
Member Author

Oh it's based on PR's tags and not the source code. Noted. Thanks compiler-errors!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 1, 2024

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 1, 2024
@camelid
Copy link
Member

camelid commented May 2, 2024

Do you mind posting a brief summary of the feature as it currently stands? It seems there was a lot of back-and-forth over things like syntax in the implementation PR.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 22, 2024
@rfcbot
Copy link

rfcbot commented May 22, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 22, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 1, 2024
@rfcbot
Copy link

rfcbot commented Jun 1, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@GuillaumeGomez
Copy link
Member Author

@bors r=rustdoc

@bors
Copy link
Contributor

bors commented Jun 1, 2024

📌 Commit 2f6abd1 has been approved by rustdoc

It is now in the queue for this repository.

@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 Jun 1, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 1, 2024
…de_classes_in_docs, r=rustdoc

Stabilize `custom_code_classes_in_docs` feature

Fixes rust-lang#79483.

This feature has been around for quite some time now, I think it's fine to stabilize it now.

## Summary

## What is the feature about?

In short, this PR changes two things, both related to codeblocks in doc comments in Rust documentation:

 * Allow to disable generation of `language-*` CSS classes with the `custom` attribute.
 * Add your own CSS classes to a code block so that you can use other tools to highlight them.

#### The `custom` attribute

Let's start with the new `custom` attribute: it will disable the generation of the `language-*` CSS class on the generated HTML code block. For example:

```rust
/// ```custom,c
/// int main(void) {
///     return 0;
/// }
/// ```
```

The generated HTML code block will not have `class="language-c"` because the `custom` attribute has been set. The `custom` attribute becomes especially useful with the other thing added by this feature: adding your own CSS classes.

#### Adding your own CSS classes

The second part of this feature is to allow users to add CSS classes themselves so that they can then add a JS library which will do it (like `highlight.js` or `prism.js`), allowing to support highlighting for other languages than Rust without increasing burden on rustdoc. To disable the automatic `language-*` CSS class generation, you need to use the `custom` attribute as well.

This allow users to write the following:

```rust
/// Some code block with `{class=language-c}` as the language string.
///
/// ```custom,{class=language-c}
/// int main(void) {
///     return 0;
/// }
/// ```
fn main() {}
```

This will notably produce the following HTML:

```html
<pre class="language-c">
int main(void) {
    return 0;
}</pre>
```

Instead of:

```html
<pre class="rust rust-example-rendered">
<span class="ident">int</span> <span class="ident">main</span>(<span class="ident">void</span>) {
    <span class="kw">return</span> <span class="number">0</span>;
}
</pre>
```

To be noted, we could have written `{.language-c}` to achieve the same result. `.` and `class=` have the same effect.

One last syntax point: content between parens (`(like this)`) is now considered as comment and is not taken into account at all.

In addition to this, I added an `unknown` field into `LangString` (the parsed code block "attribute") because of cases like this:

```rust
/// ```custom,class:language-c
/// main;
/// ```
pub fn foo() {}
```

Without this `unknown` field, it would generate in the DOM: `<pre class="language-class:language-c language-c">`, which is quite bad. So instead, it now stores all unknown tags into the `unknown` field and use the first one as "language". So in this case, since there is no unknown tag, it'll simply generate `<pre class="language-c">`. I added tests to cover this.

EDIT(camelid): This description is out-of-date. Using `custom,class:language-c` will generate the output `<pre class="language-class:language-c">` as would be expected; it treats `class:language-c` as just the name of a language (similar to the langstring `c` or `js` or what have you) since it does not use the designed class syntax.

Finally, I added a parser for the codeblock attributes to make it much easier to maintain. It'll be pretty easy to extend.

As to why this syntax for adding attributes was picked: it's [Pandoc's syntax](https://pandoc.org/MANUAL.html#extension-fenced_code_attributes). Even if it seems clunkier in some cases, it's extensible, and most third-party Markdown renderers are smart enough to ignore Pandoc's brace-delimited attributes (from [this comment](rust-lang#110800 (comment))).

r? `@notriddle`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2024
…llaumeGomez

Rollup of 4 pull requests

Successful merges:

 - rust-lang#124577 (Stabilize `custom_code_classes_in_docs` feature)
 - rust-lang#125683 (Rewrite `suspicious-library`, `resolve-rename` and `incr-prev-body-beyond-eof` `run-make` tests in `rmake.rs` format)
 - rust-lang#125773 (Migrate run make cdylib)
 - rust-lang#125808 (Migrate `run-make/c-link-to-rust-dylib` to `rmake.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jun 1, 2024

⌛ Testing commit 2f6abd1 with merge 05965ae...

@bors
Copy link
Contributor

bors commented Jun 1, 2024

☀️ Test successful - checks-actions
Approved by: rustdoc
Pushing 05965ae to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 1, 2024
@bors bors merged commit 05965ae into rust-lang:master Jun 1, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 1, 2024
@GuillaumeGomez GuillaumeGomez deleted the stabilize-custom_code_classes_in_docs branch June 1, 2024 13:13
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (05965ae): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.8%, -0.2%] 9
Improvements ✅
(secondary)
-0.7% [-0.9%, -0.5%] 2
All ❌✅ (primary) -0.4% [-0.8%, -0.2%] 9

Max RSS (memory usage)

Results (primary 1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Cycles

Results (secondary 2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.1%, 4.4%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.037s -> 668.888s (0.13%)
Artifact size: 318.86 MiB -> 318.81 MiB (-0.02%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 7, 2024
@decathorpe
Copy link

This PR seems to have caused a regression (or at least a possibly unexpected behaviour change) that affects a large-ish number of crates - previously, code block attributes that were not recognized as "rust code" in some way were ignored and not compiled, but now "invalid" code block attributes raise a new rustdoc warning lint, but are compiled (and run) as Rust code.

For example, the code blocks here use {rust,ignore} instead of rust,ignore - they were previously ignored, but are now attempted to be compiled + run:

https://github.com/servo/rust-cssparser/blob/main/src/lib.rs#L44-L64

I noticed this because the CI we have for Fedora packages started to turn red for 2-3 dozen Rust crates after Rust 1.80 landed a few days ago.

Was this behaviour change intentional? While I agree that it is likely a good idea to warn on "invalid" or unrecognized code block attributes, attempting to compile + run code blocks with unrecognized attributes might not be.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 9, 2024
…-not-rust, r=GuillaumeGomez

rustdoc: do not run doctests with invalid langstrings

rust-lang#124577 (comment)

CC `@decathorpe`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 9, 2024
…-not-rust, r=GuillaumeGomez

rustdoc: do not run doctests with invalid langstrings

rust-lang#124577 (comment)

CC ``@decathorpe``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2024
Rollup merge of rust-lang#128838 - notriddle:notriddle/invalid-tag-is-not-rust, r=GuillaumeGomez

rustdoc: do not run doctests with invalid langstrings

rust-lang#124577 (comment)

CC ``@decathorpe``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

Tracking Issue for user-provided classnames in fenced code blocks