[PATCH] D139458: [clangd] Full support for #import insertions

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ASTSignals.cpp:28
+  // Source files: Use #import if the ObjC language flag is enabled
+  if (isHeaderFile(AST.tuPath(), AST.getLangOpts())) {
+    if (AST.getLangOpts().ObjC) {
----------------
can we rewrite this as:
```
bool preferImports(const ParsedAST &AST) {
  // If we don't have any objc-ness, always prefer #include
  if (!AST.getLangOpts().ObjC)
     return false;
  // If this is not a header file and has objc set as language, prefer #import
  if (!isHeaderFile(...))
     return true;
  // Headers lack proper compile flags most of the time, so we might treat a header as objc accidentally.
  // Perform some extra checks to make sure this works. 

  // Any file with a #import, should keep #import-ing.
  for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
    if (Inc.Directive == tok::pp_import)
       return true;
  }
  
  // Any file declaring an objc struct should also #import.
  // No need to look over the references, as the file doesn't have any #imports, it must be declaring interesting Objc-like decls.
  for(Decl *D: AST.getLocalTopLevelDecls()) {
        if (isa<...InterestingDeclTypes...>(D)
           return true;
   });
  return false;
}
```

It's fine to run findExplicitReferences for an extra time here, as we do it only when this is an ObjC header with no `#import`s


================
Comment at: clang-tools-extra/clangd/ASTSignals.h:33
   llvm::StringMap<unsigned> RelatedNamespaces;
+  /// Whether include insertion should suggest imports or includes.
+  Symbol::IncludeDirective InsertionDirective =
----------------
can we rather say `Preferred PP-directive to use for inclusions by the file.`


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:569
       }
+      Symbol::IncludeDirective Directive = Symbol::IncludeDirective::Include;
+      if (Signals)
----------------
we already have access to `Clang->getLangOpts()` here. we also have the `FileName` available so we can check for header-ness as well. we also have preamble includes to scan for #import directives (well, we might be building an AST without a preamble sometimes but it's fine if IncludeFixer can't suggest `#import`s in those cases).

so can we detect import-ness based on these here? we're only lacking information about whether the file declares any objc symbols, e.g. we might fail to suggest #import directives in a objc file without any #import statements, but I'd say it's fine considering all the extra complexity needed to propagate it from previous run. (if this turns out to be an important UX issue we can consider plumbing this information directly, so please leave a FIXME).


================
Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:91
+  HeaderTU.Code = R"cpp(
+  #include "TestTU.h"
+  )cpp";
----------------
can you put this test first with a comment like `objc lang option is not enough for headers`?


================
Comment at: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp:97
+  HeaderTU.Code = R"cpp(
+  #include "TestTU.h"
+
----------------
this only works because we actually implicitly `#import` contents of `HeaderTU.HeaderCode` in TestTU.

can you instead clear up the  `HeaderCode` and define the interface directly in the header file?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2727
 
-  Results = completions("Fun^", {Sym}, {}, "Foo.m").Completions;
+  CodeCompleteOptions Opts;
+  Opts.ImportInsertions = true;
----------------
can you use this `Opts` (without Signals) in the previous test as well to make sure we're `#include`ing the symbol in a non-import context even if the option is set?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139458/new/

https://reviews.llvm.org/D139458