Make run-clang-tidy.py print the configured checks correctly

[JesApp]

Summary

The test invocation at the start of run-clang-tidy.py (line 257) prints all enabled checks - meaning either the default set or anything configured via the -checks option. If any checks were (un-)configured via the -config option, these are not printed. This is confusing to the user, since the list of checks that are printed may be different from the list of checks that are used by the non-testing calls to clang-tidy, where the -config option is passed correctly.

This patch adds the -config option to the test invocation of clang-tidy at the start of the script. This means that checks (un-)configured via the -config option (rather than the -checks option) are applied correctly, when printing the list of enabled checks.

[JesApp]

See this github issue for the bug this fixes.

[carlosgalvezp]

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

256–263

Would it make sense to re-use get_tidy_invocation with the proper arguments here?

[JesApp]

As requested by @carlosgalvezp, this update uses the get_tidy_invocation function, rather than building it's own test invocation.

I chose to pass all arguments of the script, in case any of them ever have an impact on what is being printed. The exception is the tmpdir argument, since this directory would get created even if the test invocation then failed.

[carlosgalvezp]

Looks good to me! A general comment not related to this patch is that the get_tidy_invocation is very error prone when called like that, since all arguments are strings. It would be better to specify the arguments:

get_tidy_invocation(name="", clang_tidy_binary=args.clang_tidy_binary,...)

But as said no need to fix here.

I haven't reviewed this file much and I'm quite new as reviewer so perhaps it's good that someone else takes another look and agrees with the change.

[JesApp]

Thanks, @carlosgalvezp!

I'm new to phabricator, buildkite, your whole setup. Any idea why the patch fails?

[carlosgalvezp]

Perhaps the patch was applied on a too old baseline, could you try rebasing on top of latest main?

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

163

Am I missing something, or is this function now unused?

[carlosgalvezp]

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

163

Nope, still used in line 301.

[JesApp]

Resubmitting the same changes after rebasing onto current main.

[JesApp]

Anyone have an idea why it's still building?

[carlosgalvezp]

In D118104#3278493, @JesApp wrote:

Anyone have an idea why it's still building?

Very strange, I have no clue :/

[aaron.ballman]

In D118104#3278493, @JesApp wrote:

Anyone have an idea why it's still building?

Hmmm, it looks like an infrastructure issue with the CI builder. https://buildkite.com/organizations/llvm-project/pipelines/diff-checks/builds/85880/jobs/928e70c7-6afc-4585-9e42-00b1644d0bf1/raw_log suggests that the issue comes from GitHub closing the connection forcefully (so it could be an issue with whatever login credentials are being used by the bot perhaps).

Rebasing onto current main may be insufficient to trigger the bot to rebuild. I'd recommend making a change to the patch (add some whitespace changes or something) and resubmit that. That should trigger a new build and we can pull the changes back out before landing. If the issue still reproduces, we can get the CI folks involved in investigating further.

[JesApp]

I can (re)start a build manually. Let's see how that goes, before we start doing workarounds like that.

[njames93]

In D118104#3279524, @JesApp wrote:

I can (re)start a build manually. Let's see how that goes, before we start doing workarounds like that.

I think there's just some serious latency problems with the pre merge checks atm

[JesApp]

In D118104#3279654, @njames93 wrote:

In D118104#3279524, @JesApp wrote:

I can (re)start a build manually. Let's see how that goes, before we start doing workarounds like that.

I think there's just some serious latency problems with the pre merge checks atm

I guess you are right, since the build has now passed.

Any other changes I should include before this can be merged?

[salman-javed-nz]

Do you reckon it's worth expanding the test: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp to look at this issue?
There isn't much testing of the run-clang-tidy.py at the moment, but it doesn't have to always remain that way.

[JesApp]

Well, since this was more of source of confusion than actual incorrect behaviour, I don't think there should be a test for it.

In general though, I think the script is complex enough to warrant some testing. That being said: I don't think they should be part of this patch. Also, I'm doing this on company time and I don't think my boss would be to happy if I wrote a testsuite from scratch when I just wanted to fix one bug. :D

[LegalizeAdulthood]

In D118104#3292862, @JesApp wrote:

In general though, I think the script is complex enough to warrant some testing.

What is the feasibility of Python unit tests Instead of a heavy-weight lit oriented test?

[JesApp]

In D118104#3294883, @LegalizeAdulthood wrote:

What is the feasibility of Python unit tests Instead of a heavy-weight lit oriented test?

I don't know the code-base well enough to answer that. Is python used for testing purposes anywhere else? I'm pretty sure the actual effort of writing the tests would be roughly the same, regardless.

Anyway, I'm new here, so I don't want to tell you guys how to run things, but: In the projects I usually work on, a fix would get merged quickly and things like "let's write a test suite for this script" would go in the backlog. What's your opinion on that?

[salman-javed-nz]

In D118104#3292862, @JesApp wrote:

Well, since this was more of source of confusion than actual incorrect behaviour, I don't think there should be a test for it.

In general though, I think the script is complex enough to warrant some testing. That being said: I don't think they should be part of this patch. Also, I'm doing this on company time and I don't think my boss would be to happy if I wrote a testsuite from scratch when I just wanted to fix one bug. :D

The idea I had was a file in clang-tools-extra\test\clang-tidy\infrastructure with something like this:

// RUN: %run_clang_tidy -checks="-*,google-explicit-constructor,modernize-use-auto" %s | FileCheck -check-prefix=CHECK-ENABLED-CHECKS-1 %s
// CHECK-ENABLED-CHECKS-1: Enabled checks:
// CHECK-ENABLED-CHECKS-1-NEXT: google-explicit-constructor
// CHECK-ENABLED-CHECKS-1-NEXT: modernize-use-auto

// RUN: %run_clang_tidy -checks="-*,google-explicit-constructor,modernize-use-auto" -config "Checks: -modernize-use-auto" %s | FileCheck -check-prefix=CHECK-ENABLED-CHECKS-2 %s
// CHECK-ENABLED-CHECKS-2: Enabled checks:
// CHECK-ENABLED-CHECKS-2-NEXT: google-explicit-constructor
// CHECK-ENABLED-CHECKS-2-NOT: modernize-use-auto

Note: I haven't tested this. If it doesn't work, then it's probably a minor tweak away from working.

This would confirm that the "Enabled checks:" message reflects the combined results of -checks and -config arguments.
I was wondering whether a test like the one above was an easy enough win to consider bundling in with the fix. In my initial comment I just wanted to bring up the topic, not to make a call on whether it should be done or not.

[njames93]

In D118104#3296340, @salman-javed-nz wrote:

In D118104#3292862, @JesApp wrote:

Well, since this was more of source of confusion than actual incorrect behaviour, I don't think there should be a test for it.

In general though, I think the script is complex enough to warrant some testing. That being said: I don't think they should be part of this patch. Also, I'm doing this on company time and I don't think my boss would be to happy if I wrote a testsuite from scratch when I just wanted to fix one bug. :D

The idea I had was a file in clang-tools-extra\test\clang-tidy\infrastructure with something like this:

// RUN: %run_clang_tidy -checks="-*,google-explicit-constructor,modernize-use-auto" %s | FileCheck -check-prefix=CHECK-ENABLED-CHECKS-1 %s
// CHECK-ENABLED-CHECKS-1: Enabled checks:
// CHECK-ENABLED-CHECKS-1-NEXT: google-explicit-constructor
// CHECK-ENABLED-CHECKS-1-NEXT: modernize-use-auto

// RUN: %run_clang_tidy -checks="-*,google-explicit-constructor,modernize-use-auto" -config "Checks: -modernize-use-auto" %s | FileCheck -check-prefix=CHECK-ENABLED-CHECKS-2 %s
// CHECK-ENABLED-CHECKS-2: Enabled checks:
// CHECK-ENABLED-CHECKS-2-NEXT: google-explicit-constructor
// CHECK-ENABLED-CHECKS-2-NOT: modernize-use-auto

Note: I haven't tested this. If it doesn't work, then it's probably a minor tweak away from working.

This would confirm that the "Enabled checks:" message reflects the combined results of -checks and -config arguments.
I was wondering whether a test like the one above was an easy enough win to consider bundling in with the fix. In my initial comment I just wanted to bring up the topic, not to make a call on whether it should be done or not.

FWIW the --checks option takes priority over the --config option.
If a check is enabled via --checks, but disabled via --config, the check will be enabled.
If a check is disabled via --checks, but enabled via --config, the check will be disabled.

[salman-javed-nz]

My bad. Test should have called run-clang-tidy with -checks=-some-check in the first test case, and with -config "Checks: -some-check" in the second. In both cases, the disabled check should not appear in the Enabled checks: printout.

There could be third test case where neither -checks nor -config are provided - this should display the default checks.

Anyway, I don't want what was a drive-by comment by me baloon into a lot of extra work for the author, so I will not push for it, unless others really want it.

[LegalizeAdulthood]

In D118104#3296467, @salman-javed-nz wrote:

Anyway, I don't want what was a drive-by comment by me baloon into a lot of extra work for the author, so I will not push for it, unless others really want it.

I think it's reasonable to accept this change and have the tests added as a follow-up change.

[LegalizeAdulthood]

In D118104#3295876, @JesApp wrote:

In D118104#3294883, @LegalizeAdulthood wrote:

What is the feasibility of Python unit tests Instead of a heavy-weight lit oriented test?

I don't know the code-base well enough to answer that. Is python used for testing purposes anywhere else?

I'm not sure; this is a little bit unusual because there is not much production
python code in llvm/clang. I'm actually bumping into some grief in check_clang_tidy.py
that might go easier if I were to unit test some of the python itself.

Anyway, I'm new here, so I don't want to tell you guys how to run things,
but: In the projects I usually work on, a fix would get merged quickly and things
like "let's write a test suite for this script" would go in the backlog. What's your
opinion on that?

In this case, I think it's fine. Generally speaking though, the coding guidelines for llvm/clang
require that tests be added alongside the new features or bug fixes. This is certainly the case
when we add new checks, for instance.

See https://llvm.org/docs/DeveloperPolicy.html#test-cases

[JesApp]

I'm not so much opposed to writing a test case for the one change I made (especially after @salman-javed-nz suggestion) as I am opposed to learning the build system. I've tried to set for a few minutes and oh man, that's a lot of targets...

So yeah, If someone could merge it like this, I'd be grateful.

[aaron.ballman]

In D118104#3297315, @LegalizeAdulthood wrote:

In D118104#3296467, @salman-javed-nz wrote:

Anyway, I don't want what was a drive-by comment by me baloon into a lot of extra work for the author, so I will not push for it, unless others really want it.

I think it's reasonable to accept this change and have the tests added as a follow-up change.

Normally I'd be opposed to this, but given the testing difficulties with this tool are not new and this patch doesn't make them substantially worse, I think it's fine to land as-is and address testing in a follow-up.

[JesApp]

Thanks for accepting. Do I need to do anything to get this merged or is this basically done?

[aaron.ballman]

In D118104#3303665, @JesApp wrote:

Thanks for accepting. Do I need to do anything to get this merged or is this basically done?

Oops, sorry for failing to ask if you needed this committed on your behalf. :-) What name and email address would you like me to use for patch attribution?

[JesApp]

No problem. :)

Jesko Appelfeller, [e-mail] please.

[aaron.ballman]

I've commit on your behalf in 0851970af5771f6721c29d066c88a72db823ba83, thank you!

[JesApp]

Thank you! :)

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

def apply_fixes(args, tmpdir):
   if args.format:
     invocation.append('-format')
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
 
 def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
     name = queue.get()
     invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
                                      args.allow_enabling_alpha_checkers,
                                      args.extra_arg, args.extra_arg_before,
                                      args.quiet, args.config, args.line_filter)
def main():
   db_path = 'compile_commands.json'
 
   if args.build_path is not None:
     build_path = args.build_path
   else:
     # Find our database
     build_path = find_compilation_database(db_path)
 
   try:
-    invocation = [args.clang_tidy_binary, '-list-checks']
-    if args.allow_enabling_alpha_checkers:
-      invocation.append('-allow-enabling-analyzer-alpha-checkers')
-    invocation.append('-p=' + build_path)
-    if args.checks:
-      invocation.append('-checks=' + args.checks)
+    invocation = get_tidy_invocation("", args.clang_tidy_binary, args.checks,
+                                     None, build_path, args.header_filter,
+                                     args.allow_enabling_alpha_checkers,
+                                     args.extra_arg, args.extra_arg_before,
+                                     args.quiet, args.config, args.line_filter)
+    invocation.append('-list-checks')
     invocation.append('-')
     if args.quiet:
       # Even with -quiet we still want to check if we can call clang-tidy.
       with open(os.devnull, 'w') as dev_null:
         subprocess.check_call(invocation, stdout=dev_null)
     else:
       subprocess.check_call(invocation)
   except:
     print("Unable to run clang-tidy.", file=sys.stderr)