[clang-tidy] Fix relative path in header-filter.

[xyb]

Summary

Clang-tidy supports output diagnostics from header files if user
specifies --header-filter. But it can't handle relative path well.
For example, the folder structure of a project is:

// a.h is in /src/a/a.h
#include "../b/b.h"

// b.h is in /src/b/b.h
...

// c.cpp is in /src/c.cpp
#include "a/a.h"

Now, we set --header-filter as --header-filter=/a/. That means we only
want to check header files under /src/a/ path, and ignore header files
uder /src/b/ path, but in current implementation, clang-tidy will check
/src/b/b.h also, because the name of b.h used in clang-tidy is
/src/a/../b/b.h.

This change tries to fix this issue.

[gribozavr]

clang-tools-extra/test/clang-tidy/file-filter.cpp

60 ↗

(On Diff #219919)

Could you rename header.h files to header_a.h, header_b.h etc. to make it obvious in CHECK lines where the messages are coming from?

[MyDeveloperDay]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

555 ↗

(On Diff #219919)

elide the {}

[xyb]

Updated

[gribozavr]

Thanks! Please let me know if you need me to commit the patch for you.

[xyb]

Yes, I need your help to submit the patch. I don't have the permission. Thanks.

[gribozavr]

Sorry, I reverted this patch in r372601.

Unfortunately, it makes paths printed in clang-tidy'd diagnostics inconsistent with what -header-filter operates on.

For example, imagine that file-filter.cpp includes header_alias.h, which is a symlink to header.h. The diagnostics printed by clang-tidy refer to header_alias.h, however, after this patch, -header-filter logic calls realpath() and operates on header.h -- making it very difficult for users to understand how exactly to set up filters.

Also, note that paths in diagnostics don't collapse foo/.. or symlinks.

We should only change both sides of path handling simultaneously (paths in diagnostics and paths in filters). However, since getting the user's preferred path is potentially very difficult when symlinks are present, I'm not sure if anything can be done here. Maybe only collapsing foo/.. would be viable?

[gribozavr]

I added a test in r372607 for your reference. It tests both foo/.. and symlink behavior.

Maybe only collapsing foo/.. would be viable?

@ilya-biryukov told me that even that is not viable, in case foo is a symlink to a directory.

The only workable suggestion that we could come up with was adding a separate command-line option, -normalized-header-filter, that would normalize paths before matching the regex. It can be used by people whose project layout does not have complex symlink mazes.

[alexfh]

In D67501#1678962, @gribozavr wrote:

Sorry, I reverted this patch in r372601.

Unfortunately, it makes paths printed in clang-tidy'd diagnostics inconsistent with what -header-filter operates on.

For example, imagine that file-filter.cpp includes header_alias.h, which is a symlink to header.h. The diagnostics printed by clang-tidy refer to header_alias.h, however, after this patch, -header-filter logic calls realpath() and operates on header.h -- making it very difficult for users to understand how exactly to set up filters.

Also, note that paths in diagnostics don't collapse foo/.. or symlinks.

We should only change both sides of path handling simultaneously (paths in diagnostics and paths in filters). However, since getting the user's preferred path is potentially very difficult when symlinks are present, I'm not sure if anything can be done here. Maybe only collapsing foo/.. would be viable?

Even collapsing foo/.. may be problematic , if foo is a symlink.

$ mkdir -p a/b/c
$ ln -s a/b/c d
$ ls
a d
$ ls a/..
a d
$ ls d/..
c

[xyb]

The only workable suggestion that we could come up with was adding a separate command-line option, -normalized-header-filter, that would normalize paths before matching the regex. It can be used by people whose project layout does not have complex symlink mazes.

-normalized-header-filter. I'd like this idea. Any objections?

[ilya-biryukov]

In D67501#1679050, @xyb wrote:

-normalized-header-filter. I'd like this idea. Any objections?

Having two solution for filter is unfortunate - making both discoverable and documenting why we need them is hard.
If there are important use-cases that can't be served otherwise, I guess there's no way to avoid it.

However, if there are workaround (even if they're slightly annoying), it's much simpler to have a single option.

clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
   // -DMACRO definitions on the command line have locations in a virtual buffer
   // that doesn't have a FileEntry. Don't skip these as well.
   if (!File) {
     LastErrorRelatesToUserCode = true;
     LastErrorPassesLineFilter = true;
     return;
   }
 
-  StringRef FileName(File->getName());
+  StringRef FileName = File->tryGetRealPathName();
+  if (FileName.empty())
+    FileName = File->getName();
   LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
                                Sources.isInMainFile(Location) ||
                                getHeaderFilter()->match(FileName);
 
   unsigned LineNumber = Sources.getExpansionLineNumber(Location);
   LastErrorPassesLineFilter =
       LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
 }

clang-tools-extra/trunk/test/clang-tidy/Inputs/file-filter/subfolder_a/header_a.h

+#include "../subfolder_b/header_b.h"
+
+class SA { SA(int); };

clang-tools-extra/trunk/test/clang-tidy/Inputs/file-filter/subfolder_b/header_b.h

+class SB { SB(int); };

clang-tools-extra/trunk/test/clang-tidy/Inputs/file-filter/subfolder_c/header_c.h

+class SC { SC(int); };

clang-tools-extra/trunk/test/clang-tidy/file-filter.cpp

 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK-QUIET %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK2 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK2-QUIET %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header2\.h' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK3 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='header2\.h' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK3-QUIET %s
 // FIXME: "-I %S/Inputs/file-filter/system/.." must be redundant.
 //       On Win32, file-filter/system\system-header1.h precedes
 //       file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_a' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_b' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK6-QUIET %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7 %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='subfolder_c' -quiet %s -- -I %S/Inputs/file-filter -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK7-QUIET %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
 // CHECK-QUIET-NOT: warning:
 // CHECK2: header1.h:1:12: warning: single-argument constructors must be marked explicit
 // CHECK2-QUIET: header1.h:1:12: warning: single-argument constructors must be marked explicit
 // CHECK3-NOT: warning:
 // CHECK3-QUIET-NOT: warning:
 // CHECK4: header1.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header1.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
 
 #include "header2.h"
 // CHECK-NOT: warning:
 // CHECK-QUIET-NOT: warning:
 // CHECK2: header2.h:1:12: warning: single-argument constructors
 // CHECK2-QUIET: header2.h:1:12: warning: single-argument constructors
 // CHECK3: header2.h:1:12: warning: single-argument constructors
 // CHECK3-QUIET: header2.h:1:12: warning: single-argument constructors
 // CHECK4: header2.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: header2.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
+
+#include "subfolder_a/header_a.h"
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+// CHECK2: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK2-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK3-NOT: warning:
+// CHECK3-QUIET-NOT: warning:
+// CHECK4: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK4-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK5: header_a.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK5-QUIET: header_a.h:3:12: warning: single-argument constructors must be marked explicit
+// CHECK6: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK6-QUIET: header_b.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
+
+#include "subfolder_c/header_c.h"
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+// CHECK2: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK2-QUIET: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK3-NOT: warning:
+// CHECK3-QUIET-NOT: warning:
+// CHECK4: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK4-QUIET: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7: header_c.h:1:12: warning: single-argument constructors must be marked explicit
+// CHECK7-QUIET: header_c.h:1:12: warning: single-argument constructors must be marked explicit
 
 #include <system-header.h>
 // CHECK-NOT: warning:
 // CHECK-QUIET-NOT: warning:
 // CHECK2-NOT: warning:
 // CHECK2-QUIET-NOT: warning:
 // CHECK3-NOT: warning:
 // CHECK3-QUIET-NOT: warning:
 // CHECK4: system-header.h:1:12: warning: single-argument constructors
 // CHECK4-QUIET: system-header.h:1:12: warning: single-argument constructors
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
 
 class A { A(int); };
 // CHECK: :[[@LINE-1]]:11: warning: single-argument constructors
 // CHECK-QUIET: :[[@LINE-2]]:11: warning: single-argument constructors
 // CHECK2: :[[@LINE-3]]:11: warning: single-argument constructors
 // CHECK2-QUIET: :[[@LINE-4]]:11: warning: single-argument constructors
 // CHECK3: :[[@LINE-5]]:11: warning: single-argument constructors
 // CHECK3-QUIET: :[[@LINE-6]]:11: warning: single-argument constructors
 // CHECK4: :[[@LINE-7]]:11: warning: single-argument constructors
 // CHECK4-QUIET: :[[@LINE-8]]:11: warning: single-argument constructors
+// CHECK5: :[[@LINE-9]]:11: warning: single-argument constructors
+// CHECK5-QUIET: :[[@LINE-10]]:11: warning: single-argument constructors
+// CHECK6: :[[@LINE-11]]:11: warning: single-argument constructors
+// CHECK6-QUIET: :[[@LINE-12]]:11: warning: single-argument constructors
+// CHECK7: :[[@LINE-13]]:11: warning: single-argument constructors
+// CHECK7-QUIET: :[[@LINE-14]]:11: warning: single-argument constructors
 
 // CHECK-NOT: warning:
 // CHECK-QUIET-NOT: warning:
 // CHECK2-NOT: warning:
 // CHECK2-QUIET-NOT: warning:
 // CHECK3-NOT: warning:
 // CHECK3-QUIET-NOT: warning:
 // CHECK4-NOT: warning:
 // CHECK4-QUIET-NOT: warning:
+// CHECK5-NOT: warning:
+// CHECK5-QUIET-NOT: warning:
+// CHECK6-NOT: warning:
+// CHECK6-QUIET-NOT: warning:
+// CHECK7-NOT: warning:
+// CHECK7-QUIET-NOT: warning:
 
-// CHECK: Suppressed 3 warnings (3 in non-user code)
+// CHECK: Suppressed 6 warnings (6 in non-user code)
 // CHECK: Use -header-filter=.* to display errors from all non-system headers.
 // CHECK-QUIET-NOT: Suppressed
 // CHECK2: Suppressed 1 warnings (1 in non-user code)
 // CHECK2: Use -header-filter=.* {{.*}}
 // CHECK2-QUIET-NOT: Suppressed
-// CHECK3: Suppressed 2 warnings (2 in non-user code)
+// CHECK3: Suppressed 5 warnings (5 in non-user code)
 // CHECK3: Use -header-filter=.* {{.*}}
 // CHECK3-QUIET-NOT: Suppressed
 // CHECK4-NOT: Suppressed {{.*}} warnings
 // CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+// CHECK5: Suppressed 5 warnings (5 in non-user code)
+// CHECK5: Use -header-filter=.* to display errors from all non-system headers.
+// CHECK5-QUIET-NOT: Suppressed
+// CHECK6: Suppressed 5 warnings (5 in non-user code)
+// CHECK6: Use -header-filter=.* to display errors from all non-system headers.
+// CHECK6-QUIET-NOT: Suppressed
+// CHECK7: Suppressed 5 warnings (5 in non-user code)
+// CHECK7: Use -header-filter=.* to display errors from all non-system headers.
+// CHECK7-QUIET-NOT: Suppressed