Our use case (DR conformance testing) demands something along the lines of an // expected-maybe-no-diagnostics magic comment, that works like // expected-no-diagnostics but doesn’t trigger an error if diagnostics are actually expected further on in the file. We’ll be equally well served by a -cc1 option of the same effect. The main question is which way this functionality should be introduced? Or even in both ways simultaneously.
It should be noted that even when enabled this feature is not going to compromise VerifyDiagnosticConsumer strictness in any way that introduces false positives or false negatives into tests that rely on it.
Use case
We’re working on the C++ defect report test suite. Currently, test cases are grouped by 100 in a file (example), which makes sense due to the sheer number of DRs against the C++ Standard (2608 and counting), while a significant part of them does not require a test or require couple of lines to test. Namespaces almost provide a sufficient level of isolation for the purposes of grouping those tests under a single file. But recently we came across a situation leading to nasty interference between tests, which is the reason D148433 is stuck. A reduced example is available on godbolt. In short, Clang skips over template instantiation if an unrecoverable error was encountered earlier in the file, which prevents it from issuing diagnostics that otherwise would be issued.
While useful for general user experience, such behavior is undesirable and surprising for conformance tests (which DR tests are a part of). So we’d like to improve test isolation while not compromising the succinct notation used to specify DR statuses. We’re going to rely on the split-file utility to split grouped test files into individual files per test, and extend lit to avoid duplicating RUN commands for each test (we have a RUN command per each Standard revision) (patch). Here’s a comparison between old layout and new layout: link.
With test cases individually compiled and checked by VerifyDiagnosticConsumer, tests that doesn’t expect any diagnostic are not grouped together with tests that do anymore. Sprinkling // expected-no-diagnostics seems an obvious solution, but dr5xx.cpp alone contains 27 tests that would require such marking. Instead of being actually useful, those comments are going to become noise for the sake of taming VerifyDiagnosticConsumer. This doesn’t seem sound, so we want a way to adjust VerifyDiagnosticConsumer a bit for our test suite, and do that in a single place or handful of places.
Other usages
Non-DR conformance tests could also leverage this infrastructure in order to e.g. group tests for individual paragraphs under a single file, simplifying directory structure.
Thank you for continuing to try to improve the DR conformance testing situation! In general, I think using the split-file approach is going to help get us hermetic tests without adding significant overhead for people writing the tests, so this is heading in a good direction IMO.
In terms of whether to go with a new verifier comment or with a command line switch, my (very slight) preference is for the comment. A cc1 flag would certainly work, but is a bit more heavy-handed of an approach for something that I expect we’d mostly only want to use for DR testing and perhaps modules/serialization testing as opposed to a more general feature. I think having a comment for the verifier suffices.
I was a little bit uncomfortable at first of the idea of having a “maybe this is expected, maybe it’s not” kind of comment. However, the only situation where I think this would come back to bite us is when adding additional tests to an existing file already using this marking and the additional tests should have diagnostics but don’t (for whatever reason). In that case, the test will pass when it would have otherwise failed. But this seems like an edge case that we can live with.
CC @zygoloid@dblaikie@cjdb for some extra opinions given this is a question of which approach to take.
I agree that this is both theoretically possible and something we can live with. That’s how this situation would look like in practice when applied to conformance testing:
There’s no concept of XFAIL tests there. You have test that pass, and you have some more tests that pass, but have FIXME comments inside.
So if your new test doesn’t produce a diagnostic it should, and you forgot about that, what you actually is going to missing is a FIXME comment.
This would mean that instead of setting the status to non-conformant, you set it to conformant. At this point I’d question if author understands what they’re testing.
Then we have reviewer(s). If they let it through, I’d have the same question for them as well.
I see this happening in two cases:
a) underqualified author and underqualified reviewer; both are interested enough in conformance testing, though;
b) Standard wording that could be misunderstood easily enough to pass through qualified person unnoticed.
I don’t see how something purely technical like requiring // expected-no-diagnostics can help us with those severe issues on organizational level.
But again, this is conformance testing perspective. Your scenario could be more realistic for other use cases.
Yeah, mixed feelings similar to your own, but nothing stronger. It doesn’t seem like it’d be that big a deal to have an explicit annotation on each micro-test that either shows the expected diagnostic, or an explicit statement that none is expected.
But if you folks who work on this more feel that’d be too noisy, I wouldn’t hold it against you to add this feature to the verify diagnostic consumer.