[CodeCompletion] Generally consider header files without extension

[ckandeler]

Summary

Real-world use case: The Qt framework's headers have the same name
as the respective class defined in them, and Qt's traditional qmake
build tool uses -I (rather than -isystem) to pull them in.

[ckandeler]

I hope this isn't too controversial. After all, the files are located in include directories, so there shouldn't be any random garbage getting picked up.

[sammccall]

the files are located in include directories

These are directories that may contain headers, not directories that only contain headers. (Which we mostly expect -Isystem to be).
For example, many projects keep headers next to sources, and so have sources on their include path. And the directory containing the current file is always on the include path.
We'd expect to see Makefile, SConstruct, BUILD files etc here. And maybe a smattering of "random" files that don't follow particular conventions.

Trying to support QT headers seems very reasonable though. Seems like our options are:

• current behavior with false negatives on QT
• proposed behavior with false positives on Makefile etc
• current behavior and try to detect QT as an exception
• proposed behavior and try to detect Makefile etc as exceptions

WDYT about detecting QT headers specifically? It seems hacky, but I don't see a way out of this that doesn't involve hardcoding some filenames. Are they in a directory like "qt-11/QFoo" that we can recognize? Even Q followed by another capital letter might be a good enough heuristic.
(The docs suggest it's just <QFoo> but the docs also say to use angle brackets so I'm not sure whether to believe them)

[ckandeler]

WDYT about detecting QT headers specifically? It seems hacky, but I don't see a way out of this that doesn't involve hardcoding some filenames. Are they in a directory like "qt-11/QFoo" that we can recognize? Even Q followed by another capital letter might be a good enough heuristic.
(The docs suggest it's just <QFoo> but the docs also say to use angle brackets so I'm not sure whether to believe them)

The headers are, as far as I can tell, always located in a directory whose name starts with "Qt". This parent directory is also in the include path, so e.g. to get access to QString, which is located under QtCore/, you'd typically just write:

#include <QString>

This is the recommended, documented way of pulling in headers.
Though you could also write:

#include <QtCore/QString>

as the parent parent directory is also in the list of include paths.

Looking at the code, it seems we have access to the parent directory, so we could do that name check (which I suppose has less potential for false positives than checking the file name).
For framework builds, the directory would be "Headers", which also seems safe.

[sammccall]

In D112996#3102925, @ckandeler wrote:

WDYT about detecting QT headers specifically? It seems hacky, but I don't see a way out of this that doesn't involve hardcoding some filenames. Are they in a directory like "qt-11/QFoo" that we can recognize? Even Q followed by another capital letter might be a good enough heuristic.
(The docs suggest it's just <QFoo> but the docs also say to use angle brackets so I'm not sure whether to believe them)

The headers are, as far as I can tell, always located in a directory whose name starts with "Qt". This parent directory is also in the include path, so e.g. to get access to QString, which is located under QtCore/, you'd typically just write:

#include <QString>

This is the recommended, documented way of pulling in headers.
Though you could also write:

#include <QtCore/QString>

as the parent parent directory is also in the list of include paths.

Looking at the code, it seems we have access to the parent directory, so we could do that name check (which I suppose has less potential for false positives than checking the file name).

Yeah, that makes sense to me. Also a bit cheaper since we only have to do this once per parent dir.

For framework builds, the directory would be "Headers", which also seems safe.

I agree extensionless headers in frameworks seem fine to show.
We already know which includepath entries are frameworks, so we can just use that info directly (see line 9674)

[ckandeler]

Limited the matching to Qt headers.

[ckandeler]

For framework builds, the directory would be "Headers", which also seems safe.

I agree extensionless headers in frameworks seem fine to show.
We already know which includepath entries are frameworks, so we can just use that info directly (see line 9674)

These aren't technically framework includes, as in order to make prefix-less headers work, the include path has to point directly into the Headers directory.

[sammccall]

In D112996#3105783, @ckandeler wrote:

For framework builds, the directory would be "Headers", which also seems safe.

I agree extensionless headers in frameworks seem fine to show.
We already know which includepath entries are frameworks, so we can just use that info directly (see line 9674)

These aren't technically framework includes, as in order to make prefix-less headers work, the include path has to point directly into the Headers directory.

I see. In that case we can detect this pattern in any framework by checking whether the directory contains ".framework/Headers", right? Headers itself doesn't seem specific enough.

clang/lib/Sema/SemaCodeComplete.cpp

9647

can you extract this up near dirname, and give a name that explains the idea (e.g. ExtensionlessHeaders)

9647

we've lost the condition that there's no extension, and now accept *any* extension (e.g. .DS_Store!)
Unless this is important I think we should keep that check.

And to keep the number of cases/paths low, I think it's fine to bundle system headers into the same bucket - really this is just for the C++ standard library, which does not have extensions.

So some logic like:

bool ExtensionlessHeaders = IsSystem || Dir.endswith(".framework/Headers") || Dirname.startswith("Qt");
...
case regular_file:
  bool IsHeader = Filename.endswith(...) || Filename.endswith(...) || (ExtensionlessHeaders && !Filename.contains('.'));
  if (!IsHeader) break;

[ckandeler]

Addressed comments.

[sammccall]

LG, thanks! can we the tests again though?

[ckandeler]

clang/lib/Sema/SemaCodeComplete.cpp

9619

I'm just noticing that this is often a symlink into a versioned directory (e.g. xxx.framework/Version/5/Headers). The include paths are not canonicalized, I hope?

[ckandeler]

In D112996#3106459, @sammccall wrote:

can we the tests again though?

Sorry, I don't understand what you mean by that.

[ckandeler]

Addressed formatting comments.

[ckandeler]

Can someone please merge this?

[sammccall]

In D112996#3106469, @ckandeler wrote:

In D112996#3106459, @sammccall wrote:

can we the tests again though?

Sorry, I don't understand what you mean by that.

Apologies, I mangled the comment.

This should have a test.

[ckandeler]

Added test cases.

[sammccall]

Thanks Christian! Sorry for the delay.

[HazardyKnusperkeks]

I have a similar structure to Qt regarding the headers and use clangd trough the QtCreator, so for my own libraries I cannot use the auto completion.

In D112996#3102782, @sammccall wrote:

the files are located in include directories

These are directories that may contain headers, not directories that only contain headers. (Which we mostly expect -Isystem to be).
For example, many projects keep headers next to sources, and so have sources on their include path. And the directory containing the current file is always on the include path.
We'd expect to see Makefile, SConstruct, BUILD files etc here. And maybe a smattering of "random" files that don't follow particular conventions.

Trying to support QT headers seems very reasonable though. Seems like our options are:

• current behavior with false negatives on QT
• proposed behavior with false positives on Makefile etc
• current behavior and try to detect QT as an exception
• proposed behavior and try to detect Makefile etc as exceptions

WDYT about detecting QT headers specifically? It seems hacky, but I don't see a way out of this that doesn't involve hardcoding some filenames. Are they in a directory like "qt-11/QFoo" that we can recognize? Even Q followed by another capital letter might be a good enough heuristic.
(The docs suggest it's just <QFoo> but the docs also say to use angle brackets so I'm not sure whether to believe them)

Could we extend the heuristic to match directories called include? For me it is

Path to Library                   .../code/cast
- include                         .../code/cast/include
  - LibraryName                   .../code/cast/include/Cast
    - Header                      .../code/cast/include/Cast/Signed

And for that I want #include <Cast/ to offer Signed>.

clang/lib/Sema/SemaCodeComplete.cpp

if (!NativeRelDir.empty()) {
 
         llvm::sys::path::append(Dir, *Begin + ".framework", "Headers");
         llvm::sys::path::append(Dir, ++Begin, End);
       } else {
         llvm::sys::path::append(Dir, NativeRelDir);
       }
     }
 
+    const StringRef &Dirname = llvm::sys::path::filename(Dir);
+    const bool isQt = Dirname.startswith("Qt") || Dirname == "ActiveQt";
+    const bool ExtensionlessHeaders =
+        IsSystem || isQt || Dir.endswith(".framework/Headers");
     std::error_code EC;
     unsigned Count = 0;
     for (auto It = FS.dir_begin(Dir, EC);
          !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) {
       if (++Count == 2500) // If we happen to hit a huge directory,
         break;             // bail out early so we're not too slow.
       StringRef Filename = llvm::sys::path::filename(It->path());
 
for (auto It = FS.dir_begin(Dir, EC);
         // All entries in a framework directory must have a ".framework" suffix,
         // but the suffix does not appear in the source code's include/import.
         if (LookupType == DirectoryLookup::LT_Framework &&
             NativeRelDir.empty() && !Filename.consume_back(".framework"))
           break;
 
         AddCompletion(Filename, /*IsDirectory=*/true);
         break;
-      case llvm::sys::fs::file_type::regular_file:
-        // Only files that really look like headers. (Except in system dirs).
-        if (!IsSystem) {
-          // Header extensions from Types.def, which we can't depend on here.
-          if (!(Filename.endswith_insensitive(".h") ||
+      case llvm::sys::fs::file_type::regular_file: {
+        // Only files that really look like headers. (Except in special dirs).
+        const bool IsHeader = Filename.endswith_insensitive(".h") ||
                               Filename.endswith_insensitive(".hh") ||
                               Filename.endswith_insensitive(".hpp") ||
-                Filename.endswith_insensitive(".inc")))
-            break;
-        }
+                              Filename.endswith_insensitive(".inc") ||
+                              (ExtensionlessHeaders && !Filename.contains('.'));
+        if (!IsHeader)
         AddCompletion(Filename, /*IsDirectory=*/false);
         break;
+      }
       default:
         break;
       }
     }
   };
 
   // Helper: adds results relative to IncludeDir, if possible.
   auto AddFilesFromDirLookup = [&](const DirectoryLookup &IncludeDir,

clang/test/CodeCompletion/included-files.cpp

-// RUN: rm -rf %t && mkdir %t && cp %s %t/main.cc && mkdir %t/a
-// RUN: touch %t/foo.h && touch %t/foo.cc && touch %t/a/foosys %t/a/foosys.h
+// RUN: rm -rf %t && mkdir %t && cp %s %t/main.cc && mkdir %t/a && mkdir %t/QtCore && mkdir %t/Headers %t/Some.framework %t/Some.framework/Headers
+// RUN: touch %t/foo.h && touch %t/foo.cc && touch %t/a/foosys %t/a/foosys.h && touch %t/QtCore/foosys %t/QtCore/foo.h
+// RUN: touch %t/Headers/foosys %t/Headers/foo.h %t/Some.framework/Headers/foosys %t/Some.framework/Headers/foo.h
 
 // Quoted string shows header-ish files from CWD, and all from system.
 #include "foo.h"
-// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:5:13 %t/main.cc | FileCheck -check-prefix=CHECK-1 %s
+// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:6:13 %t/main.cc | FileCheck -check-prefix=CHECK-1 %s
 // CHECK-1-NOT: foo.cc"
 // CHECK-1: foo.h"
 // CHECK-1: foosys"
 
 // Quoted string with dir shows header-ish files in that subdir.
 #include "a/foosys"
-// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:12:13 %t/main.cc | FileCheck -check-prefix=CHECK-2 %s
+// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:13:13 %t/main.cc | FileCheck -check-prefix=CHECK-2 %s
 // CHECK-2-NOT: foo.h"
 // CHECK-2: foosys.h"
 // CHECK-2-NOT: foosys"
 
 // Angled shows headers from system dirs.
 #include <foosys>
-// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:19:13 %t/main.cc | FileCheck -check-prefix=CHECK-3 %s
+// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:20:13 %t/main.cc | FileCheck -check-prefix=CHECK-3 %s
 // CHECK-3-NOT: foo.cc>
 // CHECK-3-NOT: foo.h>
 // CHECK-3: foosys>
 
 // With -I rather than -isystem, the header extension is required.
 #include <foosys>
-// RUN: %clang -fsyntax-only -I %t/a -Xclang -code-completion-at=%t/main.cc:26:13 %t/main.cc | FileCheck -check-prefix=CHECK-4 %s
+// RUN: %clang -fsyntax-only -I %t/a -Xclang -code-completion-at=%t/main.cc:27:13 %t/main.cc | FileCheck -check-prefix=CHECK-4 %s
 // CHECK-4-NOT: foo.cc>
 // CHECK-4-NOT: foo.h>
 // CHECK-4-NOT: foosys>
 
 // Backslash handling.
 #include "a\foosys"
-// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:33:13 %t/main.cc -fms-compatibility | FileCheck -check-prefix=CHECK-5 %s
+// RUN: %clang -fsyntax-only -isystem %t/a -Xclang -code-completion-at=%t/main.cc:34:13 %t/main.cc -fms-compatibility | FileCheck -check-prefix=CHECK-5 %s
 // CHECK-5: foosys.h"
+
+// Qt headers don't necessarily have extensions.
+#include <foosys>
+// RUN: %clang -fsyntax-only -I %t/QtCore -Xclang -code-completion-at=%t/main.cc:39:13 %t/main.cc -fms-compatibility | FileCheck -check-prefix=CHECK-6 %s
+// CHECK-6-NOT: foo.cc>
+// CHECK-6: foo.h>
+// CHECK-6: foosys>
+
+// If the include path directly points into a framework's Headers/ directory, we allow extension-less headers.
+#include <foosys>
+// RUN: %clang -fsyntax-only -I %t/Some.framework/Headers -Xclang -code-completion-at=%t/main.cc:46:13 %t/main.cc -fms-compatibility | FileCheck -check-prefix=CHECK-7 %s
+// CHECK-7-NOT: foo.cc>
+// CHECK-7: foo.h>
+// CHECK-7: foosys>
+
+// Simply naming a directory "Headers" is not enough to allow extension-less headers.
+#include <foosys>
+// RUN: %clang -fsyntax-only -I %t/Headers -Xclang -code-completion-at=%t/main.cc:53:13 %t/main.cc -fms-compatibility | FileCheck -check-prefix=CHECK-8 %s
+// CHECK-8-NOT: foo.cc>
+// CHECK-8: foo.h>
+// CHECK-8-NOT: foosys>