Add new invalid_markdown_table rustdoc lint

[GuillaumeGomez]

View all comments

Fixes #159186.

r? @Urgau

[GuillaumeGomez]

Applied comments.

[Urgau]

Implementation looks good to me.

View changes since this review

[Urgau]

2nd try. Let's do an FCP.

unescaped_pipe_in_table_cell details

unescaped_pipe_in_table_cell

This lint is warn-by-default. It detects unescaped pipes (|) in table rows which
lead to some row cells being ignored. For example:

//! | col1 |
//! | ---- |
//! | `code_with(|arg| arg)` |

Which will give:

error: table row has too many columns
  --> $DIR/unescaped_pipe_in_table_cell.rs:5:18
   |
5  | //! | `code_with(|arg| arg)` |
   |                  ^ help: any content after this column divider is discarded
   |
   = help: to escape `|` characters in tables, add a `\` before them like `\|`
note: the lint level is defined here
  --> $DIR/unescaped_pipe_in_table_cell.rs:1:9
   |
1  | #![deny(rustdoc::unescaped_pipe_in_table_cell)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@rfcbot merge rustdoc-internals

[rust-rfcbot]

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

• @GuillaumeGomez
• @Manishearth
• @Urgau
• @camelid
• @fmease
• @lolbinarycat
• @notriddle
• @yotamofek

Concerns:

• overly specific lint name resolved by Add new invalid_markdown_table rustdoc lint #159583 (comment)

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.

[camelid]

Wow TIL that this is how GFM works. Very strange design...

View changes since the review

[GuillaumeGomez]

Yeah I was super confused as well.

[notriddle]

Minor nits about the implementation, but massive approval for the lint in concept!

[camelid]

Definitely in favor of the lint itself, but I wonder if we should have a more generic lint name and then we group together this and any related (current or future) lints about Markdown gotchas? E.g. we have rustdoc::invalid_html_tags that groups together anything related to that, including (soon: #158709) issues when HTML is interleaved with Markdown.

For example, it could be rustdoc::invalid_markdown, or rustdoc::markdown_gotchas, or a Better Name (TM). I could even imagine us renaming rustdoc::invalid_html_tags to something more general like rustdoc::invalid_markdown and including this as part of it.

On a related note, my feeling with #158709 is that additions of new cases to an existing warn-by-default lint do not require FCP since they are not part of stability guarantees, but please let me know if you disagree.

[camelid]

@rfcbot reviewed
@rfcbot concern overly specific lint name

[GuillaumeGomez]

@camelid: Very fair point. I think having a lint group for all such lints is a good idea. As for the current lint name, I'll rename it as invalid_markdown_table.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-timer]

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (ff4bc60): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.5%, 0.6%]	2

Max RSS (memory usage)

Results (primary 7.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	7.8%	[7.8%, 7.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.8%	[7.8%, 7.8%]	1

Cycles

Results (primary -2.9%, secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 471.419s -> 469.832s (-0.34%)
Artifact size: 400.34 MiB -> 400.27 MiB (-0.02%)

[JonathanBrouwer]

Caused the perf regression in the rollup.
This does more work because it's a new lint, and the regression is quite minor, so we probably just have to accept this
@rustbot label: +perf-regression-triaged