[clang-tools-extra] [clang][include-cleaner]skip stdlib recogntion only when there are defintion with body in main file. (PR #95797)

[kadir çetinkaya via cfe-commits]

================
@@ -39,20 +40,24 @@ Hints declHints(const Decl *D) {
 }
 
 std::vector<Hinted<SymbolLocation>> locateDecl(const Decl &D) {
-  std::vector<Hinted<SymbolLocation>> Result;
-  // FIXME: Should we also provide physical locations?
-  if (auto SS = tooling::stdlib::Recognizer()(&D)) {
-    Result.push_back({*SS, Hints::CompleteSymbol});
-    if (!D.hasBody())
-      return Result;
-  }
+  SourceManager &SM = D.getASTContext().getSourceManager();
+  bool HasBodyInMainFile = llvm::any_of(D.redecls(), [&](Decl *Redecl) {
+    return Redecl->hasBody() && SM.isInMainFile(Redecl->getLocation());
+  });
+  // if decl has body and in main file, we don't need to do further search.
+  if (!HasBodyInMainFile)
----------------
kadircet wrote:

i don't think it's actually "right" layering-wise to perform any `sourcelocation <-> fileid` mappings here. it's subtle and expensive, especially for every-redecl of stdlib symbols, which might be forward declared many times in STL (but also for any other symbol).

Hence we actually have logic down the line that maps sourcelocations to files (it's also expensive today, but is in a single place and takes care of edge cases, like macros). at least this layering ensures that if we want to introduce caching/optimizations one day, we can rely on having certain separation between different kinds of mappings.

Moreover, I don't follow the check around a redeclaration having body only in the main file. I feel like you're focusing on a very specific issue you have and not solving the issue in general. e.g. what if the user only has forward declaration of the symbol in the main file? why don't we want to suppress in that case? or what if the user has body/declaration in a different header file, which also seems pretty common.

I think if you want to improve this case, you really need to address `// FIXME: Should we also provide physical locations?`, which is definitely doable, we just need to make sure ranking is still done accordingly.

I am still strongly arguing that we should never insert a custom header if there's a name-collision with a stdlib symbol. Hence we should make sure all physical locations for a stdlib symbol is downranked, i'd recommend stripping completesymbol signal from these symbols.

You also need to consider the macro-side of this. you're addressing this situation for decls, but the user might as well have `#define assert(X) X` in their main file (or some of the other headers). we need to make sure this same logic also works for that.

https://github.com/llvm/llvm-project/pull/95797