Skip to content
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

compiletest has some parsing footguns around revisions #123765

Open
fmease opened this issue Apr 11, 2024 · 1 comment
Open

compiletest has some parsing footguns around revisions #123765

fmease opened this issue Apr 11, 2024 · 1 comment
Assignees
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented Apr 11, 2024

I've chatted with @jieyouxu and we've come to the conclusion that it's worthwhile to track compiletest issues even though we plan on fully migrating to ui_test eventually since that might still take a while.


When parsing the revisions header / directive, compiletest naively splits the payload by whitespace leading to the bizarre situation that //@ revisions: foo, bar gets understood as declaring the two revisions foo, (notice the trailing comma!) and bar.

We should either throw an error (my favored solution) or permit , to be valid separator next to whitespace (less desirable in my opinion).

If we go with the first approach we should probably go all-in and restrict revision names to the regex [[:alpha:]_\-][[:alnum:]_\-]* (*) (ASCII-only or Unicode, shouldn't matter) and we can probably also emit the hint “commas not permitted, use whitespace” if we stumble upon a comma.

(*) Restricting the grammar of revision names also helps with portability. Windows is way stricter about file paths than *nix OSes and since compiletest generates files of the form <STEM>.<REVISION>.<STDSTREAM> revisions containing special characters may lead to tests passing locally for a contributor working on a *nix machine but failing on a Windows machine. CI would catch that but still ^^'

Similarly, compiletest permits whatever garbage you put between [ and ] in //@[...] DIRECTIVE, even //@[] is allowed. We should probably restrict the content to be identifier-like, too.

@fmease fmease added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Apr 11, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 11, 2024
@fmease fmease added P-low Low priority and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 11, 2024
@jieyouxu jieyouxu self-assigned this Apr 11, 2024
@fmease
Copy link
Member Author

fmease commented Apr 22, 2024

Furthermore, if I remember correctly, in //@[a,b] xyz the revision gets parsed as a single revision literally called a,b while in error annotations like //[a,b]~ ERROR message, it gets parsed as the two revisions a and b.

In the first case we should either emit a better error or actually parse multiple revisions, too (it's kinda useful :P).

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-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Development

No branches or pull requests

3 participants