[include-cleaner] Filter references to identity macros

[kadircet]

Summary

Despite being true positives, these results just confuse users. So
filter them out.

[sammccall]

clang-tools-extra/include-cleaner/lib/Analysis.cpp

37

shouldIgnoreMacroReference would better distinguish vs macro definition

43

Comment nit: I'm having trouble imagining cases that are actually*unrelated* to the stdlib.

If stderr is an impl-defined macro, then the only way to use the name to refer to something else is if it's not defined (inside ifndef stderr, or if you can be sure your TU doesn't include stdio first). Seems implausible...

That said I feel like we've had this conversation before and I've just forgotten the details.

44

Nit: "false positives" is a little unclear: positives for this function are negatives for walkAST and could be either for diagnostics.

Also, I think you mean *references* rather than declarations?

[kadircet]

• Rename helper, update comments.

[kadircet]

clang-tools-extra/include-cleaner/lib/Analysis.cpp

43

If stderr is an impl-defined macro, then the only way to use the name to refer to something else is if it's not defined

That's absolutely right. The issue here is macro expansion triggers independent of the context, e.g.

#include <stdio.h>
namespace ns { void stderr(); }
void foo() { ns::stderr(); }

here we have a (well two) reference to stderr macro from stdandard library, which is not user's intent, but rather a quirk of the language.

44

this should've been "false negatives" from the "usedness perspective", we basically drop the reference to ns::stderr in above example, because the name is not spelled in the main file (but rather spelled inside a macro body).

[sammccall]

still LG, comments are still confusing me a little

clang-tools-extra/include-cleaner/lib/Analysis.cpp

43

Sure, but this code is not valid C++ (since stderr is not guaranteed to expand to a single identifier). Is this actually a common/motivating case?

44

And now it's backwards if your perspective is this function itself!

just avoid "false positives"/"false negatives" terminology and describe the actual effect?

[kadircet]

clang-tools-extra/include-cleaner/lib/Analysis.cpp

43

Sure, but this code is not valid C++ (since stderr is not guaranteed to expand to a single identifier).

That's true. Most of the standard library implementations I checked does that, but I also don't like that reliance, so making it a little bit more generic (which meant some more plumbing, sorry for the big diff :( ), by making sure that we apply this to all the macros that expand to themselves.

[kadircet]

• Apply filtering only to macros that expand to themselves

[sammccall]

clang-tools-extra/include-cleaner/lib/Analysis.cpp

42

still call out stdout/stderr?

clang-tools-extra/include-cleaner/lib/Analysis.cpp

 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include <cassert>
 #include <string>
 
 namespace clang::include_cleaner {
 
+namespace {
+bool shouldIgnoreMacroReference(const Macro &M) {
+  static const auto *MacroNamesToIgnore = new llvm::StringSet<>{
+      // C standard says these are implementation defined macros, hence most of
+      // the standard library providers implement it by defining these as macros
+      // that resolve to themselves.
+      // This results in surprising behavior from users point of view (we
+      // generate a usage of stdio.h, in places unrelated to standard library).
+      // FIXME: Also eliminate the false negatives by treating declarations
+      // resulting from these expansions as used.
+      "stdin",
+      "stdout",
+      "stderr",
+  };
+  return MacroNamesToIgnore->contains(M.Name->getName());
+}
+} // namespace
+
 void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
               llvm::ArrayRef<SymbolReference> MacroRefs,
               const PragmaIncludes *PI, const SourceManager &SM,
               UsedSymbolCB CB) {
   // This is duplicated in writeHTMLReport, changes should be mirrored there.
   tooling::stdlib::Recognizer Recognizer;
   for (auto *Root : ASTRoots) {
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
       auto FID = SM.getFileID(SM.getSpellingLoc(Loc));
       if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID())
         return;
       // FIXME: Most of the work done here is repetitive. It might be useful to
       // have a cache/batching.
       SymbolReference SymRef{ND, Loc, RT};
       return CB(SymRef, headersForSymbol(ND, SM, PI));
     });
   }
   for (const SymbolReference &MacroRef : MacroRefs) {
     assert(MacroRef.Target.kind() == Symbol::Macro);
-    if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)))
+    if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) ||
+        shouldIgnoreMacroReference(MacroRef.Target.macro()))
       continue;
     CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI));
   }
 }
 
 AnalysisResults
 analyze(llvm::ArrayRef<Decl *> ASTRoots,
         llvm::ArrayRef<SymbolReference> MacroRefs, const Includes &Inc,

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

EXPECT_THAT(
             Contains(Pair(Code.point("exp"), UnorderedElementsAre(Fwd, Def))),
             Contains(Pair(Code.point("full"), UnorderedElementsAre(Fwd, Def))),
             Contains(
                 Pair(Code.point("implicit"), UnorderedElementsAre(Fwd, Def))),
             Contains(
                 Pair(Code.point("partial"), UnorderedElementsAre(Partial)))));
 }
 
+TEST_F(WalkUsedTest, IgnoresIdentityMacros) {
+  llvm::Annotations Code(R"cpp(
+  #include "header.h"
+  void $bar^bar() {
+    $stdin^stdin();
+  }
+  )cpp");
+  Inputs.Code = Code.code();
+  Inputs.ExtraFiles["header.h"] = guard(R"cpp(
+  #include "inner.h"
+  void stdin();
+  )cpp");
+  Inputs.ExtraFiles["inner.h"] = guard(R"cpp(
+  #define stdin stdin
+  )cpp");
+
+  TestAST AST(Inputs);
+  auto &SM = AST.sourceManager();
+  auto MainFile = Header(SM.getFileEntryForID(SM.getMainFileID()));
+  EXPECT_THAT(offsetToProviders(AST, SM),
+              UnorderedElementsAre(
+                  // FIXME: we should have a reference from stdin to header.h
+                  Pair(Code.point("bar"), UnorderedElementsAre(MainFile))));
+}
 } // namespace
 } // namespace clang::include_cleaner