rust-lang / rust-lang/rust

compiletest has some parsing footguns around revisions

Open
#123,765 3 comments 0 reactions 1 assignee View on GitHub

@jieyouxu is already working on this.

Since Apr 11, 2024.

A-compiletest A-testsuite C-bug P-low T-compiler
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.