-
Notifications
You must be signed in to change notification settings - Fork 14k
compiletest: Do the known-directives check only once, and improve its error message #148510
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
Conversation
This allows every check to be a single hashtable lookup instead of a linear scan.
This also incorporates some requested improvements to the error message for unknown directives.
|
Some changes occurred in src/tools/compiletest cc @jieyouxu
|
|
This was originally going to wait until after #147955, but then Ralf and I both got annoyed by the error message, and improving the error message would be a lot harder without extracting the check first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Eventually, I would like us to move towards a late rejection, where if we fail to match the directive against any registered handle, then that constitutes an unknown directive. But in the meantime, making this "stop the bleeding" logic nicer is also a good improvement.
|
We can land this first. |
Rollup of 4 pull requests Successful merges: - #148248 (Constify `ControlFlow` methods without unstable bounds) - #148285 (Constify `ControlFlow` methods with unstable bounds) - #148510 (compiletest: Do the known-directives check only once, and improve its error message) - #148655 (Fix invalid macro tag generation for keywords which can be followed by values) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148510 - Zalathar:known-directives, r=jieyouxu compiletest: Do the known-directives check only once, and improve its error message This PR is a combination of three changes: - Store the list of known directives in a set, so that checking a directive name doesn't require a linear scan - Extract the known-directives check out of `iter_directives` and do it only once, instead of multiple times per file - Improve the error message for unknown directives, [as requested on Zulip](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Compiletest.20directive.20parsing.20error/with/553598083) The change to error messages is fused with the extraction, since doing it independently would have been more awkward. --- ## Before ```text Testing stage1 with compiletest suite=coverage mode=coverage-map (aarch64-apple-darwin) errors encountered during EarlyProps parsing: /Users/stuart/Dev/rust/rust/tests/coverage/trivial.rs 2025-11-05T01:55:46.440012Z ERROR compiletest::directives: /Users/stuart/Dev/rust/rust/tests/coverage/trivial.rs:2: detected unknown compiletest test directive `add-core-stubs` thread '<unnamed>' (36268582) panicked at src/tools/compiletest/src/directives.rs:72:13: errors encountered during EarlyProps parsing note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` ## After ```text Testing stage1 with compiletest suite=coverage mode=coverage-map (aarch64-apple-darwin) thread '<unnamed>' (36270772) panicked at src/tools/compiletest/src/lib.rs:876:9: directives check failed: ERROR: unknown compiletest directive `add-core-stubs` at /Users/stuart/Dev/rust/rust/tests/coverage/trivial.rs:2 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ```
This PR is a combination of three changes:
iter_directivesand do it only once, instead of multiple times per fileThe change to error messages is fused with the extraction, since doing it independently would have been more awkward.
Before
After