Teach compiletest to parse arbitrary cfg options

[inquisitivecrystal]

This gives compiletest the ability to check for any cfg option that rustc sets by default for certain platforms. For instance, it is now possible to only run a test on targets where target_has_atomic="8" is true.

Resolves #87377.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[nagisa]

I don't see how this handles situations where the test specifies specific flags for rustc, e.g. like this:

// compile-flags: --target=wasm32-unknown-unknown -Ctarget-feature=+simd128
// cfg-target-feature: simd128

should probably work.

[inquisitivecrystal]

I don't see how this handles situations where the test specifies specific flags for rustc, e.g. like this:

// compile-flags: --target=wasm32-unknown-unknown -Ctarget-feature=+simd128
// cfg-target-feature: simd128

should probably work.

It doesn't. I agree that it would be lovely if that were supported. However, I'd really prefer not to implement it right now.

The current code is structured so that configuration parameters are resolved solely based on the main target, rather than any compile-flags. That means that compile-flag information isn't presently available to the section of the code that parses configurations. To add this feature, I'd have to restructure the code so that parse_cfg_name_directive somehow gets per test information. That refactoring is tangential to the main point of this change, and it deserves it's own PR.

Fortunately, there's a relatively easy workaround for that case: just check whether the feature you want is supported on the target you have requested. It's not perfect, but it's a lot more workable than checking whether the feature is supported on every individual target, having to account for the possibility that future targets with different features might be added. Thus, the feature I'm adding here is quite useful on its own.

Given all that, I don't think we should block this on adding compile-flags support.

[nagisa]

Given all that, I don't think we should block this on adding compile-flags support.

I think it is fairly important that writing a test (possibly by combining concepts seen in other tests) is easy to get correct. My view of this specific implementation is that the behaviour is not intuitive at all.

I don't mind this being a separate PR, but from what I can tell, a better integrated implementation would have to redo most of this PR either way. At the very least I want to see a tidy lint that disallows tricky combinations, if we land this feature as-is.

[inquisitivecrystal]

Given all that, I don't think we should block this on adding compile-flags support.

I think it is fairly important that writing a test (possibly by combining concepts seen in other tests) is easy to get correct. My view of this specific implementation is that the behaviour is not intuitive at all.

I don't mind this being a separate PR, but from what I can tell, a better integrated implementation would have to redo most of this PR either way. At the very least I want to see a tidy lint that disallows tricky combinations, if we land this feature as-is.

That seems fair. This isn't any worse than the other only and ignore directives, but one could say that the behavior is confusing there too. When I have a chance, I'll have a go at actually supporting it, and if that seems like too much work I'll add the tidy check. I'll also implement Mark-Simulacrum's comments at the same time.

Also, you're right that a PR that did that would have to rewrite a fair portion of this: I'd estimate about 50%.

[inquisitivecrystal]

I think I'm going to temporarily close this. I am working on it again though. Hopefully I'll have progress soon, and I'll reopen then.

[inquisitivecrystal]

I feel like this is worth reopening for another round of comments. I believe that I've implemented all of the feedback I previously received. I'm sincerely sorry that this took so long. It's been a busy few months, but that's no excuse.

Things that are missing from this implementation are caching of the results and testing. I'd like hear people's thoughts about whether caching is necessary and how to go about it; I'll leave a comment on the relevant area of code. I think testing would likely be advisable, but I'm still working out the best way to go about it. I'm also clarifying some of the relevant details with someone else at the moment.

One other specific thing that I wanted to draw attention to is that I changed the code to refer to the sort of thing that goes after an ignore- as a "prop", for property. They were previously called cfgs. The problem was that this file uses cfg and config and Config to refer to so many things that I was finding it difficult to reason about which was which. Additionally, although there are some analogies between these properties and #[cfg(...)] directives, they aren't all the sort of thing that would go in that kind of directive. Feedback is welcome.

Please feel welcome to point out anything that I've missed, and of course any new comments as well. I'm sure there are places where the code could be improved. CC @Mark-Simulacrum and @nagisa, who reviewed this previously.

[inquisitivecrystal]

The cfg_data could be cached somewhere around this point. Doing so could presumably result in some small performance saving for the case where target == self.target, which is likely to come up with the most frequency. I'm not sure whether this is worthwhile, or if so, how to go about it.

As an amusing anecdote, I actually fiddled around with a version where all of the targets had their information calculated in advance and it was stored in Config. I figured that computing all of that would only take a few extra seconds. It took me a few days to figure out why doing so was causing a one third to one half regression in testing times. It turns out that Config gets cloned a fair number of times, and by the end was taking enough memory that it was vastly increasing the duration of forking the threads to execute the tests. At least, that's what it was as far as I can tell. In any case, not doing that again!

[inquisitivecrystal]

I added tests!

[Mark-Simulacrum]

I think I agree with @nagisa that this should respect compile-flags -- or, at least, abort if any such flags are specified so that it can't be combined with compile-flags by accident in a way that leads to misleading behavior. Is that implemented in the current version of the PR?

I'll also note that I feel a little uncertain about the exact shape of these options. It feels like they're an improvement over the manual listing of various targets to ignore, but are a little weak in terms of readability -- ignore-cfg: ... doesn't obviously suggest ignore if this cfg is true to me. Maybe ignore-if-cfg would be better here, not sure.

[inquisitivecrystal]

I think I agree with @nagisa that this should respect compile-flags -- or, at least, abort if any such flags are specified so that it can't be combined with compile-flags by accident in a way that leads to misleading behavior. Is that implemented in the current version of the PR?

Well, I thought it was, but... I only made it respect the --target compile-flag. Looking back at nagisa's example, that was a mistake. -Ctarget-feature is relevant as well, for instance. I'm not sure why someone would set a target feature and then check for it, but it should probably work if they do. I'll change it to just use all of the flags. The target flag will still need special-casing, so this is going to get interesting.

I'll also note that I feel a little uncertain about the exact shape of these options. It feels like they're an improvement over the manual listing of various targets to ignore, but are a little weak in terms of readability -- ignore-cfg: ... doesn't obviously suggest ignore if this cfg is true to me. Maybe ignore-if-cfg would be better here, not sure.

I feel uncertain about that too. ignore-cfg: ... isn't the clearest. ignore-if-cfg seems like a clarity improvement, but still not ideal. Perhaps I should solicit opinions on Zulip? Programmers are terrific at bikeshedding. :)

[bors]

☔ The latest upstream changes (presumably #94134) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@inquisitivecrystal any updates on this?

[inquisitivecrystal]

@inquisitivecrystal any updates on this?

I apologize for the delay. I know this PR has been hanging around for ages at this point. I'm still planning on finishing everything up sooner or later, but it may be a month or so before I have time to work on it again, as I'm extremely busy IRL. Unfortunately, the "proper" fix is so complicated that I don't know how to implement it cleanly (and the difference wouldn't be relevant all that often, in any case). When I do implement this, I'm planning to go with a simpler implementation (using tidy to reject confusing combinations, rather than trying to support them).

In the meantime, I don't know how to handle the PR. We can close it so WG-triage doesn't have to deal with keeping track of it or leave it open if that's more convenient. I'm even happy to let someone else take over, if there's someone who wants to work on that. In any case, I once again apologize for how long it's taken me to handle this.

[Dylan-DPC]

Nothing to apologise about. I am going to close it for now, but feel free to reoopen it if nobody has got to it yet.