Page MenuHomePhabricator

[clang-tidy][test] Allow empty checks in check_clang_tidy.py
Needs ReviewPublic

Authored by gamesh411 on Sep 17 2020, 6:55 AM.

Details

Summary

Currently there is no way to assert that a check does not produce warnings for
a specific run. The CHECK-NOT directive can be used, but an empty file is
always discarded by FileCheck. Extend the FileCheck invocation with
'--allow-empty' to support this use-case.

This allows the following usage:

// RUN: %check_clang_tidy %s -check-suffix=CUSTOM <check-name> %t
// CHECK-NOTES-CUSTOM-NOT: NO DIAGS

Diff Detail

Event Timeline

gamesh411 created this revision.Sep 17 2020, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 6:55 AM
gamesh411 requested review of this revision.Sep 17 2020, 6:55 AM
gamesh411 updated this revision to Diff 292485.Sep 17 2020, 7:00 AM

Update commit msg with example

gamesh411 updated this revision to Diff 292487.Sep 17 2020, 7:06 AM

Tidy up commit message

gamesh411 edited the summary of this revision. (Show Details)Sep 17 2020, 7:06 AM

Do you have some thoughts about this, should this be pursued, or do you think the use-case is not relevant?

Thank you for the changes, but do you know of any tests that are impacted by this? I'm not opposed to the proposed changes, but I am wondering -- the current behavior is that an empty output file (which can happen with especially egregious bugs) is always an error but if I understand the proposed patch, that may now be silently accepted as a passing test? So I'm wondering if we want to require something like // expected-no-diagnostics from the -verify command in Clang so that users have to explicitly opt a test into the behavior? Or should this not be an issue in general because // CHECK lines should still be present in the test and those will fail when compared against an empty file?

know of any tests that are impacted by this?

I haven't found any tidy-tests that were negative-tests (ie.: tests that assert that there are no diagnostics).

... if I understand the proposed patch, that may now be silently accepted as a passing test?

This is not entirely the case. Let me demonstrate with a few cases:
Before this patch:

// empty test
// RUN: %check_clang_tidy %s %t
// no CHECK lines in the entire file

In the above case the test will fail with CHECK-FIXES, CHECK-MESSAGES or CHECK-NOTES not found in the input.

// empty test, but explicityly mention we dont want diagnostics
// RUN: %check_clang_tidy %s %t
// CHECK_MESSAGES_NOT: ARBITRARY STRING HERE
// ...
// there are *no* other CHECK messages

This passes the first round of sanity checking, but FileCheck nevertheless rejects the processed test file:
FileCheck error: '/home/gamesh411/clang-rwa/tools/clang/tools/extra/test/clang-tidy/checkers/Output/empty.cpp.tmp.cpp.msg' is empty.

// empty test, but explicityly mention we dont want diagnostics
// RUN: %check_clang_tidy %s %t
// CHECK_MESSAGES_NOT: arbitrary string here, I prefer NO DIAG
// ...
// there *are* some other CHECK messages further down

This is fine, but only means that the arbitrary string diagnostic is expected to be *not* seen, while some others are.

After patch:
The second case above becomes fine, everything else stays the same.

Or should this not be an issue in general because // CHECK lines should still be present in the test and those will fail when compared against an empty file?

This means that the only accidental empty test scenario is when someone accidentally skips the CHECK messages he wants to put in, *but* manages to put in the CHECK-NOT by accident as well. This can result from copy-paste, but still, I think this is not likely.

I'm wondering if we want to require something like // expected-no-diagnostics from the -verify command in Clang so that users have to explicitly opt a test into the behavior?

My thoughts were exactly that; I need something like the DiagVerifier's expected-no-diagnostics. That was the motivation behind this patch.

Probably not quite as verbose but should do the job

// RUN: clang-tidy %s --checks=-*,my-check-to-test --warnings-as-errors=* -- -std=c++11

Probably not quite as verbose but should do the job

// RUN: clang-tidy %s --checks=-*,my-check-to-test --warnings-as-errors=* -- -std=c++11

Thanks 👍 ! I can live with this solution as well. If however there is a suggestion on how to handle this in an idiomatic way, please share, maybe I can help implement it.