[Clang] Wide delimiters ('{{{') for expect strings #77326

sethp · 2024-01-08T15:28:55Z

Prior to this commit, it was impossible to use the simple string matching directives to look for any content that contains unbalanced {{ }} pairs, such as:

// expected-note {{my_struct{{1}, 2}}}

Which would parse like so:

             "nested" brace v
// expected-note {{my_struct{{1}, 2}}}
          closes the nested brace  ^ |
                            trailing }

And the frontend would complain 'cannot find end ('}}') of expected'.

At this snapshot, VerifyDiagnosticConsumer's parser now counts the opening braces and looks for a matching length of closing sigils, allowing the above to be written as:

// expected-note {{{my_struct{{1}, 2}}}}
   opening brace |-|                 |-|
  closing brace is '}}}', found here ^

This came about as a result of this discussion: #74852 (comment)

cc @erichkeane

github-actions · 2024-01-08T15:29:10Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-08T15:29:37Z

@llvm/pr-subscribers-clang

Author: None (sethp)

Changes

Prior to this commit, it was impossible to use the simple string matching directives to look for most content that contains {{, such as:

// expected-note {{my_struct{{1}, 2}}}

Which would parse like so:

             "nested" brace v
// expected-note {{my_struct{{1}, 2}}}
          closes the nested brace  ^ |
                            trailing }

And the frontend would complain 'cannot find end ('}}') of expected'.

At this snapshot, VerifyDiagnosticConsumer's parser now counts the opening braces and looks for a matching length of closing sigils, allowing the above to be written as:

// expected-note {{{my_struct{{1}, 2}}}}
   opening brace |-|                 |-|
  closing brace is '}}}', found here ^

This came about as a result of this discussion: #74852 (comment)

cc @erichkeane

Full diff: https://github.com/llvm/llvm-project/pull/77326.diff

2 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+1-1)
(modified) clang/lib/Frontend/VerifyDiagnosticConsumer.cpp (+11-4)

diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 715e0c0dc8fa84..4bf0ab54a046c1 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -166,7 +166,7 @@ def err_verify_no_such_marker : Error<
 def err_verify_missing_start : Error<
     "cannot find start ('{{') of expected %0">;
 def err_verify_missing_end : Error<
-    "cannot find end ('}}') of expected %0">;
+    "cannot find end ('%1') of expected %0">;
 def err_verify_invalid_content : Error<
     "invalid expected %0: %1">;
 def err_verify_missing_regex : Error<
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index ab8174f4f4db92..5eab7bd3619f19 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -612,12 +612,19 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
                    diag::err_verify_missing_start) << KindStr;
       continue;
     }
+    llvm::SmallString<8> CloseBrace("}}");
+    const char *const DelimBegin = PH.C;
     PH.Advance();
+    // Count the number of opening braces for `string` kinds
+    for (; !D.RegexKind && PH.Next("{"); PH.Advance())
+      CloseBrace += '}';
     const char* const ContentBegin = PH.C; // mark content begin
-    // Search for token: }}
-    if (!PH.SearchClosingBrace("{{", "}}")) {
-      Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-                   diag::err_verify_missing_end) << KindStr;
+    // Search for closing brace
+    StringRef OpenBrace(DelimBegin, ContentBegin - DelimBegin);
+    if (!PH.SearchClosingBrace(OpenBrace, CloseBrace)) {
+      Diags.Report(Pos.getLocWithOffset(PH.C - PH.Begin),
+                   diag::err_verify_missing_end)
+          << KindStr << CloseBrace;
       continue;
     }
     const char* const ContentEnd = PH.P; // mark content end

cor3ntin

Looks reasonable to me, thanks!

erichkeane · 2024-01-08T15:45:38Z

Can you update https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics as well please? It should be in-repo, but we should make sure that keeps up to date here. Else, LGTM, thanks!

sethp · 2024-01-08T15:49:38Z

Ah, I was looking for those docs, thank you! I'll be sure to tweak the description there.

shafik

Can we add a few tests for this change?

sethp · 2024-01-08T16:54:33Z

@shafik I can, though I'm not quite sure where: my actual use-case is in a file that doesn't exist yet. There's already accidentally a couple of positive tests for the widened delimiters in equality_tracking.c:, e.g.

llvm-project/clang/test/Analysis/equality_tracking.c

Line 286 in ff47989

clang_analyzer_value(x); // expected-warning {{{ [0, 255] }}}

Do those work for you? I didn't spot any way to do negative-case testing ("who verifies the verifier?"), if that's what you had in mind?

cor3ntin · 2024-01-08T16:57:48Z

actually, we could add tests in test/Frontend/verify.c, probably.

sethp · 2024-01-08T16:58:51Z

Ah, perfect! I'll look into that, thank you!

clang/docs/InternalsManual.rst

clang/test/Frontend/verify.c

sethp · 2024-01-08T18:46:02Z

clang/test/Frontend/verify.c

+// CHECK-WIDE-DELIM-NEXT:   verify.c Line 169: cannot find end ('}}') of expected regex
+// CHECK-WIDE-DELIM-NEXT:   verify.c Line 171: cannot find end ('}}}') of expected string
+// CHECK-WIDE-DELIM-NEXT:   verify.c Line 172: cannot find end ('}}') of expected regex
+// CHECK-WIDE-DELIM-NEXT:   verify.c Line 173: cannot find start of regex ('{{[{][{]}}') in {no regex


This is probably confusing; it's {no regex because the delimiter is always {{ in regex-mode, so the string is parsed like:

// expected-error-re {{{no regex}}} |---------|^ trailing }

clang/docs/InternalsManual.rst

Prior to this commit, it was impossible to use the simple string matching directives to look for most content that contains `{{`, such as: ``` // expected-note {{my_struct{{1}, 2}}} ``` Which would parse like so: ``` "nested" brace v // expected-note {{my_struct{{1}, 2}}} closes the nested brace ^ | trailing } ``` And the frontend would complain 'cannot find end ('}}') of expected'. At this snapshot, VerifyDiagnosticConsumer's parser now counts the opening braces and looks for a matching length of closing sigils, allowing the above to be written as: ``` // expected-note {{{my_struct{{1}, 2}}}} opening brace |-| |-| closing brace is '}}}', found here ^ ```

sethp · 2024-01-09T16:07:48Z

I think I've followed all the steps in https://llvm.org/docs/GitHub.html#landing-your-change except performing the actual merge (or squash), for which I lack write permissions. Is there anything else for me to do here?

erichkeane · 2024-01-09T16:15:47Z

I think I've followed all the steps in https://llvm.org/docs/GitHub.html#landing-your-change except performing the actual merge (or squash), for which I lack write permissions. Is there anything else for me to do here?

Nope! I'll put this on my list to merge when teh tests all pass.

sethp · 2024-01-09T16:17:13Z

Awesome, thank you for your help!

sethp · 2024-01-10T16:18:23Z

@ZequanWu would either of these work for you?

 DCHECK_OK(foo); // expected-error@components/reporting/util/status_macros.h:* {{{CHECK,DCHECK,ASSERT,EXPECT}_OK do not accept a type other than Status or StatusOr.}}}

or

 DCHECK_OK(foo); // expected-error-re@components/reporting/util/status_macros.h:* {{{CHECK,DCHECK,ASSERT,EXPECT}_OK do not accept a type other than Status or StatusOr.}}

sethp · 2024-01-10T16:28:42Z

Though, maybe we should revert this change, and gate it behind a new suffix (expected-error-ext?), since it is a breaking change to the verifier's mini-language.

ZequanWu · 2024-01-10T16:30:22Z

@ZequanWu would either of these work for you?

 DCHECK_OK(foo); // expected-error@components/reporting/util/status_macros.h:* {{{CHECK,DCHECK,ASSERT,EXPECT}_OK do not accept a type other than Status or StatusOr.}}}

or

 DCHECK_OK(foo); // expected-error-re@components/reporting/util/status_macros.h:* {{{CHECK,DCHECK,ASSERT,EXPECT}_OK do not accept a type other than Status or StatusOr.}}

Thanks, it's a simple fix, just adding a space between 2nd and 3rd braces:

DCHECK_OK(foo);  // expected-error@components/reporting/util/status_macros.h:* {{ {CHECK,DCHECK,ASSERT,EXPECT}_OK do not accept a type other than Status or StatusOr.}}

sethp · 2024-01-10T16:33:34Z

Of course, thank you for tracking it down!

@erichkeane

Prior to this commit, it was impossible to use the simple string matching directives to look for any content that contains unbalanced `{{` `}}` pairs, such as: ``` // expected-note {{my_struct{{1}, 2}}} ``` Which would parse like so: ``` "nested" brace v // expected-note {{my_struct{{1}, 2}}} closes the nested brace ^ | trailing } ``` And the frontend would complain 'cannot find end ('}}') of expected'. At this snapshot, VerifyDiagnosticConsumer's parser now counts the opening braces and looks for a matching length of closing sigils, allowing the above to be written as: ``` // expected-note {{{my_struct{{1}, 2}}}} opening brace |-| |-| closing brace is '}}}', found here ^ ``` This came about as a result of this discussion: llvm#74852 (comment) cc @erichkeane

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 8, 2024

cor3ntin approved these changes Jan 8, 2024

View reviewed changes

shafik reviewed Jan 8, 2024

View reviewed changes

erichkeane reviewed Jan 8, 2024

View reviewed changes

clang/docs/InternalsManual.rst Outdated Show resolved Hide resolved

clang/test/Frontend/verify.c Show resolved Hide resolved

sethp commented Jan 8, 2024

View reviewed changes

clang/docs/InternalsManual.rst Outdated Show resolved Hide resolved

erichkeane approved these changes Jan 9, 2024

View reviewed changes

sethp force-pushed the feat/verify-wide-delims branch from 019a8e0 to 1afdcda Compare January 9, 2024 16:02

erichkeane merged commit baa8c2a into llvm:main Jan 9, 2024

sethp deleted the feat/verify-wide-delims branch January 9, 2024 23:26

[Clang] Wide delimiters ('{{{') for expect strings #77326

[Clang] Wide delimiters ('{{{') for expect strings #77326

Uh oh!

Conversation

sethp commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 8, 2024

Uh oh!

llvmbot commented Jan 8, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Jan 8, 2024

Uh oh!

sethp commented Jan 8, 2024

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

sethp commented Jan 8, 2024

Uh oh!

cor3ntin commented Jan 8, 2024

Uh oh!

sethp commented Jan 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethp Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sethp commented Jan 9, 2024

Uh oh!

erichkeane commented Jan 9, 2024

Uh oh!

sethp commented Jan 9, 2024

Uh oh!

sethp commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sethp commented Jan 10, 2024

Uh oh!

ZequanWu commented Jan 10, 2024

Uh oh!

sethp commented Jan 10, 2024

Uh oh!

Uh oh!

sethp commented Jan 8, 2024 •

edited

Loading

sethp commented Jan 10, 2024 •

edited

Loading