Skip to content

Compiletest: Simplify {Html,Json}DocCk directive handling #143850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmease
Copy link
Member

@fmease fmease commented Jul 12, 2025

So much more maintainable and extensible.

r? @jieyouxu as discussed

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-rustc-dev-guide Area: rustc-dev-guide A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 12, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Comment on lines -215 to -217
if args and not args[:1].isspace():
print_err(lineno, line, "Invalid template syntax")
continue
Copy link
Member Author

@fmease fmease Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a roundabout way this used to check that the separator between the directive and its args was a space because the capture group args used to immediately follow the directive name.

@rust-log-analyzer

This comment has been minimized.

LINE_PATTERN = re.compile(
r"""
//@\s+
(?P<negated>!?)(?P<cmd>[A-Za-z0-9]+(?:-[A-Za-z0-9]+)*)
(?P<negated>!?)(?P<cmd>.+?)
[\s:]
Copy link
Member Author

@fmease fmease Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically had to extend the grammar of {Html,Json}DocCk directives to admit : as a legal separator besides whitespace.

Otherwise it would've either been included in the args if I hadn't relaxed cmd's grammar (so //@ has: ... would've meant [has, : ...]) which would've lead to bad diagnostics down the line like "unexpected argument" (because : ... would've been further split into [:, ...] by shlex) or it would've been accepted by compiletest but rejected ignored by HtmlDocCk (remember, we can't reject any unknown or malformed directive in HtmlDocCk since it no longer knows if it's a compiletest directive or not).

Disregarding the technical reasons, {Html,Json}DocCk directives should be extensions to regular compiletest directives, so it only makes sense that : is allowed as a separator.

@fmease fmease force-pushed the comptest-simp-docck-handling branch from c9870d8 to 48c3962 Compare July 12, 2025 17:58
@fmease fmease marked this pull request as draft July 12, 2025 18:04
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2025
@fmease fmease force-pushed the comptest-simp-docck-handling branch from 48c3962 to 60471e3 Compare July 12, 2025 18:23
@fmease fmease marked this pull request as ready for review July 12, 2025 18:23
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2025
@rustbot

This comment was marked as off-topic.

@fmease fmease force-pushed the comptest-simp-docck-handling branch 2 times, most recently from 4d6875e to 1be2c85 Compare July 12, 2025 18:25
@fmease fmease force-pushed the comptest-simp-docck-handling branch from 1be2c85 to 917171a Compare July 12, 2025 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-rustc-dev-guide Area: rustc-dev-guide A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants