Match hyphen in multi-revision comment matchers

[tgross35]

Currently, the matcher //[rev-foo,rev-bar]~ does not get selected by the regex. Change the matcher to include -.

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[tgross35]

Reasoning: https://github.com/rust-lang/rust/pull/123816/files#diff-0238262528c07a864b8aa49d6d3aa989c49b045a012b1a8e2c491c84e3ccac3b wasn't matching anything labeled //[legacy,verbose-legacy]~^, @michaelwoerister suggested fixing the regex.

[compiler-errors]

r? @jieyouxu

[michaelwoerister]

The new regex looks good, @tgross35. Would you mind adding a unit test for the parse_expected function that covers the basics? See this for examples of other unit tests in compiletest:
https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/header/tests.rs

[jieyouxu]

Making revision names accept - makes sense to me, but I don't remember if the dash messes with crate names generated by revisions, could you double check if something like

//@ revisions: hello-world
//@[hello-world] run-pass
fn main() {}

isn't borked?

[jieyouxu]

IIRC revision names are used in crate names for artifact names and output, I don't immediately recall if we normalized it to become underscore.

[jieyouxu]

Does this regex mean test.name is accepted, along other non-alphanumeric characters? If so, that will break anything using revision name as crate names unless you also perform "name mangling" to normalize the non-alphanumeric characters

[jieyouxu]

This is another case of "silently failing" in compiletest where it really really needs to loudly report that your directives don't work and won't work as expected...

[michaelwoerister]

#123816 is modifying a test case that already uses - in a revision name. It seems to work except for when used in error annotations.

However, generally restricting revision names to valid ASCII-only identifiers seems reasonable to me (as long as we give a good error message when compiletest encounters an invalid revision name 🙂)

[jieyouxu]

#123816 is modifying a test case that already uses - in a revision name. It seems to work except for when used in error annotations.

However, generally restricting revision names to valid ASCII-only identifiers seems reasonable to me (as long as we give a good error message when compiletest encounters an invalid revision name 🙂)

I think that only recently works because I added some explicit crate name mangling for revisioned run-rustfix: #123601, which modifying the accepted revision name regex luckily hits. But that only handles . and - I want to say? So if the revision name contains a $ it probably breaks?

[jieyouxu]

However, generally restricting revision names to valid ASCII-only identifiers seems reasonable to me (as long as we give a good error message when compiletest encounters an invalid revision name 🙂)

I think we could just only accept revision names that are crate-name-like (that is, restrict revision names to only consist of alphanumeric + underscore characters), and report an error otherwise. I don't think we want to perform any weird crate name mangling, because e.g. if you naively mangle rev-name as rev_name, then the corresponding crate name will conflict with another revision rev_name. People usually don't write revision names like this, but yeah...

[michaelwoerister]

I agree, the simpler the better.

[tgross35]

Having - in revision names seems reasonably common, just based on a quick search https://github.com/search?q=repo%3Arust-lang%2Frust+%2F%40.*revisions%3A.*-%2F&type=code, so it seems like that should work without any special mangling. I just changed the regex to match everything so if all revision names get matched even if invalid, and let them fail elsewhere rather than silently be skipped (doesn’t help if the revision doesn’t exist, but that is a different problem…).

What would you prefer for this PR? I can leave it as-is and add a test, or change back to the original pattern and just add -.

[jieyouxu]

Having - in revision names seems reasonably common, just based on a quick search https://github.com/search?q=repo%3Arust-lang%2Frust+%2F%40.*revisions%3A.*-%2F&type=code, so it seems like that should work without any special mangling. I just changed the regex to match everything so if all revision names get matched even if invalid, and let them fail elsewhere rather than silently be skipped (doesn’t help if the revision doesn’t exist, but that is a different problem…).

What would you prefer for this PR? I can leave it as-is and add a test, or change back to the original pattern and just add -.

Huh, I suppose then it does work with -s. In that case, let's respect the existing accepted revision names (alphanumeric + _ + -), but just update the error annotation regex to accept _ and - in addition to alphanumeric characters only (and not any character). Could you also enforce that revision names can only contain alphanumeric + _ + -, and report an error if revision names don't match the regex? A test is also desirable to verify our behavior here.

[jieyouxu]

But otherwise it seems like an oversight that revisions accept - but not the error annotations (lol). What we really should do in compiletest (in the long term) is to have the revisions parsing be strict about what revision names it accept. Thanks for the fix, helping compiletest be more self-consistent is very helpful.

[tgross35]

I changed this PR to just do the simplest thing and match - in addition to what was there before. I plan on refactoring #123882, which I think would be a better place to check that the name is valid.

Since there is no new rejecting behavior, is this okay as-is or do you want a test still? (if so, what would that look like - just a dummy UI test?)

[jieyouxu]

Since there is no new rejecting behavior, is this okay as-is or do you want a test still? (if so, what would that look like - just a dummy UI test?)

Could you still add a unit test for parse_expected checking that a revision with a name containing - is accepted? I realize that uhh the error reporting here is less than ideal, but for the time being having a regression test might remind future reworks in terms of what revision names can be accepted.

[tgross35]

Alrightie I added a simple test, let me know if that looks fine.

[tgross35]

Tidy failed so it didn't push before, should be up to date now

[jieyouxu]

Looks good to me, thanks for the patch!

@bors r+ rollup

[bors]

📌 Commit 282488c has been approved by jieyouxu

It is now in the queue for this repository.

[tgross35]

Thanks!

[michaelwoerister]

Thanks for getting this fixed, everyone!