[clang-tidy] fix check_clang_tidy to forbid mixing of CHECK-NOTES and CHECK-MESSAGES

[JonasToth]

Summary

The check_clang_tidy.py script would allow mixing of CHECK-NOTES and CHECK-MESSAGES but running FileCheck for that would implicitly fail, because CHECK-NOTES bails out if there is a warning.

That means a clang-tidy test can not mix these constructs to check warnings with CHECK-MESSAGES and notes with CHECK-NOTES. The script gives now a clear error if that happens.

[lebedev.ri]

You would still have to duplicate the check-lines for error: though.

As it was discussed in https://reviews.llvm.org/D36892 i would think the current behavior is correct:
the CHECK-MESSAGES vs CHECK-NOTES denotes the severity of the output msg that is ok to be ignored.
If you are ok with ignoring notes, just use the CHECK-MESSAGES prefix.
If you want the notes to be checked, use CHECK-NOTES prefix.
Either/or. But don't mix them.

It's not a good idea to mix them, e.g. because the tool then has to run FileCheck twice..

[JonasToth]

@lebedev.ri lets do it in the the other patch, to not split discussions. But what do you mean by You would still have to duplicate the check-lines for error: though.?

[lebedev.ri]

In D51381#1217047, @JonasToth wrote:

@lebedev.ri lets do it in the the other patch, to not split discussions.

Let's do it here instead, since that differential requires some changes to this script.

In D51381#1217047, @JonasToth wrote:

But what do you mean by You would still have to duplicate the check-lines for error: though.?

In it's current state, CHECK-MESSAGES implies -implicit-check-not={{warning|error}},
and CHECK-NOTES will imply -implicit-check-not={{note|error}}.
I.e. they both imply -implicit-check-not=error.
So if the check produces any error: output, and you want to use both the CHECK-NOTES and CHECK-MESSAGES,
you need to write the check-line for every error: with both the CHECK-NOTES and CHECK-MESSAGES prefixes.
This seems sub-optimal.

In D48714#1217044, @JonasToth wrote:

In D48714#1216989, @lebedev.ri wrote:

In D48714#1216537, @JonasToth wrote:

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

You are right that replacing all of it with CHECK-NOTES works as well.
FileCheck is run twice if you have FIXMES as well. Having another run for the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure that only one of CHECK-MESSAGES or CHECK-NOTES is present in the file and adjust the check_clang_tidy.py script a little.

I don't have a strong preference, but if that works i would prefer to go that route, since that is what the initial intended semantics of the CHECK-NOTES.
If that works for you?

[JonasToth]

Yes, you are write.

I would ensure in the script that CHECK-MESSAGES XOR CHECK-NOTES is
used, to avoid the mistake i made. I can do it in this diff as well.

Am 29.08.2018 um 20:29 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

In https://reviews.llvm.org/D51381#1217047, @JonasToth wrote:

@lebedev.ri lets do it in the the other patch, to not split discussions.

Let's do it here instead, since that differential requires some changes to this script.

In https://reviews.llvm.org/D51381#1217047, @JonasToth wrote:

But what do you mean by You would still have to duplicate the check-lines for error: though.?

In it's current state, CHECK-MESSAGES implies -implicit-check-not={{warning|error}},
and CHECK-NOTES will imply -implicit-check-not={{note|error}}.
I.e. they both imply -implicit-check-not=error.
So if the check produces any error: output, and you want to use both the CHECK-NOTES and CHECK-MESSAGES,
you need to write the check-line for every error: with both the CHECK-NOTES and CHECK-MESSAGES prefixes.
This seems sub-optimal.

In https://reviews.llvm.org/D48714#1217044, @JonasToth wrote:

In https://reviews.llvm.org/D48714#1216989, @lebedev.ri wrote:

In https://reviews.llvm.org/D48714#1216537, @JonasToth wrote:

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the https://reviews.llvm.org/D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

You are right that replacing all of it with CHECK-NOTES works as well.
FileCheck is run twice if you have FIXMES as well. Having another run for the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure that only one of CHECK-MESSAGES or CHECK-NOTES is present in the file and adjust the check_clang_tidy.py script a little.

I don't have a strong preference, but if that works i would prefer to go that route, since that is what the initial intended semantics of the CHECK-NOTES.
If that works for you?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D51381

[JonasToth]

• adjust check_clang_tidy to use either CHECK-NOTES or CHECK-MESSAGES but not both

[lebedev.ri]

LG, thank you!

test/clang-tidy/check_clang_tidy.py

def main():
   has_check_fixes = check_fixes_prefix in input_text
   has_check_messages = check_messages_prefix in input_text
   has_check_notes = check_notes_prefix in input_text
 
   if not has_check_fixes and not has_check_messages and not has_check_notes:
     sys.exit('%s, %s or %s not found in the input' % (check_fixes_prefix,
              check_messages_prefix, check_notes_prefix) )
 
+  if has_check_notes and has_check_messages:
+    sys.exit('Please use either CHECK-NOTES or CHECK-MESSAGES but not both')
+
   # Remove the contents of the CHECK lines to avoid CHECKs matching on
   # themselves.  We need to keep the comments to preserve line numbers while
   # avoiding empty lines which could potentially trigger formatting-related
   # checks.
   cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
 
   write_file(temp_file_name, cleaned_test)