[kantnero]

View all comments

While documenting diagnostic::do_not_recommend and diagnostic::on_unimplemented, #[doc(attribute = "...")] errored cause the current implementation only support built-in attributes. #157604 (comment)

This implementation splits the value at :: and check tool against registered_tools and that the other half is a valid identifier.

This PR was tested by documenting namespace attribute and running ./x test library/core --doc to be sure no error was encountered.

Should close: #163543
Disclosure: I used LLM to help understand the attribute parsing code and for an initial sketch of the check, which I rewrote and tested myself

r? @mejrs

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[mejrs]

Can you add this to the existing doc attribute tests in rustdoc-ui, rustdoc-html etc?

And can you also add tests for custom tool attributes (with the register_tool attribute)? No need to make them work if they don't, I just want the current state to be covered in our tests.

[kantnero]

@rustbot ready

[mejrs]

Can you fix the CI errors?

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[kantnero]

@rustbot ready

[mejrs]

Unfortunately this creates file names like \core\attribute.diagnostic::do_not_recommend.html, but this is not a valid filename on Windows as colons are not valid in file names. So this would break rendering std docs on windows.

[kantnero]

Instead of :: maybe rustdoc could do a search and replace with _ or something else?

[mejrs]

@fmease, @GuillaumeGomez do you have any ideas on the best way forward here?

[fmease]

The most proper solution would be something like \core\tool.diagnostic\attribute.do_not_recommend.html IMO but I don't know how much would need to be touched to achieve that.

[kantnero]

This would mean stripping off the name before :: and prefixing with tool.diagnostic

This would also mean each tool attr will have a seperate dir

[mejrs]

A tool attribute can also have more segments, like #[tool::one::two]. Currently there are none where this has meaning, but it works in principle.

[kantnero]

A tool attribute can also have more segments, like #[tool::one::two]. Currently there are none where this has meaning, but it works in principle.

How should we move forward since this PR doesn't take that into account.

My suggestion:
#[diagnostic::one::two]

core\tool.diagnostic\attribute.one_two.html

In this case if there ever exist a tool attribute with more than one segment nothing has to change.

[fmease]

diagnostic::one::two would then clash with diagnostic::one_two.

A tool attribute can also have more segments, like #[tool::one::two]. Currently there are none where this has meaning, but it works in principle.

a::b::c::d is a path. If a, b & c are modules we choose to represent it using the filesystem hierarchy in rustdoc, so CRATE/a/b/c/KIND.d.html. Analogously, if a was a tool namespace & d was a doc(attribute) attribute (making b & c semanticless path segments I guess) in an ideal world / long-term I would expect us to represent the Rust path as filesystem path CRATE/tool.a/b/c/attribute.d.html (or as CRATE/a/b/c/attribute.d.html if we generally don't need to disambiguate between modules & tool namespaces; I just don't remember the name resolution rules atm).

In the short-term we can opt for a less principled approach, of course.

[mejrs]

We can also substitute with any token that isn't valid in an ident, for example
\core\attribute.diagnostic-one-two.html

[mejrs]

(not sure how this interacts with doc search, searching "diagnostic::one::two" should still get us there)

[kantnero]

(not sure how this interacts with doc search, searching "diagnostic::one::two" should still get us there)

Yes it should

[kantnero]

Hi @mejrs
How do you want to proceed with this?

[mejrs]

I'm not sure what the best solution is. I think some sort of substitution is probably the way to go but I'm not sure what followup changes are required to make everything work properly. You could go to zulip and ask the rustdoc people for advice.

[rust-bors]

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