[clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Endill]

Summary

This patch introduces a // expected-maybe-not-diagnostics that silence warnings about missing // expected-no-diagnostic, but doesn't conflict with neither // expected-error directives nor // expected-no-diagnostics.

I go into details in the Discourse thread about why we need this, and why we need this in the form implemented.

[aaron.ballman]

This patch introduces a // expected-maybe-not-diagnostic that silence

typo in the summary, it introduces // expected-maybe-no-diagnostics

To save other reviewers some clicking around, the reason why this is being proposed is because we want to use the split-file utility to separate monolithic DR tests into individual test cases to keep them hermetic. We could require each individual test within the greater file to specify // expected-no-diagnostics, but that adds quite a bit of noise to the test file due to how often those comments need to repeat. The idea behind this patch is that we can use one // expected-maybe-no-diagnostics marking in a header file that is force included on the RUN lines for the file. This way, each split off file says "if you get no diagnostics, that's fine". In the case where the file legitimately has no diagnostics, we pass that test. But in the case where the file does expect diagnostics, we avoid the "expected no diagnostics, but got diagnostics" failure mode. This allows us to continue to group DRs by number instead of by whether they expect diagnostics or not.

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

279

[Endill]

Add missing comma after enum item

[Endill]

Thank you for providing a nice explanation!

[jdenny]

Thanks for working on this. I've been wanting something like it too. In downstream work, I've used a hack that seems to accomplish the same thing but probably shouldn't: expected-error 0 {{}}.

Anyway, please add documentation for the new directive here: https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html#details

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

This continue skips the prefix checking below, which is important when there are multiple prefixes active (e.g., -verify=foo,bar). That is, any old BOGUS-maybe-no-diagnostics will be effective then.

clang/test/Frontend/verify-maybe-no-diagnostics.c

39

Please test this case:

#ifdef TEST_C3
// expected-maybe-no-diagnostics
#error test_c3
#endif

That is, does Clang actually fail in this case as expected because there's an error and no corresponding expected-error directive? Or does it ignore the error because of the expected-maybe-no-diagnostics directive?

52–53

So expected-no-diagnostics overrides expected-maybe-no-diagnostics. In your use case, omitting one or the other is not always possible?

105

This diagnostic is confusing. Should we add "except 'expected-maybe-no-diagnostics'"?

[Endill]

Address feedback
Docs contributed by @aaron.ballman

[Endill]

Thank you for the review!

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

This should be fixed now. Thank you for spotting this!

clang/test/Frontend/verify-maybe-no-diagnostics.c

39

Added C2 and C4 tests that should cover that.

That is, does Clang actually fail in this case as expected because there's an error and no corresponding expected-error directive? Or does it ignore the error because of the expected-maybe-no-diagnostics directive?

It works like there's no // expected-maybe-no-diagnostics. That is, failing because there's no matching expected-error directive for an error produced.

52–53

Yes, it works the way you describe.
In our case, // expected-maybe-no-diagnostic comes into hundreds of test cases from force include. It's very much possible for us to ensure that only one of them is present, by banning expected-no-diagnostics.

expected-maybe-no-diagnostic is by design low-priority and no-friction directive, contrary to strict nature of all other directives. That's why it could be very easily overridden.

105

As I mentioned in another comment, maybe-no-diagnostics has the lowest priority, and doesn't have strict and declarative nature, unlike any other directive. That's why it should never be expected (and ideally very rarely used).

The purpose of all the tests I added is to ensure expected-no-diagnostic doesn't affect existing directives and their interaction in any way.

[jdenny]

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

186–202

Thanks for adding documentation.

I feel that this edit makes the behavior a little clearer, and it clarifies what happens when "expected-no-diagnostics" and "expected-maybe-no-diagnostics" are combined.

Also, the original text had:

but they do not fail automatically due to a combination of "expected-no-diagnostics" and "expected-*" within the same test

That reads to me like it's ok to combine "expected-no-diagnostics" and "expected-*" directives specifying diagnostics. Hopefully this edit clarifies that point.

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

Thanks for the fix. Please add a test so this bug doesn't pop up again.

472–473

Shouldn't expected-maybe-no-diagnostics have this too so that expected-maybe-no-diagnostics-re is skipped? Please add a test.

clang/test/Frontend/verify-maybe-no-diagnostics.c

105

I don't see how that addresses my concern. Maybe it's because, after the latest edits, phab shifted my comment to the wrong test. I was originally commenting on this:

'expected-no-diagnostics' directive cannot follow other expected directives

This message is now incorrect. expected-no-diagnostics can follow expected-maybe-no-diagnostics. What if we reword as follows?

'expected-no-diagnostics' directive cannot follow directives that expect diagnostics

[Endill]

Address feedback

[Endill]

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

186–202

Yeah, it's better the way you propose.

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

Done as A2 test

472–473

You're right, there's no good reason for us not to do the same. Though I don't like that we silently skip over not-too-sensical // expected-no-diagnostics-re. I'll prepare a follow-up patch that makes -re a hard error for those two special directives.

Tested in A3

clang/test/Frontend/verify-maybe-no-diagnostics.c

105

Sorry, I misunderstood your initial comment. It's fixed now.

[jdenny]

Thanks for addressing all my concerns.

LGTM except for a test issue I just commented on.

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

Does A2 trigger the original bug? I think there must be multiple prefixes for this fix to matter.

[Endill]

Thank you for your time!

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

I think prefix checking below works for a single prefix as well: https://github.com/llvm/llvm-project/blob/5217498dc88aa2de2b728462205ffa8b01d96cab/clang/lib/Frontend/CompilerInvocation.cpp#LL2382C47-L2382C47

[jdenny]

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

I downloaded this version of your patch: https://reviews.llvm.org/D151320?vs=on&id=525216. That version has the bug discussed in this comment thread.

I applied that patch and added test A2. A2 passed. That means A2 does not reproduce the bug it was intended to reproduce.

Then I changed A2 to have -verify=foo,bar. A2 then failed because it then reproduced the bug. (And of course with the current version of your patch, it passes because you've fixed the bug.)

Please add such a test.

[Endill]

Replace A2 test with E1-E3 tests

[Endill]

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

I replaced A2 test with more comprehensive E set of tests.

Surprisingly to me, -verify and -verify=foo,bar work quite differently because of PH.Search() call 40 lines above. I can't say I have full understanding what's going on, so I wrote a more comprehensive set of tests. Does this address your concern?

[jdenny]

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

469

Surprisingly to me, -verify and -verify=foo,bar work quite differently because of PH.Search() call 40 lines above. I can't say I have full understanding what's going on,

That line is optimized for the case of a single prefix: it ignores text without that prefix. If there are multiple prefixes, then it searches for a more general pattern and waits until the std::binary_search call below to worry about the prefix.

so I wrote a more comprehensive set of tests. Does this address your concern?

Yes, and thanks for the extra testing.

[aaron.ballman]

LGTM!

clang/include/clang/Basic/DiagnosticFrontendKinds.td

def err_verify_invalid_content : Error<
     "invalid expected %0: %1">;
 def err_verify_missing_regex : Error<
     "cannot find start of regex ('{{') in %0">;
 def err_verify_inconsistent_diags : Error<
     "'%0' diagnostics %select{expected|seen}1 but not %select{seen|expected}1: "
     "%2">;
 def err_verify_invalid_no_diags : Error<
     "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
-    "%select{'expected-no-diagnostics' directive|other expected directives}0">;
+    "%select{'expected-no-diagnostics' directive|directives that expect diagnostics}0">;
 def err_verify_no_directives : Error<
     "no expected directives found: consider use of 'expected-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 
 def note_fixit_applied : Note<"FIX-IT applied suggested code changes">;
 def note_fixit_in_macro : Note<
     "FIX-IT unable to apply suggested code changes in a macro">;

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

 /// VerifyDiagnosticConsumer expects at least one expected-* directive to
 /// be found inside the source code.  If no diagnostics are expected the
 /// following directive can be used to indicate this:
 ///
 /// \code
 ///   // expected-no-diagnostics
 /// \endcode
 ///
+/// Additionally, you can use:
+///
+/// \code
+///   // expected-maybe-no-diagnostics
+/// \endcode
+///
+/// The "expected-no-diagnostics" and "expected-maybe-no-diagnostics"
+/// directives have the same behavior except "expected-no-diagnostics" adds the
+/// restriction that no diagnostics can be specified by other "expected-*" 
+/// directives. If both "expected-no-diagnostics" and 
+/// "expected-maybe-no-diagnostics" are specified, 
+/// "expected-maybe-no-diagnostics" is ignored.
+///
+/// Because of its strictness, "expected-no-diagnostics" is almost always preferred,
+/// but there are scenarios where "expected-maybe-no-diagnostics" is convenient.
+/// For example, "expected-maybe-no-diagnostics" is useful for DR conformance tests
+/// where dozens of distinct test cases are within the same physical file but use
+/// the 'split-file' utility to split individual test cases into logical files
+/// at runtime. In that case, a header file containing "expected-maybe-no-diagnostics"
+/// can be force included into each RUN line in the physical file. The tests that
+/// expect diagostics continue to pass or fail depending on whether the correct
+/// diagnostics are emitted, but they do not fail automatically as they would
+/// if "expected-no-diagnostics" and "expected-*" were combined within the same
+/// test.
 class VerifyDiagnosticConsumer: public DiagnosticConsumer,
                                 public CommentHandler {
 public:
   /// Directive - Abstract class representing a parsed verify directive.
   ///
   class Directive {
   public:
     static std::unique_ptr<Directive>
void Reset() {
       Notes.clear();
     }
   };
 
   enum DirectiveStatus {
     HasNoDirectives,
     HasNoDirectivesReported,
     HasExpectedNoDiagnostics,
-    HasOtherExpectedDirectives
+    HasOtherExpectedDirectives,
+    HasExpectedMaybeNoDiagnostics,
   };
 
   class MarkerTracker;
 
 private:
   DiagnosticsEngine &Diags;
   DiagnosticConsumer *PrimaryClient;
   std::unique_ptr<DiagnosticConsumer> PrimaryClientOwner;

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

for (ParseHelper PH(S); !PH.Done();) {
     if (DToken.endswith("-re")) {
       D.RegexKind = true;
       KindStr = "regex";
       DToken = DToken.substr(0, DToken.size()-3);
     }
 
     // Type in initial directive token: -{error|warning|note|no-diagnostics}
     bool NoDiag = false;
+    bool MaybeNoDiag = false;
     StringRef DType;
     if (DToken.endswith(DType="-error"))
       D.DL = ED ? &ED->Errors : nullptr;
     else if (DToken.endswith(DType="-warning"))
       D.DL = ED ? &ED->Warnings : nullptr;
     else if (DToken.endswith(DType="-remark"))
       D.DL = ED ? &ED->Remarks : nullptr;
     else if (DToken.endswith(DType="-note"))
       D.DL = ED ? &ED->Notes : nullptr;
-    else if (DToken.endswith(DType="-no-diagnostics")) {
+    else if (DToken.endswith(DType="-maybe-no-diagnostics")) {
+      MaybeNoDiag = true;
+      if (D.RegexKind)
+        continue;
+    } else if (DToken.endswith(DType="-no-diagnostics")) {
       NoDiag = true;
       if (D.RegexKind)
         continue;
     }
     else
       continue;
     DToken = DToken.substr(0, DToken.size()-DType.size());
 
     // What's left in DToken is the actual prefix.  That might not be a -verify
     // prefix even if there is only one -verify prefix (for example, the full
     // DToken is foo-bar-warning, but foo is the only -verify prefix).
     if (!std::binary_search(Prefixes.begin(), Prefixes.end(), DToken))
       continue;
 
+    if (MaybeNoDiag) {
+      if (Status == VerifyDiagnosticConsumer::HasNoDirectives)
+        Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+      continue;
+    }
+
     if (NoDiag) {
       if (Status == VerifyDiagnosticConsumer::HasOtherExpectedDirectives)
         Diags.Report(Pos, diag::err_verify_invalid_no_diags)
           << /*IsExpectedNoDiagnostics=*/true;
       else
         Status = VerifyDiagnosticConsumer::HasExpectedNoDiagnostics;
       continue;
     }

clang/test/Frontend/verify-maybe-no-diagnostics.c

+// RUN: %clang_cc1 -DTEST_A1 -verify %s
+// RUN: not %clang_cc1 -DTEST_A2 -verify %s 2>&1 | FileCheck --check-prefix=A2-CHECK %s
+// RUN: %clang_cc1 -DTEST_B1 -verify %s
+// RUN: %clang_cc1 -DTEST_B2 -verify %s
+// RUN: %clang_cc1 -DTEST_C1 -verify %s
+// RUN: not %clang_cc1 -DTEST_C2 -verify %s 2>&1 | FileCheck --check-prefix=C2-CHECK %s
+// RUN: %clang_cc1 -DTEST_C3 -verify %s
+// RUN: not %clang_cc1 -DTEST_C4 -verify %s 2>&1 | FileCheck --check-prefix=C4-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D1 -verify %s 2>&1 | FileCheck --check-prefix=D1-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D2 -verify %s 2>&1 | FileCheck --check-prefix=D2-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D3 -verify %s 2>&1 | FileCheck --check-prefix=D3-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D4 -verify %s 2>&1 | FileCheck --check-prefix=D4-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D5 -verify %s 2>&1 | FileCheck --check-prefix=D5-CHECK %s
+// RUN: not %clang_cc1 -DTEST_D6 -verify %s 2>&1 | FileCheck --check-prefix=D6-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E1 -verify %s 2>&1 | FileCheck --check-prefix=E1-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E2 -verify %s 2>&1 | FileCheck --check-prefix=E2-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E3 -verify %s 2>&1 | FileCheck --check-prefix=E3-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E1 -verify=foo,bar %s 2>&1 | FileCheck --check-prefix=E1-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E2 -verify=foo,bar %s 2>&1 | FileCheck --check-prefix=E2-CHECK %s
+// RUN: not %clang_cc1 -DTEST_E3 -verify=foo,bar %s 2>&1 | FileCheck --check-prefix=E3-CHECK %s
+
+#ifdef TEST_A1
+// expected-maybe-no-diagnostics
+#endif
+
+#ifdef TEST_A2
+// expected-maybe-no-diagnostics-re
+
+//      A2-CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// A2-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_B1
+// expected-maybe-no-diagnostics
+// expected-no-diagnostics
+#endif
+
+#ifdef TEST_B2
+// expected-no-diagnostics
+// expected-maybe-no-diagnostics
+#endif
+
+#ifdef TEST_C1
+// expected-maybe-no-diagnostics
+#error test_c1
+// expected-error@-1 {{test_c1}}
+#endif
+
+#ifdef TEST_C2
+// expected-maybe-no-diagnostics
+#error test_c2
+
+//      C2-CHECK: error: 'error' diagnostics seen but not expected:
+// C2-CHECK-NEXT:   {{test_c2}}
+// C2-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_C3
+#error test_c3
+// expected-error@-1 {{test_c3}}
+// expected-maybe-no-diagnostics
+#endif
+
+#ifdef TEST_C4
+#error test_c4
+// expected-maybe-no-diagnostics
+
+//      C4-CHECK: error: 'error' diagnostics seen but not expected:
+// C4-CHECK-NEXT:   {{test_c4}}
+// C4-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_D1
+// expected-maybe-no-diagnostics
+#error test_d1
+// expected-error@-1 {{test_d1}}
+// expected-no-diagnostics
+
+//      D1-CHECK: error: 'error' diagnostics seen but not expected:
+// D1-CHECK-NEXT:   {{.*}} 'expected-no-diagnostics' directive cannot follow directives that expect diagnostics
+// D1-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_D2
+// expected-maybe-no-diagnostics
+// expected-no-diagnostics
+#error test_d2
+// expected-error@-1 {{test_d2}}
+
+//      D2-CHECK: error: 'error' diagnostics seen but not expected:
+// D2-CHECK-NEXT:   {{test_d2}}
+// D2-CHECK-NEXT:   {{.*}} expected directive cannot follow 'expected-no-diagnostics' directive
+// D2-CHECK-NEXT: 2 errors generated.
+#endif
+
+#ifdef TEST_D3
+// expected-no-diagnostics
+// expected-maybe-no-diagnostics
+#error test_d3
+// expected-error@-1 {{test_d3}}
+
+//      D3-CHECK: error: 'error' diagnostics seen but not expected:
+// D3-CHECK-NEXT:   {{test_d3}}
+// D3-CHECK-NEXT:   {{.*}} expected directive cannot follow 'expected-no-diagnostics' directive
+// D3-CHECK-NEXT: 2 errors generated.
+#endif
+
+#ifdef TEST_D4
+// expected-no-diagnostics
+#error test_d4
+// expected-error@-1 {{test_d4}}
+// expected-maybe-no-diagnostics
+
+//      D4-CHECK: error: 'error' diagnostics seen but not expected:
+// D4-CHECK-NEXT:   {{test_d4}}
+// D4-CHECK-NEXT:   {{.*}} expected directive cannot follow 'expected-no-diagnostics' directive
+// D4-CHECK-NEXT: 2 errors generated.
+#endif
+
+#ifdef TEST_D5
+#error test_d5
+// expected-error@-1 {{test_d5}}
+// expected-no-diagnostics
+// expected-maybe-no-diagnostics
+
+//      D5-CHECK: error: 'error' diagnostics seen but not expected:
+// D5-CHECK-NEXT:   {{.*}} 'expected-no-diagnostics' directive cannot follow directives that expect diagnostics
+// D5-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_D6
+#error test_d6
+// expected-error@-1 {{test_d6}}
+// expected-maybe-no-diagnostics
+// expected-no-diagnostics
+
+//      D6-CHECK: error: 'error' diagnostics seen but not expected:
+// D6-CHECK-NEXT:   {{.*}} 'expected-no-diagnostics' directive cannot follow directives that expect diagnostics
+// D6-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_E1
+// BOGUS-maybe-no-diagnostics
+
+//      E1-CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// E1-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_E2
+// BOGUS-expected-maybe-no-diagnostics
+
+//      E2-CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// E2-CHECK-NEXT: 1 error generated.
+#endif
+
+#ifdef TEST_E3
+// expected-BOGUS-maybe-no-diagnostics
+
+//      E3-CHECK: error: no expected directives found: consider use of 'expected-no-diagnostics'
+// E3-CHECK-NEXT: 1 error generated.
+#endif

clang/test/Frontend/verify3.c

 
 // RUN: not %clang_cc1 -DTEST2 -verify %s 2>&1 | FileCheck -check-prefix=CHECK2 %s
 #ifdef TEST2
 #warning X
 // expected-warning@-1 {{X}}
 // expected-no-diagnostics
 
 //      CHECK2: error: 'error' diagnostics seen but not expected:
-// CHECK2-NEXT:   Line 19: 'expected-no-diagnostics' directive cannot follow other expected directives
+// CHECK2-NEXT:   Line 19: 'expected-no-diagnostics' directive cannot follow directives that expect diagnostics
 // CHECK2-NEXT: 1 error generated.
 #endif
 
 // RUN: not %clang_cc1 -DTEST3 -verify %s 2>&1 | FileCheck -check-prefix=CHECK3 %s
 // RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck -check-prefix=CHECK3 %s
 #ifdef TEST3
 // no directives