[librustdoc] Only split lang string on ,, , and \t

[casey]

Split markdown lang strings into tokens on ,.

The previous behavior was to split lang strings into tokens on any
character that wasn't a _, _, or alphanumeric.

This is a potentially breaking change, so please scrutinize! See discussion in #78344.

I noticed some test cases that made me wonder if there might have been some reason for the original behavior:

t("{.no_run .example}", false, true, Ignore::None, true, false, false, false, v(), None);
t("{.sh .should_panic}", true, false, Ignore::None, false, false, false, false, v(), None);
t("{.example .rust}", false, false, Ignore::None, true, false, false, false, v(), None);
t("{.test_harness .rust}", false, false, Ignore::None, true, true, false, false, v(), None);

It seemed pretty peculiar to specifically test lang strings in braces, with all the tokens prefixed by ..

I did some digging, and it looks like the test cases were added way back in this commit from 2014 by @skade.

It looks like they were added just to make sure that the splitting was permissive, and aren't testing that those strings in particular are accepted.

Closes #78344.

[rust-highfive]

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)

[jyn514]

@rfcbot fcp merge

This changes the parsing of language strings for code blocks so that only ,, , and \t are accepted as delimiters. Previously, any character that couldn't be part of an identifier was treated as a delimiter. This gives rustdoc increased flexibility to distinguish betweeen delimiters, such as for https://github.com/casey/rfcs/blob/doctest-name/text/0000-doctest-name.md.

This is a breaking change, but in practice it seems unlikely anyone is using this. It's possible that we could find out with a crater run if crater allows denying specific warnings.

Possible alternatives:

• Only disallow =. This still unblocks the extension @casey proposes in the linked RFC, but allows using all other non-identifiers as delimiters. Any future extensions to lang strings will have to reserve more delimiters at that time.
• Keep the status quo. This is not a breaking change, but means that rustdoc will never be able to add more extensions to language strings.

[rfcbot]

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

• @GuillaumeGomez
• @Manishearth
• @cramertj
• @joshtriplett
• @jyn514
• @nikomatsakis
• @ollie27
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

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.

[jyn514]

@casey as-is, this breaks trailing text after spaces:

/// ignore (tidy)
/// goo();
/// ```
pub fn bar() -> usize { 2 }

6: @has check failed
	`XPATH PATTERN` did not match
	// @has foo/fn.bar.html '//*[@class="tooltip ignore"]/span' "This example is not tested"

What do you think about changing this to split on either spaces or , instead?

[casey]

@jyn514 Sounds good to me! I think using spaces is common, particularly with GitHub flavored markdown, so splitting on spaces seems reasonable. I also split on tabs, since it seems weird to split on spaces but not tabs, and since users sometimes use tabs for alignment.

[GuillaumeGomez]

Hum also: we use such headers in rust tests:

```rust,ignore (whatever is the reason)

So splitting on whistepaces and tabs might not be a good idea...

[casey]

Unrecognized tokens are ignored, so this shouldn't change the set of recognized tokens in the above example.

Before the change, rust,ignore (whatever is the reason) will be split into rust, ignore, whatever, is, the, reason, and all but rust and ignore are ignored.

After the change, rust,ignore (whatever is the reason) will be split into rust, ignore, (whatever, is, the, reason), and all but rust and ignore are ignored.

[GuillaumeGomez]

Yes but what's the point of splitting on whitespaces and tabs?

[casey]

If it didn't split on spaces, then something like ignore (tidy) will be split as ignore (tidy) (one token) and so won't work. This seems to be somewhat common, so it seems better not to break it.

[GuillaumeGomez]

Oh I see, good point!

[nikomatsakis]

Nominating for @rust-lang/lang meeting to draw attention to the PR.

[nikomatsakis]

I'd like to see crater results, but I think this is reasonable. It'd be nice to have a survey of how other markdown systems approach this though.

[Manishearth]

Also would like to see crater results, but I'm fine with this otherwise

@rfcbot approve

[casey]

I did a mini survey of other markdown systems.

• Github: Splits on ,, , and \t.
• Gitlab: Doesn't split on anything. Entire info string is used verbatim, so rust,foo and rust foo disable syntax highlighting.
• Misune, a popular python markdown library: Splits on ,, all , separated items are added to the class list in the generated HTML. Everything after the first space or tab is ignored, so rust foo has class=language-rust, and ignores foo.
• Remark, a popular JS markdown library: Splits on , and adds , separated items to class list in HTML. rust foo actually puts the text foo into the code block. <code class="rust">foo</code>.

All four treat characters other than ,, , and \t the same as alphanumeric, i.e. they don't split, and are included in the parsed language string.

It seems like there's little agreement 😅 If there are other systems, that I should take a look at, let me know.

Do I need to do anything to trigger a crater run?

[jyn514]

I'm not sure if crater would tell us anything meaningful here, actually. Rustdoc doesn't warn or error when it runs into a language string it doesn't understand, so even if there were a change in behavior we wouldn't see it on crater.

[casey]

We could see test failures if attributes stop being recognized. For example, this would currently get the should_panic attribute, but after the change wouldn't, and would fail:

```rust&should_panic
panic!();
```

It would be ideal to see if the number of tests that ran changed. If they stayed the same and nothing failed, we could be confident that there were no breakages.

[Manishearth]

@jyn514 correct, but we can push a change that makes this a hard error and then crater it

[jyn514]

Got it, thanks.

@casey can you change the PR to give an error/give an assertion failure if the new and old parsing don't match? You'll have to temporarily disable the tests. Then I can start a crater run to see how often this happens in practice.

[casey]

I think this is ready for a crater run. I modified it to parse lang strings twice, once with the new splitting scheme, and once with the old, and panic if the parsed LangString objects don't match.

[jyn514]

Thanks, looks great! @ollie27 can you take another look at #78429 (comment)? I think we'll be ready to go after that :)

@casey in the meantime do you mind squashing the commits to something fewer than 24?

[casey]

@casey in the meantime do you mind squashing the commits to something fewer than 24?

Yeah, 24 is a bit excessive for this change 😅 I squashed to one commit, and improved the commit message.

[rfcbot]

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

[dtolnay]

I've approved on behalf of withoutboats, as per rust-lang/team#526.

[rfcbot]

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.

The RFC will be merged soon.

[jyn514]

@bors r+

[bors]

📌 Commit 66f4883 has been approved by jyn514

[bors]

⌛ Testing commit 66f4883 with merge d95d304...

[bors]

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing d95d304 to master...

[casey]

Awesome! Thank you @jyn514 for helping me get this through!

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

I love how polite rfcbot is 😊

[jyn514]

You're very welcome! Thanks for sticking with it so long :)