[clang-tidy] Allow tests to verify changes made to header files

[LegalizeAdulthood]

Summary

This changeset improves check_clang_tidy.py to allow chnages made by a check to a header to be verified.

A new argument --header-filter allows the test file to specify the header that will be modified by the check.

check_clang_tidy.py will then:

• Copy the named header to the same temporary directory containing the source file
• Scrub CHECK-FIXES and CHECK-FIXES text from the copied header
• Run the clang-tidy on the scrubbed source files with:
  • -header-filter and the name of the copied header file.
  • -I . to specify the temporary directory in the include search path
• Verify messages and fixes on the source file.
• Verify messages and fixes on the header file.
• In the case of failure, report differences on the header and the source file.

[LegalizeAdulthood]

Squeak

[LegalizeAdulthood]

Another week without reviews...

These changes were already split from a previous changeset without being reviewed properly.

Squeak squeak squeak.

[alexfh]

Sorry for leaving this without attention for a while.

I'm somewhat concerned about this change. It's adding a certain level of complexity, but doesn't cover some less trivial cases like handling of multiple headers. Can you take a look at existing tests and say how many times this feature is going to be used, if it gets added to the testing script?

Thank you!

test/clang-tidy/check_clang_tidy.py

122

The name of the flag might be confusing, since it doesn't have the same meaning as the clang-tidy flag with the same name. The clang-tidy --header-filter flag is a regular expression, but this one is a single header filename.

[LegalizeAdulthood]

In D17482#372677, @alexfh wrote:

Sorry for leaving this without attention for a while.

I'm somewhat concerned about this change. It's adding a certain level of complexity, but doesn't cover some less trivial cases like handling of multiple headers.

In the context of a clang-tidy check, what would be gained by verifying changes made to multiple headers that isn't already tested by verifying changes made to a single header?

Can you take a look at existing tests and say how many times this feature is going to be used, if it gets added to the testing script?

At least two of the checks I've already added have complaints that they don't work on headers.

While this change was waiting to be reviewed, another check was added that made changes to headers but had no way to verify them.

[LegalizeAdulthood]

In D17482#372677, @alexfh wrote:

Can you [...] say how many times this feature is going to be used, if it gets added to the testing script?

Since you can't verify changes against headers right now, noone has bothered to add test cases for their checks against headers.

However, I think a case can be made that every check potentially operates against headers and that they should be verified as such. But, since you can't do this in the build right now, noone has written such tests.

[LegalizeAdulthood]

test/clang-tidy/misc-unused-parameters.cpp currently does a hack to verify headers by committing the manually fixed header and diffing it:

// RUN: echo "static void staticFunctionHeader(int i) {}" > %T/header.h
// RUN: echo "static void staticFunctionHeader(int  /*i*/) {}" > %T/header-fixed.h
// RUN: %check_clang_tidy %s misc-unused-parameters %t -- -header-filter='.*' -- -std=c++11 -fno-delayed-template-parsing
// RUN: diff %T/header.h %T/header-fixed.h

#include "header.h"
// CHECK-MESSAGES: header.h:1:38: warning

I think this would be much cleaner with header.h as a regularly comitted file with CHECK-MESSAGES and CHECK-FIXES lines in that file.

[LegalizeAdulthood]

Squeak

[LegalizeAdulthood]

I'm sorry to be such a nag and I know you're busy, but....

This changeset has been held up for a month and a half. (6 weeks since originally posted in http://reviews.llvm.org/D16953)

The change isn't that complicated and there haven't really been any comments requiring action on my part, so I don't understand why this is taking so long.

[alexfh]

In D17482#387457, @LegalizeAdulthood wrote:

I'm sorry to be such a nag and I know you're busy, but....

This changeset has been held up for a month and a half. (6 weeks since originally posted in http://reviews.llvm.org/D16953)

The change isn't that complicated and there haven't really been any comments requiring action on my part, so I don't understand why this is taking so long.

My main concern is that this change makes the already complicated and unobvious testing mechanism even more complicated and even less obvious for a very little (as I see it now) benefit. The added functionality supports a very limited use case (a single header) and it does it in a rather hacky way (the use of the new --header-filter flag is not self-documenting and the overall behavior seems "magical" at the first glance).

There's also an outstanding comment that you didn't seem to have addressed yet.

In D17482#378685, @LegalizeAdulthood wrote:

In D17482#372677, @alexfh wrote:

Sorry for leaving this without attention for a while.

I'm somewhat concerned about this change. It's adding a certain level of complexity, but doesn't cover some less trivial cases like handling of multiple headers.

In the context of a clang-tidy check, what would be gained by verifying changes made to multiple headers that isn't already tested by verifying changes made to a single header?

Can you take a look at existing tests and say how many times this feature is going to be used, if it gets added to the testing script?

At least two of the checks I've already added have complaints that they don't work on headers.

That's because you used isExpansionInMainFile, which is just not needed there (BTW, I believe, the usages of isExpansionInMainFile in SimplifyBooleanExprCheck.cpp are not needed as well. At most they work around some other issues - incorrect template handling, for example.). Most checks don't need to do anything special to work on headers, they don't need to know whether a location is in the main file or not, and they don't need special tests for handling headers correctly. Adding such tests "just in case" will only add work to check authors and make the process more complicated for no reason. There are a few exceptions (DefinitionsInHeadersCheck.cpp, UnusedAliasDeclsCheck.cpp, UnnamedNamespaceInHeaderCheck.cpp, IncludeOrderCheck.cpp, GlobalNamesInHeadersCheck.cpp, that's about it), and only two of them (IncludeOrderCheck and DefinitionsInHeadersCheck) actually need special tests for fixes in headers.

While this change was waiting to be reviewed, another check was added that made changes to headers but had no way to verify them.

Which check do you mean? Does it need a special treatment of headers or is it just able to make fixes in headers as most other checks?

test/clang-tidy/modernize-redundant-void-arg.h

1

Additional headers should be in the Inputs/ directory to prevent more pollution of the test/clang-tidy directory.

[LegalizeAdulthood]

As it stands currently, you can't commit a header with CHECK-MESSAGES and CHECK-FIXES lines and have them verified.

That's the whole point of this changeset.

Currently you have to do something very hacky in order to verify changes made to headers.

[LegalizeAdulthood]

In D17482#390237, @alexfh wrote:

My main concern is that this change makes the already complicated and unobvious testing mechanism [...]

The complexity and obtuseness of the existing testing mechanism is unrelated to this changeset. This changeset doesn't fundamentally change the testing mechanism.

even more complicated and even less obvious for a very little (as I see it now) benefit.

The benefit is that you would now have a consistent mechanism for verifying changes made to headers.

The added functionality supports a very limited use case (a single header)

You have to start somewhere. Currently there is no mechanism provided at all. Saying that this mechanism is not acceptable because it doesn't handle arbitrarily complex generalized checking is making the perfect the enemy of the good and isn't a reason for preventing this change from being accepted IMO.

and it does it in a rather hacky way (the use of the new --header-filter flag is not self-documenting and the overall behavior seems "magical" at the first glance).

None of the mechanisms in the testing of these files is self-documenting. If that is the criteria by which all changes are to be measured, then the entire existing system has to be thrown out. Again, this feels like making the perfect the enemy of the good. The behavior for validating header files is the same as the existing behavior for validating source files. They are copied, filtered, processed and the processed results are analyzed. Discrepencies are shown as diffs against the original source files.

There's also an outstanding comment that you didn't seem to have addressed yet.

Which specific comment are you referring to? Because, just like this comment, there's a bunch of discussion in very general terms without specific complaints against specific pieces of code that are preventing it from being committed.

In D17482#378685, @LegalizeAdulthood wrote:

In the context of a clang-tidy check, what would be gained by verifying changes made to multiple headers that isn't already tested by verifying changes made to a single header?

You haven't answered this question yet. The existing test mechanism verifies only a single source file at a time; I see no reason why verifying a single header is such a great imposition of constraints that would prevent existing checks from being enhanced to verify their changes on header files. However, should that arise in the future it is a relatively small change to add additional header processing to the script. Again, let's not make the perfect the enemy of the good. There is no reason we cannot improve the code in steps, on demand, as the need arises. This is the essence of agile development. YAGNI

At least two of the checks I've already added have complaints that they don't work on headers.

That's because you used isExpansionInMainFile, which is just not needed there

...and that was because I couldn't write an automated test against header files to verify the changes made there. The whole point of THIS changeset was to give me a mechanism for verifying the fixes in a header so that I could address those issues in the bug database.

But instead of getting on with fixing it, I've spent 6 weeks waiting for this changeset to be reviewed and that discussion has prevented an advancement of functionality in the testing framework.

While this change was waiting to be reviewed, another check was added that made changes to headers but had no way to verify them.

Which check do you mean? Does it need a special treatment of headers or is it just able to make fixes in headers as most other checks?

I already quoted the check in question earlier in this thread; please review those messages.

[alexfh]

Let's try to turn this to a productive lane. I'm not against adding this functionality, if it's actually useful, but as is the patch doesn't meet the bar. Right now, there are a few action items.

First, could you split the refactorings and send me as a separate patch? They have little to do with the added functionality, so I prefer to review them separately.

Second, please move the header to a subdirectory of test/clang-tidy/Inputs/ (http://reviews.llvm.org/D17482#inline-156627).

Third (possibly, after the refactoring patch goes in, to reduce unneeded work), please address this comment: http://reviews.llvm.org/D17482#inline-149906.

[LegalizeAdulthood]

Review process takes too long to make forward progress; abandoning.

test/clang-tidy/check_clang_tidy.py

check_clang_tidy.py [-resource-dir <resource-dir>] \
     <source-file> <check-name> <temp-file> \
     -- [optional clang-tidy arguments]
 
 Example:
   // RUN: %check_clang_tidy %s llvm-include-order %t -- -isystem $(dirname %s)/Inputs/Headers
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
 
 
 def write_file(file_name, text):
   with open(file_name, 'w') as f:
     f.write(text)
     f.truncate()
 
+
+# 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.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+    cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+    if input_messages:
+      if header_file_name in line:
+        input_messages = False
+        header_file_output += line
+      else:
+        input_file_output += line
+    else:
+      if input_file_name in line:
+        input_messages = True
+        input_file_output += line
+      else:
+        header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+    process_output = \
+      subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+    print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+    raise
+  return process_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+    ['FileCheck', '-input-file=' + messages_file, input_file_name,
+       '-check-prefix=CHECK-MESSAGES',
+       '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+      ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+       '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(original_file_name, temp_file_name):
+  try:
+    diff_output = subprocess.check_output(
+      ['diff', '-u', original_file_name, temp_file_name],
+      stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+    diff_output = e.output
+  return diff_output
+
+
 def main():
   parser = argparse.ArgumentParser()
   parser.add_argument('-resource-dir')
+  parser.add_argument('--header-filter')
   parser.add_argument('input_file_name')
   parser.add_argument('check_name')
   parser.add_argument('temp_file_name')
 
   args, extra_args = parser.parse_known_args()
 
   resource_dir = args.resource_dir
+  header_filter = args.header_filter
   input_file_name = args.input_file_name
   check_name = args.check_name
   temp_file_name = args.temp_file_name
 
   extension = '.cpp'
   if (input_file_name.endswith('.c')):
     extension = '.c'
   if (input_file_name.endswith('.hpp')):
     extension = '.hpp'
   temp_file_name = temp_file_name + extension
 
   clang_tidy_extra_args = extra_args
   if len(clang_tidy_extra_args) == 0:
     clang_tidy_extra_args = ['--', '--std=c++11'] if extension == '.cpp' \
                        else ['--']
   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_fixes, has_check_messages = has_checks(input_text)
 
   if not has_check_fixes and not has_check_messages:
     sys.exit('Neither CHECK-FIXES nor CHECK-MESSAGES 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)
-
+  cleaned_test = clean_text(input_text)
   write_file(temp_file_name, cleaned_test)
-
   original_file_name = temp_file_name + ".orig"
   write_file(original_file_name, cleaned_test)
 
+  cleaned_header_path = ''
+  fixed_header_path = ''
+  source_header_path = ''
+  if header_filter is not None:
+    cleaned_header_path, fixed_header_path, source_header_path = \
+      write_cleaned_header(input_file_name, temp_file_name, header_filter)
+    args = ['clang-tidy', '-header-filter=%s' % header_filter,
+            temp_file_name, '-fix', '--checks=-*,' + check_name] + \
+        clang_tidy_extra_args + [ '-I', '.' ]
+  else:
     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
+  clang_tidy_output = try_run(args)
 
   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)
-  except subprocess.CalledProcessError as e:
-    diff_output = e.output
+  diff_output = diff_source_files(original_file_name, temp_file_name)
+  if header_filter is not None:
+    diff_output += diff_source_files(cleaned_header_path, fixed_header_path)
 
   print('------------------------------ Fixes -----------------------------\n' +
         diff_output.decode() +
         '\n------------------------------------------------------------------')
 
   if has_check_fixes:
-    try:
-      subprocess.check_output(
-          ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
-           '-check-prefix=CHECK-FIXES', '-strict-whitespace'],
-          stderr=subprocess.STDOUT)
-    except subprocess.CalledProcessError as e:
-      print('FileCheck failed:\n' + e.output.decode())
-      raise
+    if header_filter is not None:
+      check_output_for_fixes(fixed_header_path, source_header_path)
+    check_output_for_fixes(temp_file_name, input_file_name)
 
   if has_check_messages:
     messages_file = temp_file_name + '.msg'
+    if header_filter is not None:
+      header_messages_file = messages_file + '.h'
+      header_output, input_file_output = \
+        separate_output(clang_tidy_output, header_filter, input_file_name)
+      write_file(header_messages_file, header_output)
+      write_file(messages_file, input_file_output)
+
+      check_output_for_messages(messages_file, input_file_name)
+      check_output_for_messages(header_messages_file, source_header_path)
+    else:
       write_file(messages_file, clang_tidy_output)
-    try:
-      subprocess.check_output(
-          ['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
+      check_output_for_messages(messages_file, input_file_name)
+
 
 if __name__ == '__main__':
   main()

test/clang-tidy/modernize-redundant-void-arg.h

+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H

test/clang-tidy/modernize-redundant-void-arg.cpp

-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
 int foo();
 
 void bar();
 
 void bar2();