[PATCH] D105426: [clangd] IWYU as a library: Find all references to symbols in the file

Fri Aug 6 08:47:34 PDT 2021

sammccall added a comment.

Thanks, this scope looks good for a first patch!

================
Comment at: clang-tools-extra/clangd/IWYU.cpp:18
+
+/// Main "driver" of the IWYU symbol usage discovery mechanism. Crawler
+/// traverses the AST and feeds in the locations of (sometimes implicitly) used
----------------
The first sentence doesn't mean much to me, I think we could just drop it?

================
Comment at: clang-tools-extra/clangd/IWYU.cpp:63
+  // This is helpful in getFoo().bar(), where Foo must be complete.
+  // FIXME(kirillbobyrev): This is a tricky case that raises a question - should
+  // we consider implicit types to be "used"? With auto's it's not so clear.
----------------
Is this just "FIXME: should this behavior be configurable or tweaked?"

I think it's clear enough what "this behavior" is in context.
Whereas the rest of the comment adds some confusion (e.g. what's an "implicit type", what usage of auto are you talking about etc. And I think it's *way* too soon to say that it *should* be configurable - remember the application is unused-include, turning this off only grants the ability for headers to be marked unused even if they define the type of a subexpression.

I think it's more likely we'd tweak the heuristic in the future, e.g. only do this if it's a type that can be incomplete.

================
Comment at: clang-tools-extra/clangd/IWYU.cpp:73
+    if (shouldAdd(T.getTypePtrOrNull())) { // don't care about quals
+      RecursiveASTVisitor<ReferencedLocationCrawler>::TraverseType(T);
+    }
----------------
i usually add a private `using Base = ...` and refer to `Base::TraverseType(T)` - intent is clearer and often we end up with several of these

================
Comment at: clang-tools-extra/clangd/IWYU.cpp:95
+
+  bool shouldAdd(const void *P) { return P && Visited.insert(P).second; }
+
----------------
this name is a bit vague (why wouldn't we add it?), maybe isNew?

================
Comment at: clang-tools-extra/clangd/IWYU.h:10
+/// \file
+/// This defines functionality modeled after Include-What-You-Use
+/// (https://include-what-you-use.org/). The variant we implement is not
----------------
Related to the naming discussion, this should first say what the functionality is (warnings about header hygiene: misuse of transitive headers and unused includes).
Then definitely credit the IWYU tool and mention the differences.

================
Comment at: clang-tools-extra/clangd/IWYU.h:17
+/// FIXME(kirillbobyrev): Add support for IWYU pragmas.
+/// FIXME(kirillbobyrev): Add support for mapping files.
+/// FIXME(kirillbobyrev): Add support for standard library headers.
----------------
I'm not sure we want to do this (or plan to do this) unless we have some idea of how discovery would work.

================
Comment at: clang-tools-extra/clangd/IWYU.h:34
+using ReferencedLocations = llvm::DenseSet<SourceLocation>;
+/// Goes through main file AST and computes all the locations used symbols are
+/// coming from. These locations are later used to determine which headers
----------------
Maybe just "Finds locations of all symbols used in the main file"?

If you want to talk about traversal/impl strategy, that's OK but not in the first paragraph.

================
Comment at: clang-tools-extra/clangd/IWYU.h:35
+/// Goes through main file AST and computes all the locations used symbols are
+/// coming from. These locations are later used to determine which headers
+/// should be marked as "used" and "directly used".
----------------
I'd be explicit that the returned locations may be macro expansions, and are not resolved to their spelling/expansion location.

================
Comment at: clang-tools-extra/clangd/IWYU.h:38
+///
+/// Because of the clangd limitations, this should be efficient,
+/// hence there is only one AST traversal for the whole process.
----------------
I'm not quite sure what this is saying/who the audience for the comment is.
Maybe it's interesting to say something like

We use this to compute unused headers, so:
 - we cover the whole file in a single traversal for efficiency
 - we don't attempt to describe where symbols were referenced from
 - in ambiguous cases (e.g. implicitly used symbols, multiple declarations) we err on the side of reporting all possible locations

================
Comment at: clang-tools-extra/clangd/IWYU.h:41
+///
+/// NOTE: The balance is currently shifted towards considering some of the
+/// implicitly used symbols as "used" for the sake of providing less noisy
----------------
It's a bit more general than this (e.g. multiple declaration). See previous comment for a wording suggestion

================
Comment at: clang-tools-extra/clangd/IWYU.h:43
+/// implicitly used symbols as "used" for the sake of providing less noisy
+/// "unused header" diagnostics. However, this should be re-evalated once we
+/// also want to surface "missing header" diagnostics.
----------------
I actually don't think we'd use the same entrypoint for missing include, but rather duplicate it (and *maybe* share some impl details whose docs don't belong here).

The reason is we get to "throw away" different info in each case to make the data structures as tight as possible:
 - for unused include, we can throw away the reference location and just track sets of locations and then files. This means we traverse first, build a bundle of locations, and then process them together.
 - for missing include, we need the reference location, but don't need to associate them with specific #includes. So the tight data structure here is a set of allowed FileIDs plus a cache for macro FileIDs. Then we'll traverse last and query one symbol at a time, with early exit. 

Comments about future plans can sometimes be useful I think, but this one seems pretty unlikely to pan out.

================
Comment at: clang-tools-extra/clangd/IWYU.h:47
+
+/// Computes all referenced files given a set of SourceLocations that were
+/// extracted from symbol declarations.
----------------
It's not obvious what this actually means.
Would be good to mention expanding macros

================
Comment at: clang-tools-extra/clangd/unittests/IWYUTests.cpp:28
+      {
+          "int ^x();",
+          "int y = x();",
----------------
I think i'd be useful to have a comment in these for the node type or special case that's being tested

First few are

DeclRefExpr
RecordType
TypedefType
MemberExpr
Expr (type is traversed)
Redecls
CXXConstructExpr

================
Comment at: clang-tools-extra/clangd/unittests/IWYUTests.cpp:84
+      {
+          // FIXME(kirillbobyrev): Should B also be marked as used?
+          "struct ^A {}; using B = A; using ^C = B;",
----------------
I don't think so, IMO current behavior is fine.
B is not named, nor can its absence cause the type to be incomplete.

================
Comment at: clang-tools-extra/clangd/unittests/IWYUTests.cpp:97
+      {
+          // FIXME(kirillbobyrev): Should report the UsingDecl.
+          // This information is not preserved in the AST.
----------------
A bit more explicit about the bug?

When a type is resolved via a using declaration, the UsingShadowDecl is not referenced in the AST.
Compare to TypedefType, or DeclRefExpr::getFoundDecl().

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105426