[clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

[lebedev.ri]

Summary

Currently, there is two configured prefixes: CHECK-FIXES and CHECK-MESSAGES
CHECK-MESSAGES checks that there are no test output lines with warning:|error:, which are not explicitly handled in lit tests.
However there does not seem to be a nice way to enforce for all the note: to be checked.
This was useful for me when developing D36836.

[lebedev.ri]

Rebased.

[aaron.ballman]

Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle note in addition to warning and error? I'd prefer to keep the number of "magic" names as low as possible so I have to remember less stuff when writing or reviewing tests.

[JonasToth]

Note can be handled right now as well. E.g.

// CHECK-MESSAGES: [[@LINE-2]]:3: note: type deduction did not result in an owner

would the patch handle the codelocation correctly?

[lebedev.ri]

In D36892#856469, @aaron.ballman wrote:

Instead of CHECK-NOTES, do we want to extend CHECK-MESSAGES to handle note in addition to warning and error?

If i change CHECK-MESSAGES to also require all the notes to be checked, a *lot* of tests fail:

********************
Testing Time: 4.04s
********************
Failing Tests (165):
    Clang Tools :: clang-tidy/android-cloexec-accept.cpp
    Clang Tools :: clang-tidy/android-cloexec-accept4.cpp
    Clang Tools :: clang-tidy/android-cloexec-creat.cpp
    Clang Tools :: clang-tidy/android-cloexec-dup.cpp
    Clang Tools :: clang-tidy/android-cloexec-epoll-create.cpp
    Clang Tools :: clang-tidy/android-cloexec-epoll-create1.cpp
    Clang Tools :: clang-tidy/android-cloexec-fopen.cpp
    Clang Tools :: clang-tidy/android-cloexec-inotify-init.cpp
    Clang Tools :: clang-tidy/android-cloexec-inotify-init1.cpp
    Clang Tools :: clang-tidy/android-cloexec-memfd-create.cpp
    Clang Tools :: clang-tidy/android-cloexec-open.cpp
    Clang Tools :: clang-tidy/android-cloexec-socket.cpp
    Clang Tools :: clang-tidy/boost-use-to-string.cpp
    Clang Tools :: clang-tidy/bugprone-integer-division.cpp
    Clang Tools :: clang-tidy/bugprone-suspicious-memset-usage.cpp
    Clang Tools :: clang-tidy/bugprone-undefined-memory-manipulation.cpp
    Clang Tools :: clang-tidy/cert-dcl21-cpp.cpp
    Clang Tools :: clang-tidy/cert-oop11-cpp.cpp
    Clang Tools :: clang-tidy/clean-up-code.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-cstyle-cast.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init-cxx98.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init-delayed.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-member-init.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-static-cast-downcast.cpp
    Clang Tools :: clang-tidy/cppcoreguidelines-pro-type-vararg.cpp
    Clang Tools :: clang-tidy/deduplication.cpp
    Clang Tools :: clang-tidy/google-build-explicit-make-pair.cpp
    Clang Tools :: clang-tidy/google-explicit-constructor.cpp
    Clang Tools :: clang-tidy/google-readability-casting.c
    Clang Tools :: clang-tidy/google-readability-casting.cpp
    Clang Tools :: clang-tidy/google-readability-namespace-comments.cpp
    Clang Tools :: clang-tidy/google-readability-todo.cpp
    Clang Tools :: clang-tidy/llvm-include-order.cpp
    Clang Tools :: clang-tidy/llvm-twine-local.cpp
    Clang Tools :: clang-tidy/misc-argument-comment-gmock.cpp
    Clang Tools :: clang-tidy/misc-argument-comment-strict.cpp
    Clang Tools :: clang-tidy/misc-argument-comment.cpp
    Clang Tools :: clang-tidy/misc-assert-side-effect.cpp
    Clang Tools :: clang-tidy/misc-bool-pointer-implicit-conversion.cpp
    Clang Tools :: clang-tidy/misc-definitions-in-headers.hpp
    Clang Tools :: clang-tidy/misc-forwarding-reference-overload.cpp
    Clang Tools :: clang-tidy/misc-inaccurate-erase.cpp
    Clang Tools :: clang-tidy/misc-inefficient-algorithm.cpp
    Clang Tools :: clang-tidy/misc-lambda-function-name.cpp
    Clang Tools :: clang-tidy/misc-macro-parentheses.cpp
    Clang Tools :: clang-tidy/misc-macro-repeated-side-effects.c
    Clang Tools :: clang-tidy/misc-misplaced-const.c
    Clang Tools :: clang-tidy/misc-misplaced-const.cpp
    Clang Tools :: clang-tidy/misc-move-const-arg.cpp
    Clang Tools :: clang-tidy/misc-move-constructor-init.cpp
    Clang Tools :: clang-tidy/misc-move-forwarding-reference.cpp
    Clang Tools :: clang-tidy/misc-multiple-statement-macro.cpp
    Clang Tools :: clang-tidy/misc-static-assert.c
    Clang Tools :: clang-tidy/misc-static-assert.cpp
    Clang Tools :: clang-tidy/misc-string-compare.cpp
    Clang Tools :: clang-tidy/misc-string-constructor.cpp
    Clang Tools :: clang-tidy/misc-string-integer-assignment.cpp
    Clang Tools :: clang-tidy/misc-suspicious-semicolon.cpp
    Clang Tools :: clang-tidy/misc-suspicious-string-compare.c
    Clang Tools :: clang-tidy/misc-suspicious-string-compare.cpp
    Clang Tools :: clang-tidy/misc-swapped-arguments.cpp
    Clang Tools :: clang-tidy/misc-uniqueptr-reset-release.cpp
    Clang Tools :: clang-tidy/misc-unused-alias-decls.cpp
    Clang Tools :: clang-tidy/misc-unused-parameters.c
    Clang Tools :: clang-tidy/misc-unused-parameters.cpp
    Clang Tools :: clang-tidy/misc-unused-raii.cpp
    Clang Tools :: clang-tidy/misc-unused-using-decls.cpp
    Clang Tools :: clang-tidy/misc-virtual-near-miss.cpp
    Clang Tools :: clang-tidy/modernize-avoid-bind.cpp
    Clang Tools :: clang-tidy/modernize-deprecated-headers-cxx03.cpp
    Clang Tools :: clang-tidy/modernize-deprecated-headers-cxx11.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-basic.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-camelback.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-const.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-extra.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-lowercase.cpp
    Clang Tools :: clang-tidy/modernize-loop-convert-uppercase.cpp
    Clang Tools :: clang-tidy/modernize-make-shared-header.cpp
    Clang Tools :: clang-tidy/modernize-make-shared.cpp
    Clang Tools :: clang-tidy/modernize-make-unique-header.cpp
    Clang Tools :: clang-tidy/modernize-make-unique.cpp
    Clang Tools :: clang-tidy/modernize-pass-by-value-macro-header.cpp
    Clang Tools :: clang-tidy/modernize-pass-by-value.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal-delimiter.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal-replace-shorter.cpp
    Clang Tools :: clang-tidy/modernize-raw-string-literal.cpp
    Clang Tools :: clang-tidy/modernize-redundant-void-arg-delayed.cpp
    Clang Tools :: clang-tidy/modernize-redundant-void-arg.cpp
    Clang Tools :: clang-tidy/modernize-replace-auto-ptr.cpp
    Clang Tools :: clang-tidy/modernize-return-braced-init-list.cpp
    Clang Tools :: clang-tidy/modernize-shrink-to-fit.cpp
    Clang Tools :: clang-tidy/modernize-unary-static-assert.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-cast-remove-stars.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-cast.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-iterator.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-new-remove-stars.cpp
    Clang Tools :: clang-tidy/modernize-use-auto-new.cpp
    Clang Tools :: clang-tidy/modernize-use-bool-literals-ignore-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-bool-literals.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init-assignment.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-default-member-init.cpp
    Clang Tools :: clang-tidy/modernize-use-emplace-ignore-implicit-constructors.cpp
    Clang Tools :: clang-tidy/modernize-use-emplace.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-default-copy.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-default.cpp
    Clang Tools :: clang-tidy/modernize-use-equals-delete.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept-macro.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept-opt.cpp
    Clang Tools :: clang-tidy/modernize-use-noexcept.cpp
    Clang Tools :: clang-tidy/modernize-use-nullptr-basic.cpp
    Clang Tools :: clang-tidy/modernize-use-nullptr.cpp
    Clang Tools :: clang-tidy/modernize-use-override-ms.cpp
    Clang Tools :: clang-tidy/modernize-use-override.cpp
    Clang Tools :: clang-tidy/modernize-use-transparent-functors.cpp
    Clang Tools :: clang-tidy/modernize-use-using-macros.cpp
    Clang Tools :: clang-tidy/modernize-use-using.cpp
    Clang Tools :: clang-tidy/nolint.cpp
    Clang Tools :: clang-tidy/nolintnextline.cpp
    Clang Tools :: clang-tidy/performance-faster-string-find.cpp
    Clang Tools :: clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
    Clang Tools :: clang-tidy/performance-for-range-copy.cpp
    Clang Tools :: clang-tidy/performance-inefficient-vector-operation.cpp
    Clang Tools :: clang-tidy/performance-type-promotion-in-math-fn.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-copy-initialization.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-delayed.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-header.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param-incomplete-type.cpp
    Clang Tools :: clang-tidy/performance-unnecessary-value-param.cpp
    Clang Tools :: clang-tidy/readability-avoid-const-params-in-decls.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-few-lines.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-format.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-same-line.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements-single-line.cpp
    Clang Tools :: clang-tidy/readability-braces-around-statements.cpp
    Clang Tools :: clang-tidy/readability-container-size-empty.cpp
    Clang Tools :: clang-tidy/readability-delete-null-pointer.cpp
    Clang Tools :: clang-tidy/readability-else-after-return.cpp
    Clang Tools :: clang-tidy/readability-function-size.cpp
    Clang Tools :: clang-tidy/readability-identifier-naming.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion-allow-in-conditions.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion-cxx98.cpp
    Clang Tools :: clang-tidy/readability-implicit-bool-conversion.cpp
    Clang Tools :: clang-tidy/readability-inconsistent-declaration-parameter-name.cpp
    Clang Tools :: clang-tidy/readability-misplaced-array-index.cpp
    Clang Tools :: clang-tidy/readability-named-parameter.cpp
    Clang Tools :: clang-tidy/readability-non-const-parameter.cpp
    Clang Tools :: clang-tidy/readability-redundant-control-flow.cpp
    Clang Tools :: clang-tidy/readability-redundant-declaration-ignore-macros.cpp
    Clang Tools :: clang-tidy/readability-redundant-declaration.cpp
    Clang Tools :: clang-tidy/readability-redundant-function-ptr-dereference.cpp
    Clang Tools :: clang-tidy/readability-redundant-member-init.cpp
    Clang Tools :: clang-tidy/readability-redundant-smartptr-get.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-cstr-msvc.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-cstr.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-init-msvc.cpp
    Clang Tools :: clang-tidy/readability-redundant-string-init.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr-chained-conditional-assignment.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr-chained-conditional-return.cpp
    Clang Tools :: clang-tidy/readability-simplify-bool-expr.cpp
    Clang Tools :: clang-tidy/readability-static-accessed-through-instance-nesting-threshold.cpp
    Clang Tools :: clang-tidy/readability-static-accessed-through-instance.cpp
    Clang Tools :: clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
    Clang Tools :: clang-tidy/readability-uniqueptr-delete-release.cpp

  Expected Passes    : 392
  Unexpected Failures: 165

I'd prefer to keep the number of "magic" names as low as possible so I have to remember less stuff when writing or reviewing tests.

I do agree that it makes sense to keep it as low as possible, but also i see a clear logic between all thee current checks:
There are [currently] three types of output out of clang-tidy check that we care about: fix-it; error, warning, note.

• The CHECK-FIXES is special, and only looks for fix-its.
• It would make sense to always check all the error messages, thus rest of the magic does enforce -implicit-check-not=error
• * CHECK-MESSAGES additionally enforces -implicit-check-not=warning
• None of those two magics care about notes
• * So i'm adding CHECK-NOTES, which is a CHECK-MESSAGES + -implicit-check-not=note

[lebedev.ri]

In D36892#856501, @JonasToth wrote:

Note can be handled right now as well.

Yes. Adding this new prefix is about adding -implicit-check-not=notes.
I.e. if you use CHECK-MESSAGES, then it will only enforce you to have check-lines for all the error: and warning: the check outputs.
It does *not* enforce that all the note: are to be handled. CHECK-NOTES however would enforce that.

E.g.

// CHECK-MESSAGES: [[@LINE-2]]:3: note: type deduction did not result in an owner

would the patch handle the codelocation correctly?

I'm not sure what is the question to be honest. This does not change anything about codelocation handling

[JonasToth]

alright. i thought it would do something different, but the enforcement to handle all notes is a good thing. forget what i wrote :)

[aaron.ballman]

I do agree that it makes sense to keep it as low as possible, but also i see a clear logic between all thee current checks:

Thank you for the explanation. I think that makes sense to me, but I'd like to hear from @alexfh before accepting.

test/clang-tidy/check_clang_tidy.py

100–102

I think the text should be rephrased to "CHECK-FIXES, CHECK-MESSAGES, or CHECK-NOTES not found in the input"

[lebedev.ri]

Reword the 'no magic found' error message.

[lebedev.ri]

Rebased.

[aaron.ballman]

I am okay with this direction but would still like @alexfh to accept before you commit.

[lebedev.ri]

@alexfh any thoughts on this one?

[lebedev.ri]

Rebased.

[aaron.ballman]

I think you're set to commit this -- if @alexfh has concerns, they can be addressed post-commit.

[lebedev.ri]

In D36892#953891, @aaron.ballman wrote:

I think you're set to commit this

Should i commit it though?
Until licensing concerns with D36836 are finally magically resolved and it is committed, there won't be any users, and since there is no tests for this, that would be a dead code with all the consequences.

• if @alexfh has concerns, they can be addressed post-commit.

[aaron.ballman]

In D36892#953953, @lebedev.ri wrote:

In D36892#953891, @aaron.ballman wrote:

I think you're set to commit this

Should i commit it though?
Until licensing concerns with D36836 are finally magically resolved and it is committed, there won't be any users, and since there is no tests for this, that would be a dead code with all the consequences.

Hmm, I thought we had other tests that could make use of this functionality? However, I might be misremembering. Hold off on committing until there's test coverage, but it'd be good to look at existing tests to see if any of them can be modified.

[alexfh]

It looks like some existing tests could benefit from this:

$ grep -R -l 'note: ' test/clang-tidy/
test/clang-tidy/bugprone-forward-declaration-namespace.cpp
test/clang-tidy/llvm-twine-local.cpp
test/clang-tidy/overlapping.cpp
test/clang-tidy/google-readability-nested-namespace-comments.cpp
test/clang-tidy/bugprone-suspicious-enum-usage-strict.cpp
test/clang-tidy/cert-static-object-exception.cpp
test/clang-tidy/fix.cpp
test/clang-tidy/google-readability-namespace-comments.cpp
test/clang-tidy/cppcoreguidelines-avoid-goto.cpp
test/clang-tidy/performance-move-constructor-init.cpp
test/clang-tidy/fuchsia-default-arguments.cpp
test/clang-tidy/readability-misleading-indentation.cpp
test/clang-tidy/readability-inconsistent-declaration-parameter-name-macros.cpp
test/clang-tidy/fix-errors.cpp
test/clang-tidy/bugprone-use-after-move.cpp
test/clang-tidy/cppcoreguidelines-owning-memory-containers.cpp
test/clang-tidy/bugprone-argument-comment.cpp
test/clang-tidy/cppcoreguidelines-owning-memory.cpp
test/clang-tidy/readability-function-size.cpp
test/clang-tidy/hicpp-exception-baseclass.cpp
test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp
test/clang-tidy/macros.cpp
test/clang-tidy/readability-container-size-empty.cpp
test/clang-tidy/Inputs/overlapping/o.h
test/clang-tidy/bugprone-forwarding-reference-overload.cpp

If you're interested in changing some of these tests to use CHECK-NOTES, there's no need to wait for a resolution on D36836 (which, unfortunately, may take really long). However, please update the relevant section of docs/clang-tidy/index.rst.

[lebedev.ri]

Rebased, just to control bitrot, no changes.

[alexfh]

As per the previous comment: I have no concerns as long as the documentation is updated and at least one existing test is changed to use this feature (see the list in the previous comment).

[JonasToth]

@lebedev.ri and @alexfh i would change the tests in https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this one?

For testing purposes, you could change a single line of hicpp-exception-baseclass.cpp to use the CHECK-NOTES. I do the rest :)

[alexfh]

In D36892#1187959, @JonasToth wrote:

@lebedev.ri and @alexfh i would change the tests in https://reviews.llvm.org/D48714 to use CHECK-NOTES. Is it ok, to commit this one?

For testing purposes, you could change a single line of hicpp-exception-baseclass.cpp to use the CHECK-NOTES. I do the rest :)

SGTM

[lebedev.ri]

Rebase (ugh, bitrot), port test/clang-tidy/hicpp-exception-baseclass.cpp.

[lebedev.ri]

Add docs note.

[JonasToth]

LGTM

test/clang-tidy/check_clang_tidy.py

def main():
   if resource_dir is not None:
     clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
 
   with open(input_file_name, 'r') as input_file:
     input_text = input_file.read()
 
   has_check_fixes = input_text.find('CHECK-FIXES') >= 0
   has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  has_check_notes = input_text.find('CHECK-NOTES') >= 0
 
-  if not has_check_fixes and not has_check_messages:
-    sys.exit('Neither CHECK-FIXES nor CHECK-MESSAGES found in the input')
+  if not has_check_fixes and not has_check_messages and not has_check_notes:
+    sys.exit('Neither CHECK-FIXES nor CHECK-MESSAGES '
+             'nor CHECK-NOTES found in the input')
 
   # 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-Z-]*:[^\r\n]*', '//', input_text)
 
   write_file(temp_file_name, cleaned_test)
 
   original_file_name = temp_file_name + ".orig"
   write_file(original_file_name, cleaned_test)
 
   args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
         clang_tidy_extra_args
   print('Running ' + repr(args) + '...')
   try:
     clang_tidy_output = \
         subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
   except subprocess.CalledProcessError as e:
     print('clang-tidy failed:\n' + e.output.decode())
     raise
 
   print('------------------------ clang-tidy output -----------------------\n' +
         clang_tidy_output +
         '\n------------------------------------------------------------------')
 
   try:
     diff_output = subprocess.check_output(
         ['diff', '-u', original_file_name, temp_file_name],
         stderr=subprocess.STDOUT)
try:
           ['FileCheck', '-input-file=' + messages_file, input_file_name,
            '-check-prefix=CHECK-MESSAGES',
            '-implicit-check-not={{warning|error}}:'],
           stderr=subprocess.STDOUT)
     except subprocess.CalledProcessError as e:
       print('FileCheck failed:\n' + e.output.decode())
       raise
 
+  if has_check_notes:
+    messages_file = temp_file_name + '.msg'
+    write_file(messages_file, clang_tidy_output)
+    try:
+      subprocess.check_output(
+          ['FileCheck', '-input-file=' + messages_file, input_file_name,
+           '-check-prefix=CHECK-NOTES',
+           '-implicit-check-not={{note|warning|error}}:'],
+          stderr=subprocess.STDOUT)
+    except subprocess.CalledProcessError as e:
+      print('FileCheck failed:\n' + e.output.decode())
+      raise
+
 if __name__ == '__main__':
   main()