Rustdoc doctest attribute splitting is probably too liberal

[casey]

I was thinking about proposing a new doctest attribute of the form name=TEXT that would allow giving a documentation tests names which would be printed when they run.

I was looking at what it would take to implement this change, so I checked out the way that doctest attributes are parsed from markdown info strings, and found that they are split using the following code:

let tokens = string.split(|c: char| !(c == '_' || c == '-' || c.is_alphanumeric()));

("info string" is the name that common mark and GFM use for the text that follows a ```.)

This conflicts with the name=foo syntax that I was hoping to use, since the attribute would be split on the =.

I think this behavior also conflicts with how people think the feature works, since a few people told me that attributes are split on ,, which is also what Clippy does.

My feeling is that the current attribute splitting is probably too liberal in what it accepts, and prevents otherwise desirable attributes, such as attributes of the form foo=bar, or attributes with more freeform text, from being possible.

A few thoughts:

• Is this worth worrying about at all? It's unfortunate, but it's not a huge deal, since workarounds like having attributes of the form foo-bar are possible.

• If nobody (according to crater) is relying on the liberal splitting, would it be acceptable to consider this to be a bug, and change it to just split on ,? Of course, this could break non-public code.

• Would it be worth doing as part of an edition? It would be pretty easy to automatically transform info strings from being whatever-split to being comma split as part of an edition upgrade. It would also be easy to create a lint or warning for it, just check that the current splitting rules and comma splitting produce the same attributes, and warn if they don't.

[jyn514]

cc @rust-lang/rustdoc: this proposes changing the doctest attribute parsing to only split on ,, not anything else. Personally I'm in favor - what do you think?

[GuillaumeGomez]

The handling of ignore will need a small update but otherwise sounds good to me. Also, being able to name a doctest is a good idea.

[casey]

I'm happy to submit a PR to change this, but I wonder if perhaps this is something that should be brought to the attention of more people, considering the possibility for breakage.

[jyn514]

@casey go ahead and make the PR and we can start an FCP for the breaking change there.

[casey]

@jyn514 Sounds good, I just opened #78429 with the change and some notes.