[PATCH] D152900: [clangd] Update symbol collector to use include-cleaner.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:393-395
+    const auto *FileEntry = SM.getFileEntryForID(FID);
+    for (const auto *Export : PI.getExporters(FileEntry, SM.getFileManager()))
+      return toURI(Export->tryGetRealPathName());
----------------
kadircet wrote:
> sorry i don't understand what this logic is trying to achieve.
> 
> we seem to be prefering "first" exporting header, over the headers that directly provide the symbol. Also comment says it's done for objc, but there's nothing limiting this logic to ObjC. any chance this is unintended?
No, this was intended, but possibly I didn't get it right.

>  comment says it's done for objc, but there's nothing limiting this logic to objc

AFAIU this whole code path will now only be reachable for objc symbols.  I have removed pragma includes from the canonical include mapping now, leaving the pragma handling to include cleaner. However, that only triggers for C/C++, so in case we need the `export` and `private` pragmas for objc, we need to re-insert the handling for them somewhere. 
There is no pre-existing test case for pragmas in objc and there is no usage of these pragmas for objc code in the code base either,  so I have no way of knowing if this is actually needed. But I believe you've mentioned in some discussion we should still handle the pragmas for objc.

> we seem to be preferring "first" exporting header, over the headers that directly provide the symbol.

Isn't that correct if there's an `IWYU export` pragma involved? The snippet comes from `include_cleaner::findHeaders`, with the only difference that it stops at the first exporter (since the canonical include mapping also just stored one mapping for the header).

Let me know how to do it better, or maybe if this is necessary at all. Honestly, I am not sure about this, since, as mentioned, there are no `export` or `private/public` pragmas in objc files in the codebase atm.



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:903
+        NewSym = *S;
+        if (!IncludeHeader.empty()) {
           NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
----------------
kadircet wrote:
> this is using legacy mappings for non-objc symbols too
Removed this whole passage.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:934-936
+              SymbolIncludeSpelling[SID] = HeaderFileURIs->getIncludeHeader(
+                  ASTCtx->getSourceManager().getOrCreateFileID(
+                      H.physical(), SrcMgr::CharacteristicKind::C_User));
----------------
kadircet wrote:
> we actually only want to use `HeaderFileURIs->toURI` here and not the `getIncludeHeader`, because:
> - We want to make use of mapping logic in include-cleaner solely, and we already have all that information in Providers mapping.
> - The extra logic we want to apply for ObjC symbols are already applied before reaching here.
> 
> the way we create a FileID here is a little bit iffy, by using `toURI`, hopefully we can avoid that conversion.
> 
> the only remaining issue is, we actually still want to use system header mappings inside `CanonicalIncludes` until we have better support for them in include-cleaner library itself. So I think we should still call `Includes->mapHeader(H.physical())` before using `toURI`.
Ok SGTM. It seems, though, that we still need the "iffy" FID for the canonical mapping.. Let me know if you know a better way. 




================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:131
+  }
+  void setPreprocessor(Preprocessor &PP) {
+    this->PP = &PP;
----------------
kadircet wrote:
> unfortunately this alternative is called after we've parsed the file, from `clangd/index/FileIndex.cpp`, `indexSymbols`. hence attaching `PI` to `PPCallbacks` won't have any effect.
> 
> This is working unintentionally because we're still populating `CanonicalIncludes` with legacy IWYUPragmaHandler in clangd, and using that to perform private -> public mappings.
> 
> We should instead propagate the `PragmaIncludes` we have in `ParsedAST` and in preamble builds here. You should be able to test the new behaviour `clang-tools-extra/clangd/unittests/FileIndexTests.cpp` to make sure pragma handling through dynamic indexing code paths keep working as expected using an `export` pragma.
Ok thank you. I'm using `PragmaIncludes` from AST and preamble builds for the dynamic index now.

For the background index, I've finally managed to move pragma recording to the `IndexAction` now.

I've also removed the redundant (to include cleaner) comment handlers from the AST build and preamble building logic. 

Also removed the `mapSymbol` method from `CanonicalIncludes`, since it had one usage which should now be covered by the include cleaner, I believe. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152900