compiletest: Stricter parsing of //@ normalize-* headers
#126370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-testsuite
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
Yes, apparently some directives require the colon or else it silently fails cc #123760. If you notice compiletest directive handling related wonkiness, please do file specific issues and tag them with A-compiletest so at least it can be tracked and we are aware that certain footguns exist. |
2 tasks
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 13, 2024
Rollup of 6 pull requests Successful merges: - rust-lang#126347 (Simplify `try_*`'s signature on `Iterator`) - rust-lang#126348 (Improve error message if dependency installation in tidy fails) - rust-lang#126366 (Add a new trait to retrieve StableMir definition Ty) - rust-lang#126370 (compiletest: Stricter parsing of `//@ normalize-*` headers) - rust-lang#126379 (interpret: update doc comment for find_closest_untracked_caller_location) - rust-lang#126384 (add tracking issue for is_none_or) Failed merges: - rust-lang#126388 (const-eval: make lint scope computation consistent) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 13, 2024
Rollup merge of rust-lang#126370 - Zalathar:normalize, r=oli-obk compiletest: Stricter parsing of `//@ normalize-*` headers I noticed some problems with the existing parser for these headers: - It is extremely lax, and basically ignores everything other than the text between two pairs of double-quote characters. - Unlike other name-value headers, it doesn't even check for a colon after the header name, so the test suite contains a mixture of with-colon and without-colon normalization rules. - If parsing fails, the header is silently ignored. The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.
This was referenced Jun 21, 2024
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 10, 2024
Require a colon in `//@ normalize-*:` test headers The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds. This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory. (Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.) Addresses one of the points of rust-lang#126372.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 11, 2024
Require a colon in `//@ normalize-*:` test headers The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds. This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory. (Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.) Addresses one of the points of rust-lang#126372.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 11, 2024
Require a colon in `//@ normalize-*:` test headers The previous parser for `//@ normalize-*` headers (before rust-lang#126370) was so lax that it did not require `:` after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds. This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory. (Because the normalization parser only runs *after* the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.) Addresses one of the points of rust-lang#126372.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed some problems with the existing parser for these headers:
The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.