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] Add no-hidden-lines codeblock attribute #118711

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 7, 2023

Fixes #118027.

Motivation

When documentation macro and proc-macro, there is currently an extra layer of complexity to take into account when it uses # characters. Allowing to disable this rustdoc behaviour with a codeblock attribute would make this task much simpler for (proc) macro documentation writers.

Example from #118027:

define_component!(
    #[derive(Debug)]
    ##[min(0.0))]
    ##[max(100.0))]
    ##[default(50.0)]
    pub Fuel(pub f32);
);

in documentation codeblocks becomes:

define_component!(
    #[derive(Debug)]
    #[min(0.0))]
    #[max(100.0))]
    #[default(50.0)]
    pub Fuel(pub f32);
);

Generating invalid documentation for end users.

What is this feature about?

It is about adding a new no-hidden-lines codeblock attribute which would remove this # character cleanup pass in order to make it easier for (proc) macro API to have code examples.

The no-hidden-lines attribute works just like should_panic or ignore attributes: if there are no other tags, it'll consider the code block as a rust one and hence will test it.

Example of usage:

```no-hidden-lines
define_component!(
    #[derive(Debug)]
    ##[min(0.0))]
    ##[max(100.0))]
    ##[default(50.0)]
    pub Fuel(pub f32);
);
```

Concerns

  • Is no-hidden-lines the right name for this codeblock attribute?
  • Is this codeblock attribute really necessary?

r? @notriddle

@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 Dec 7, 2023
@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Member

Urgau commented Dec 7, 2023

Wouldn't it be simpler to have disambiguator so that you could still hide (for example your imports) ?

Using your example it wouldn't be possible to show #a, ##b and ###c but hide the import.

//! ```rust,raw
//! # use crate::test;  // this should be hidden
//!
//! test!(
//!     #a,             // but not thoses
//!     ##b,
//!     ###c
//! );
//! ```

@GuillaumeGomez
Copy link
Member Author

Macros are a nightmare on their own. You could very well have:

//! ```rust,raw
//! # use crate::test;  // this should be hidden
//!
//! test!(
//!     # use something::other;
//! );
//! ```

@notriddle
Copy link
Contributor

Is raw the right name for this codeblock attribute?

Not really. It should be more descriptive, like no_hidden_lines.

Is this codeblock attribute really necessary?

I'm curious how this will interact with custom. I know if it's used, custom should solve this same problem (at the cost of disabling syntax highlighting entirely).

Would turning syntax highlighting off entirely be an acceptable compromise? On the one hand, syntax highlighting doesn't seem very helpful with a huge macro DSL anyway, since rustdoc won't know anything about the syntax of the macro, but, on the other hand, it might work fine if the macro embeds lots of normal Rust code (or is intentionally designed to use Rust keywords for its own keywords instead of making them up).

@GuillaumeGomez
Copy link
Member Author

Using custom would also mean no doctest, which I think would be a bigger problem than not having syntax highlighting.

@notriddle
Copy link
Contributor

That's what it looks like to me, too, though I can't find any test cases for that (should probably add one).

I know #78917 wanted a way to have doctested code without syntax highlighting. Is there any way to do that right now?

@GuillaumeGomez
Copy link
Member Author

No, doctests are only rust codeblocks. And rust codeblocks have syntax highlighting.

@notriddle
Copy link
Contributor

That's weird... #78917 wanted to turn syntax highlighting off while leaving doctesting on (though they still wanted hidden lines).

So, reading through that issue again, I guess no_hidden_lines is the best name for this option, since it's orthogonal to the actual question of syntax highlighting.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 7, 2023

Indeed. I'll also use the opportunity to rebase to fix the CI failure.

@GuillaumeGomez GuillaumeGomez force-pushed the raw-codeblock-attribute branch from 25061a1 to d7fa81a Compare December 8, 2023 09:59
@GuillaumeGomez GuillaumeGomez changed the title [rustdoc] Add raw codeblock attribute [rustdoc] Add no-hidden-lines codeblock attribute Dec 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

Could not assign reviewer from: notriddle.
User(s) notriddle are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@GuillaumeGomez GuillaumeGomez force-pushed the raw-codeblock-attribute branch from d7fa81a to dcf973f Compare December 8, 2023 09:59
@GuillaumeGomez
Copy link
Member Author

Renamed the codeblock attribute to no-hidden-lines.

// @matches - '//*[@class="rust rust-example-rendered"]/code' '\(\s+#a,\s+##b,\s+###c\s+\);'
// @snapshot 'codeblock' - '//*[@class="rust rust-example-rendered"]/code'

//! ```rust,no_hidden_lines
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this isn't gated behind a feature flag. Is that intentional?

(I'm fine either way, but if it's going to be insta-stable then it's going to need to go through an FCP.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's what I want indeed but in any case I'd like us to go through FCP so every one can check it.

@notriddle
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 8, 2023

Team member @notriddle 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 Dec 8, 2023
@aDotInTheVoid
Copy link
Member

@rfcbot concern why is this insta stable?

Otherwise this looks good

@GuillaumeGomez
Copy link
Member Author

It seemed small enough to me (it's a new codeblock attribute for a very specific use-case). If it's an issue I can put it behind a feature. Just seems not worth it.

@Nemo157
Copy link
Member

Nemo157 commented Dec 9, 2023

From the docs, line hiding only occurs on # , so the only time we need to do the replacement is ## => # not ##[ => #[; that seems like it might be backwards compatible to fix (EDIT: or maybe if there are people using ###[ already to work around the issue, it could be an edition fix).

@Manishearth
Copy link
Member

@rfcbot concern is-this-a-bug

I'm not convinced the behavior where indented pound symbols are hidden is not a bug. I didn't even know we did that, I always thought it only applied to lines that start with a pound symbol.

If we were designing the feature today I would have been pretty strongly against having it apply to all pound signs regardless of indentation.

It would be somewhat breaking but it's worth figuring out how many crates out there actually use this when indented.

@GuillaumeGomez GuillaumeGomez force-pushed the raw-codeblock-attribute branch from dcf973f to c989304 Compare December 10, 2023 20:26
@GuillaumeGomez
Copy link
Member Author

From the docs, line hiding only occurs on # , so the only time we need to do the replacement is ## => # not ##[ => #[; that seems like it might be backwards compatible to fix (EDIT: or maybe if there are people using ###[ already to work around the issue, it could be an edition fix).

That's why I went with raw as tag originally. I can revert this part of the change if wanted. Might be worth checking if we can remove it.

@rfcbot concern is-this-a-bug

I'm not convinced the behavior where indented pound symbols are hidden is not a bug. I didn't even know we did that, I always thought it only applied to lines that start with a pound symbol.

If we were designing the feature today I would have been pretty strongly against having it apply to all pound signs regardless of indentation.

It would be somewhat breaking but it's worth figuring out how many crates out there actually use this when indented.

I can send a PR tomorrow removing this behaviour so we can make a crater run.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
… r=<try>

Remove weird handling of ## in code examples

As mentioned in rust-lang#118711, the handling of `##` in code blocks in code examples is very surprising. Let's see if crates are using it with a crater run.

r? `@notriddle`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
… r=<try>

Remove weird handling of ## in code examples

As mentioned in rust-lang#118711, the handling of `##` in code blocks in code examples is very surprising. Let's see if crates are using it with a crater run.

r? `@notriddle`
@bors
Copy link
Contributor

bors commented Dec 13, 2023

☔ The latest upstream changes (presumably #118900) made this pull request unmergeable. Please resolve the merge conflicts.

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. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc replaces ## with # even if it is in a code block
10 participants