Don't emit rustdoc::broken_intra_doc_links for GitHub-flavored Markdown admonitions like [!NOTE]

[lolbinarycat]

fixes #141866

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[fmease]

Some final nits.

[fmease]

@GuillaumeGomez What do you think about this — frankly — compatibility hack? I'm not super happy that we need to this but that's the real world for you ^^'. Should we FCP this?

[GuillaumeGomez]

We officially support commonmark, so I don't think it's a good idea here. We could however improve the warning to say "this is github-flavored markdown, which is different than the one used in rustdoc: commonmark".

[fmease]

The problem is that there's no workaround for the user as far as I can tell. There's nothing they could do to their repo README.md that would fix this warning. (Well, they could #![allow(rustdoc::broken_intra_doc_links)] at the … crate root? That's not desirable since it would disable that lint for the whole crate when they just wanted to disable it for their README)

[GuillaumeGomez]

Oh I see. Hum... Should we make a specific lint for it? Like github_flavored_markdown? Or am I just too picky here and just going through with this PR is good enough?

[fmease]

Or am I just too picky here and just going through with this PR is good enough?

No, definitely not too picky. Your position is absolutely fair because it could open the floodgates to a never-ending stream of compatibility hacks for GitHub-flavored markdown (esp. since it keeps getting extended). I wonder if we already have hacks like this in rustdoc?

Should we make a specific lint for it? Like github_flavored_markdown?

That sounds like a good solution/compromise! I'm all for a new lint. We probably don't want to have github in its name since I don't think we want to tie ourselves to that platform like that (it might not exist in 10 years time / another platform might become the new 'default' platform with their own flavor of Markdown). Maybe other_flavored_markdown or differently_flavored_markdown (bikesheddable names)?

[GuillaumeGomez]

non_commonmark_flavored_markdown? =D

[fmease]

Or simply rustdoc::nonstandard_markdown (borrowing the adjective from nonstandard_style, https://doc.rust-lang.org/rustc/lints/groups.html#lint-groups), maybe that's too vague tho.

Ok, enough of the bikeshedding. Let's wait for binarycat :)

[fmease]

Unfortunately, even a new lint wouldn't solve the "lint level issue":

#![allow(rustdoc::nonstandard_markdown)] // today: broken_intra_doc_links
#![doc = "> [!NOTE]"] // stand-in for `include_str!("path/to/README.md")`
                      // this one we want to allow
#![deny(rustdoc::nonstandard_markdown)] // today: broken_intra_doc_links
#![doc = "> [!NOTE]"] // this one we don't want to allow

The lower deny overwrites the upper allow because they're at the same module level, so we still flag both.

😩😖

Time for //! <!-- #![allow(rustdoc::nonstandard_markdown)] --> or #![doc(allow(rustdoc::nonstandard_markdown))] /s

[lolbinarycat]

My reasoning for adding this logic is it is adding a simple heuristic to a lint that already uses a bunch of heuristics.

I have a number of problems with a nonstandard_markdown:

1. the name is simply incorrect. rustdoc uses it's own flavor of markdown, from obvious domain-specific things like intra-doc lints, to benign things like footnotes that are not actually in the commonmark spec.
2. this would be way more likely to lead to more logic for unsupported markdown being added, as people report "false negatives" for the lint.
3. commonmark, similar to html5, is defined in such a way that every document has an interpretation, so it is not always clear what is intended as using non-standard features. * [x] y could either be a task list, or a reference link to x.

I think if any lint should be added, it should be rustdoc::broken_link_references, as that seems like something that would be desirable to deny in CI in order to make sure no typos slip through, and a decent number of issues get filed complaining about broken references not being denied. This would need to be an allow-by-default lint, at least initially, in order to not cause massive ecosystem churn.

[lolbinarycat]

addressed feedback from code review

@rustbot review

cc @fmease @GuillaumeGomez

[fmease]

r=fmease,GuillaumeGomez if GuillaumeGomez also approves of this unfortunate compatibility hack (round two).

Ofc we could also poll this but I'm not sure how long it would take to reach all members (also, I dislike rfcbot poll bc it doesn't discern between "disapprove" and "haven't seen yet").

[GuillaumeGomez]

Well in this case it's not so much about markdown flavor but rather that we shouldn't try to resolve an intra-doc link starting with ! (and which isn't only !).

[GuillaumeGomez]

Thanks everyone!

@bors r=fmease,GuillaumeGomez rollup

[bors]

📌 Commit c022ed9 has been approved by fmease,GuillaumeGomez

It is now in the queue for this repository.