compiletest has some parsing footguns around revisions
@jieyouxu is already working on this.
Since Apr 11, 2024.
- Dominant language
- Rust
- Stars
- 119k
- Forks
- 16.1k
- PR merge metrics
- PR metrics pending
Description
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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.