[clangd] #include statements support for Open definition

[Nebiroth]

Summary

ctrl-clicking on #include statements now opens the file being pointed by that statement.

[ilya-biryukov]

clangd/ClangdUnit.cpp

148

Please use our descriptive Path typedef.

unittests/clangd/ClangdTests.cpp

764

Some includes may come outside of preamble, I would expect current implementation to fail for this case:

#include <vector> // <-- works here 
// preamble ends here
int main() {

  #include "my_include.h" // <-- does not work here
}

[Nebiroth]

clangd/ClangdUnit.cpp

136

If I were to use InclusionDirective , how would that callback be called automatically? As far as I know, it wouldn't be called automatically for every file that gets indexed the same way AfterExecute would be.

[ilya-biryukov]

clangd/ClangdUnit.cpp

136

It will be called for each #include directive that Preprocessor encountered while building the preamble.
A separate instance of CppFilePreambleCallbacks is created every time we build a preamble for file.

Does that answer your question?

[Nebiroth]

• Now overriding InclusionDirective as a callback to get proper includes information.
• Fixed tests.

[Nebiroth]

• Fixed adding incorrect test file.

This revision requires modifications to clang which can be reviewed on revision D39375.

[malaperle]

I'm not sure it's better to use the InclusionDirective callback for this. We need to get the includes in two places: in the preamble and non-preamble. In the preamble we use the callback, have to store some temporary stuff because we don't have a SourceManager in InclusionDirective, then in finish we use the SourceManager to convert everything. In the non-preamble, we cannot use the callback so we use the SourceManager to go through the includes. Therefore, we have to maintain two different ways of getting the inclusion map. Without using the InclusionDirective callback, we use the SourceManager in both cases and can use the same code to iterate through inclusions, just on two different SourceManagers at different moments. We also don't need to make another patch to modify the PreambleCallbacks. I also double this is much slower but I think this simplifies the code nicely.
What do you think?

[ilya-biryukov]

In D38639#920427, @malaperle wrote:

I'm not sure it's better to use the InclusionDirective callback for this. We need to get the includes in two places: in the preamble and non-preamble. In the preamble we use the callback, have to store some temporary stuff because we don't have a SourceManager in InclusionDirective, then in finish we use the SourceManager to convert everything. In the non-preamble, we cannot use the callback so we use the SourceManager to go through the includes. Therefore, we have to maintain two different ways of getting the inclusion map. Without using the InclusionDirective callback, we use the SourceManager in both cases and can use the same code to iterate through inclusions, just on two different SourceManagers at different moments. We also don't need to make another patch to modify the PreambleCallbacks. I also double this is much slower but I think this simplifies the code nicely.
What do you think?

I think we should never iterate through SourceManager, as it's much easier to get wrong than using the callbacks. I agree that all that fiddling with callbacks is unfortunate, but it's well worth the fact that it'd be much easier to tell that the implementation is correct by simply looking at the implementation and not knowing how SourceManager works. PPCallbacks is a much more direct API that was designed to handle our purposes.

Note that we don't need to use SourceManager to find non-preamble includes, we should implement proper PPCallbacks and use them when building the AST.

[ilya-biryukov]

clangd/ClangdServer.cpp

368 ↗

(On Diff #120485)

This map and two vectors come up quite often in your commit.
Is this a storage for saved includes? Could we define a class that stores this data?

Something like

class IncludeReferenceMap {
  llvm::Optional<PathRef> findIncludeTargetAtLoc(Location Loc);

private:
// maps, vectors, whatever we need go here ....
};

We'd build it using the callbacks, then store it in PreambleData (for PCH include) and ParsedAST (for non-PCH includes). And later query both maps to get the result.

clangd/ClangdUnit.cpp

30

We probably don't need it this include

34

What's the purpose of this class?

145

Please also use PPCallbacks interface instead of the SourceManager (should be possible to hook it up somewhere in ParsedAST::Build).

[malaperle]

In D38639#920487, @ilya-biryukov wrote:

I think we should never iterate through SourceManager, as it's much easier to get wrong than using the callbacks. I agree that all that fiddling with callbacks is unfortunate, but it's well worth the fact that it'd be much easier to tell that the implementation is correct by simply looking at the implementation and not knowing how SourceManager works. PPCallbacks is a much more direct API that was designed to handle our purposes.

Note that we don't need to use SourceManager to find non-preamble includes, we should implement proper PPCallbacks and use them when building the AST.

Sounds good!

[Nebiroth]

clangd/ClangdUnit.cpp

34

We need to be able to use a wrapper class to be able to make a unique_ptr to be sent to PrecompiledPreamble::Build in order to add the list of preprocessor Callbacks.

[Nebiroth]

Using PPCallbacks interface to find non-preamble includes
Created inner class to store vectors in to find locations
Refactored methods to remove some unnecessary parameters
Refactored Unit tests
Merge with most recent master branch + clang

[Nebiroth]

Fixed re-added include

[ilya-biryukov]

clangd/ClangdUnit.cpp

34

Could we implement an instance of PPCallbacks that contains CppFilePreambleCallbacks and forwards to that specific method instead?

The reason is that we're not really delegating other methods in this calls(nor should we, the implementation would be too compilcated).
Having a class that contains CppFilePreambleCallbacks &Collector and calling Collector.InclusionDirective seems perfectly fine, though: its purpose is clear and the implementation is easy.

[Nebiroth]

Creating unique_ptr for wrapper class now uses overriden method createPPCallbacks() from PrecompiledPreamble class
Moved wrapper class to inline inside createPPCallbacks()
Wrapper class now uses CppFilePreambleCallbacks as field

[malaperle]

clangd/ClangdServer.cpp

454 ↗

(On Diff #126429)

IncludeReferenceMap & here? See other comment in takeIRM

clangd/ClangdUnit.cpp

30

remove

109

remove

116

These need to be swapped to be in the same order that they are declared below.

118

I don't think we need this if we pass the map by reference (and store it as a reference, see other comment)

125

Remove (see previous comment)

128

remove

144

I tried to simply the methods introduced here:

void AfterExecute(CompilerInstance &CI) override {
  SourceMgr = &CI.getSourceManager();
  for (auto InclusionLoc : TempPreambleIncludeLocations)
    addIncludeLocation(InclusionLoc);
}

void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                        StringRef FileName, bool IsAngled,
                        CharSourceRange FilenameRange, const FileEntry *File,
                        StringRef SearchPath, StringRef RelativePath,
                        const Module *Imported) override {
  auto SR = FilenameRange.getAsRange();
  if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
    return;

  if (SourceMgr) {
    addIncludeLocation({SR, File->tryGetRealPathName()});
  } else {
    // When we are processing the inclusion directives inside the preamble,
    // we don't have access to a SourceManager, so we cannot convert
    // SourceRange to Range. This is done instead in AfterExecute when a
    // SourceManager becomes available.
    TempPreambleIncludeLocations.push_back({SR, File->tryGetRealPathName()});
  }
}

void addIncludeLocation(std::pair<SourceRange, std::string> InclusionLoc) {
  // Only inclusion directives in the main file make sense. The user cannot
  // select directives not in the main file.
  if (SourceMgr->getMainFileID() == SourceMgr->getFileID(InclusionLoc.first.getBegin()))
    IRM.insert({getRange(InclusionLoc.first), InclusionLoc.second});
}

Range getRange(SourceRange SR) {
  Position Begin;
  Begin.line = SourceMgr->getSpellingLineNumber(SR.getBegin());
  Begin.character = SourceMgr->getSpellingColumnNumber(SR.getBegin());
  Position End;
  End.line = SourceMgr->getSpellingLineNumber(SR.getEnd());
  End.character = SourceMgr->getSpellingColumnNumber(SR.getEnd());
  return {Begin, End};
}

148

IncludeReferenceMap &IRM;

That way, we can use the same map and pass it multiple times to different "collectors".

166

I think we need a "is main file" check here. In case this is used on a AST with no preamble.

267

I don't think this change was brought back intentionally?

277–282

IncludeReferenceMap &IRM

308

unnecessary std::move

317

std::move(IRM)

326–327

I think after rebase, all this code in DeclarationLocationsFinder will need to be moved around, probably inside clangd::findDefinitions?

328

for (auto &IncludeLoc : IncludeLocationMap) {
  Range R = IncludeLoc.first;
  if (isSameLine(R.start.line))
    addLocation(URI::fromFile(IncludeLoc.second), R);
}

335

Probably if an include location was found, we don't want to continue and it should early return.

555

I think you can put the construction of SerializedDeclsCollector back to where it was.

633

You can pass just IRM, so there is no need for the getIRM. We'll have eventually to transfer ownership to ParsedAST so maybe std::move(IRM) here?

clangd/ClangdUnit.h

62

With the map being the only field left (see other comments), there is no need for a class I think. Maybe make a handy type alias for this?

using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;

63

findIncludeTargetAtLoc is never used, remove

67

No need to keep this.

68

No need to keep this.

94

We need to keep this copy of IRM alive for subsequent calls to "open definition" for the same ParsedAST. So it is correct to not do a std::move here. However, I think it should be changed to:

IncludeReferenceMap &getIRM()

and also the caller should assign to a reference.

121

this wrongly came back?

unittests/clangd/ClangdTests.cpp

624–625

why is this check removed?

753

Need to test includes outside preamble.

780

I get an "unchecked assertion" here before the Expected is not checked. I was able to replace it with:

auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
ASSERT_TRUE(!!ExpectedLocations);
std::vector<Location> Locations = ExpectedLocations->Value;

786

I get a test failure here.

../tools/clang/tools/extra/unittests/clangd/ClangdTests.cpp(785): Error:       Expected: check
      Which is: "clangd-test/foo."
To be equal to: FooH
      Which is: { '/' (47, 0x2F), 'c' (99, 0x63), 'l' (108, 0x6C), 'a' (97, 0x61), 'n' (110, 0x6E), 'g' (103, 0x67), 'd' (100, 0x64), '-' (45, 0x2D), 't' (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74), '/' (47, 0x2F), 'f' (102, 0x66), 'o' (111, 0x6F), 'o' (111, 0x6F), '.' (46, 0x2E), 'h' (104, 0x68) }

Not sure what's wrong. There is an additional front slash and missing ".h" at the end?

795

Similar unchecked problem here, this works (although a bit yucky):

ExpectedLocations = Server.findDefinitions(FooCpp, P3);
ASSERT_TRUE(!!ExpectedLocations);
Locations = ExpectedLocations->Value;
EXPECT_TRUE(!Locations.empty());

// Test invalid include
Position P2 = Position{2, 11};

ExpectedLocations = Server.findDefinitions(FooCpp, P2);
ASSERT_TRUE(!!ExpectedLocations);
Locations = ExpectedLocations->Value;
EXPECT_TRUE(Locations.empty());

[Nebiroth]

inner class IncludeReferenceMap replaced by one map
fillRangeVector() and findPreambleIncludes() code moved into wrapper class
Open definition now returns an empty Range struct (line, character = 0)
Fixed unit tests
Minor code improvements

[ilya-biryukov]

clangd/ClangdServer.cpp

446 ↗

(On Diff #127161)

We don't need this Preamble variable, right?

clangd/ClangdUnit.cpp

109

We don't need to derive this class from PPCallbacks, DelegatingPPCallbacks can call any method on it anyway as it knows the exact type. Please remove this inheritance.

115

Instead of passing SourceManager in constructor, maybe add a BeforeExecute(CompilerInstance &CI) method to PreambleCallbacks and set SourceManager when its called.

150

Adding BeforeExecute would allow to get rid of this vector.

201

NIT: replace with return llvm::make_unique<DelegatingPPCallbacks>(*this); and remove the local variable.

277–282

Let's store IncludeReferenceMap in ParsedAST instead of having it as out parameter.
It allows to properly manage the lifetime of IncludeReferenceMap (logically, it is exactly tied to the AST).

305

Why do we use CppFilePreambleCallbacks here? Factor the code that handles the references into a separate class and use it here:

class IncludeRefsCollector : public PPCallbacks {
public:
  IncludeRefsCollector(IncludeReferenceMap &Refs) : Refs(Refs) {}

   // implementation of PPCallbacks ...

private;
  IncludeReferenceMap &Refs;
};

/// .....
class CppFilePreambleCaallbacks {
public:
// ....

  std::unique_ptr<PPCallbacks> createPPCallbacks() { return llvm::make_unique<IncludeRefsCollector>(this->IRM); }
};

// ....
void ParsedAST::Build() {
  // ....
  // No need to create CppFilePreambleCallbacks here.
  Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<IncludeRefsCollector>(IRM));
  //....
}

327

Use IncludeReferenceMap typedef here.

327

Do we change semantics of getting the file paths here? Maybe leave the code as is?

327

NIT: empty linke at the start of the function

329

• use a typedef for the map here
• accept the map by const reference to avoid copies

330

Let's check the the point is in range, not that it's not the same line. So that we don't trigger goto in strange places, like comments:

#include <vector> // this include is very important.  <--- we should not skip to include when editor caret is over a comment

330

If we store IncludeLocationMap in ParsedAST we don't need to change interface of this function.

332

This method does not seem particularly useful. One can construct location like this: Location{Uri, R}, the whole method can be written as DeclarationLocations.push_back(Location{Uri, R}).

clangd/ClangdUnit.h

62

We store a map, but only iterate through it in order. Let's use vector<pair<Range, Path>> instead.

[Nebiroth]

CppFilePreambleCallbacks no longer extends PPCallbacks
CppFilePreambleCallbacks no longer needs SourceManager parameter
Removed temporary vector TempPreambleIncludeLocations
IncludeReferenceMap in ParsedAST no longer passed by reference
Code handling includes outside of preambles is now separated in IncludeRefsCollector class
Removed addLocation
Changed isSameLine function name and now checks if searched location is in range instead of verifying the line
Removed changes to findDefinitions interface

[ilya-biryukov]

Thanks for addressing the comments quickly.
I took another look and added a few more comments.
This moves in the right direction, though, this is really close to landing.

clangd/ClangdServer.cpp

25 ↗

(On Diff #127386)

Is this include redundant now? Can we remove it?

430 ↗

(On Diff #127386)

Capturing this seems redundant. Remove it?

clangd/ClangdUnit.cpp

85

clang-format please

85

Do we need both checks? Doesn't SourceMgr.isInMainFile handles all the cases?

115

Let's create a new empty map inside this class and have a takeIncludeReferences method (similar to TopLevelDeclIDs and takeTopLevelDeclIDs)

150

We should have SourceMgr at all the proper places now, let's store IncludeReferenceMap directly

278

What reference does this IncludeReferenceMap contain? Includes from the preamble?

326–327

This class handles processing AST and preprocessor, it does not need to get IncludeLocationMap in constructor or store it at all.
Remove IncludeLocationMap from this class and handle getting references from IncludeLocationMap outside this class instead.

clangd/ClangdUnit.h

94

Make it all const?

const IncludeReferenceMap &getIRM() const { return IRM; }

278

This function moved without need and lost a comment.

[Nebiroth]

clangd/ClangdUnit.cpp

85

This check was here to prevent an issue that appeared previously. I don't think this check is needed anymore.

150

The idea was to pass a single map around to fill with every reference instead of having separate maps being merged together.

278

This should contain every reference available for one file.

[Nebiroth]

Removed some useless inclusions
Removed superfluous check when inserting data in map
Moved addition to DeclarationLocations in finish() outside of DeclMacrosFinder
Merged with revision 321087 (moved findDefinitions and findDocumentHighlights

[ilya-biryukov]

Another round of review

clangd/ClangdUnit.cpp

115

This comment is not addressed yet, but marked as done.

141

Maybe add assert(SourceMgr && "SourceMgr must be set at this point") here?

150

You copy the map for preamble and then append to it in CppFilePreambleCallbacks? That also LG, we should not have many references there anyway.

278

Thanks for clarifying, LG.

282

Don't we already store the map we need in PreambleData? Why do we need an extra IRM parameter?

clangd/ClangdUnit.h

278

Now we have duplicated definitions here. Bad merge?

clangd/CodeComplete.cpp

328 ↗

(On Diff #127617)

Accidental change?

807 ↗

(On Diff #127617)

Accidental change?

clangd/XRefs.cpp

41

We don't need locations anymore. Remove them.

177

Probably a good place for a comment. Also, maybe rename local var to something easier to understand like IncludeDefinitions

/// Process targets for paths inside #include directive.
std::vector<Location> IncludeTargets;

178

No need to special case empty maps, remove the if altogether.

183

Replace with DeclMacrosFinder->getSearchedLocation() SourceLocationBeg, it makes the code easier to read.

185

why do we need to convert to unsigned? To slience compiler warnings?

188

this should be R.start.charater <= CharNumber && CharNumber <= R.end.character

[ilya-biryukov]

clangd/XRefs.cpp

183

Sorry, the comment got messed up. Here's the correct version:

Replace DeclMacrosFinder->getSearchedLocation() with SourceLocationBeg, it makes the code easier to read.

[Nebiroth]

Minor code cleanup

[malaperle]

@ilya-biryukov Hi! I'll be updating William's patches that were in progress. I just have a few comments/question before I send a new update. (I also don't know if I can update this diff or I have to create a new diff on Phabricator... I guess we'll see!!).

clangd/ClangdUnit.cpp

115

As mentioned below, the idea was to have a single map being appended to, without having to merge two separate maps. However, I can change the code so that two separate maps are built and merged if you prefer.

But I'm not so clear if that's what you'd prefer:

You copy the map for preamble and then append to it in CppFilePreambleCallbacks? That also LG, we should not have many references there anyway.

It's not meant to have any copy. The idea was to create a single IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble and non-preamble include references and std::move it around for later use (stored in ParsedAST).

282

Since the map will be now stored in ParsedAST and the instance doesn't exist yet, we need to keep the IRM parameter. I noticed that it wasn't being std::move'd though.

clangd/XRefs.cpp

185

Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber returns unsigned. I'll extract another var for the line number and cast both to int instead to have less casts and make the condition smaller.

[ilya-biryukov]

@malaperle, hi! Both new diff and updating this works, so feel free the one that suits you best. I tend to look over the whole change on each new round of reviews anyway.

clangd/ClangdUnit.cpp

115

We can't have a single map because AST is rebuilt more often than the Preamble, so we have two options:

• Store a map for the preamble separately, copy it when we need to rebuild the AST and append references from the AST to the new instance of the map.
• Store two maps: one contains references only from the Preamble, the other one from the AST.

I think both are fine, since the copy of the map will be cheap anyway, as we only store a list of includes inside the main file.

282

That looks fine. Also see the other comment on why we need to copy the map from the Preamble, and not std::move it.

clangd/XRefs.cpp

185

Can we maybe convert to clangd::Position using the helper methods first and do the comparison of two clangd::Positions?
Comparing between clangd::Position and clang's line/column numbers is a common source of off-by-one errors in clangd.

[malaperle]

clangd/ClangdUnit.cpp

115

We can't have a single map because AST is rebuilt more often than the Preamble, so we have two options:

Doh! Indeed.

OK so I added the map in PreambleData, this one will say every reparse unless the preamble changes. When the AST is built/rebuilt, the map is copied (or empty) and includes from the AST are appended to it then stored in ParsedAST (option #1?).

clangd/XRefs.cpp

185

offsetToPosition (if that's what you mean) uses a StringRef of the code, which is not handy in this case. I'll add another one "sourceLocToPosition" to convert a SourceLocation to a Position. WDYT? It can be used in a few other places too.

[malaperle]

Store map in PremableData and copy it on reparse.
Convert SourceLoc to Position when comparing with include Positions.

[malaperle]

Revert an unintentional white space change.

[ilya-biryukov]

clangd/ClangdUnit.h

51

We use unordered_map as a vector<pair<>> here. (i.e. we never look up values by key, because we don't only the range, merely a point in the range)
Replace map with vector<pair<>> and remove RangeHash that we don't need anymore.

clangd/XRefs.cpp

185

Looks good, thanks!

unittests/clangd/ClangdTests.cpp

780

We moved findDefinition tests to XRefsTests.cpp and added a useful Annotation helper to make writing these tests simpler.
Maybe we could use it for this test as well?

[malaperle]

Move tests to XRefsTests, change IncludeReferenceMap to a vector and rename it.

[malaperle]

clangd/ClangdUnit.h

51

Done. I also renamed, IncludeReferenceMap to InclusionLocations. I thought I was more suitable.

unittests/clangd/ClangdTests.cpp

780

Done. (I don't know why I can't use the "done" checkbox, maybe because I wasn't the original author?)

[ilya-biryukov]

clangd/ClangdUnit.cpp

97

NIT: replace FilenameRange.getAsRange() with SR

119

NIT: remove ctor, use initializer on the member instead:

private:
  SourceManager *SourceMgr = nullptr;

149

Maybe swap IncLocations and SourceMgr? Grouping TopLevelDecls, TopLevelDeclIDs and IncLocations together seems reasonable, as all of them are essentially output parameters.

286

NIT: move IncLocations closer to their first use, i.e. create the variable right before addPPCallbacks()

clangd/ClangdUnit.h

51

LG

95

NIT: move to .cpp file to be consisten with other decls in this file.

clangd/XRefs.cpp

181

NIT: SourceMgr does not depend on the loop variable, declare it out of the loop.

184

NIT: define operator <= for Position and do this instead: R.start <= Pos && Pos < R.end (note that LSP ranges are half-open, i.e. the end is excluded).
Even better: define a bool contains(Position Pos) const helper in the Range class and use it here (will abstract away to half-open nature of the range)

188

Let's follow the pattern of decls and macros here. I.e. not return from the function, merely collect all the results.

unittests/clangd/XRefsTests.cpp

53 ↗

(On Diff #133978)

NIT: remove this class, use IgnoreDiagnostics from Compiler.h instead.

245 ↗

(On Diff #133978)

Could we also add tests for corner cases: cursor before opening quote, cursor after the closing quote, cursor in the middle of #include token? (we shouldn't navigate anywhere in the middle of the #include token)

[malaperle]

unittests/clangd/XRefsTests.cpp

53 ↗

(On Diff #133978)

It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that wouldn't work.

[malaperle]

unittests/clangd/XRefsTests.cpp

245 ↗

(On Diff #133978)

cursor before opening quote, cursor after the closing quote

I assume we don't want to navigate anywhere for these positions? I don't have an opinion.

(we shouldn't navigate anywhere in the middle of the #include token)

It did in CDT and I thought it worked nicely as it made it easier to click on it. You can hold ctrl on the whole line and it underlined it (well except trailing comments). But clients don't handle the spaces nicely (underline between #include and file name) so I thought I'd work on the client first before making the server do it. Anyhow, for now it shouldn't navigate indeed.

[ilya-biryukov]

unittests/clangd/XRefsTests.cpp

53 ↗

(On Diff #133978)

You're right, I got confused, sorry. Disregard my comment.

245 ↗

(On Diff #133978)

> cursor before opening quote, cursor after the closing quote
I assume we don't want to navigate anywhere for these positions? I don't have an opinion.

I'd say we should navigate when the cursor is right before the opening quote. For the closing quote I don't have any opinions either :-)

[malaperle]

Fix some NITs, add more tests.

[ilya-biryukov]

Last drop of NITs. After they're fixed, this change is ready to land!

unittests/clangd/XRefsTests.cpp

282 ↗

(On Diff #134296)

There's a typo, should be HeaderAnnotations

302 ↗

(On Diff #134296)

NIT: one assertion using EXPECT_THAT will read better and produce more helpful messages in case of failures:

// Create FooHUri to avoid typing URIForFile everywhere.
auto FooHUri = URIForFile{FooH.str()};

EXPECT_THAT(Locations, ElementsAre(Location{FooHUri, HeaderAnnoations.range()}));

328 ↗

(On Diff #134296)

NIT: Use EXPECT_THAT here too:
EXPECT_THAT(Locations, IsEmpty());

[malaperle]

Use EXPECT_THAT in a few places and clean-ups in tests.

[ilya-biryukov]

LGTM modulo the ASSERT_TRUE nit.

unittests/clangd/XRefsTests.cpp

295 ↗

(On Diff #134432)

Maybe replace with ASSERT_TRUE?
If Locations has an error, an assertion will fire in debug mode inside Expected that the error was not handled.

[malaperle]

Change some EXPECT_TRUE to ASSERT_TRUE

clangd/ClangdUnit.h

 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include <atomic>
 #include <future>
 #include <memory>
 #include <mutex>
+#include <unordered_map>
 
 namespace llvm {
 class raw_ostream;
 }
 
 namespace clang {
 class PCHContainerOperations;
 
 namespace vfs {
 class FileSystem;
 }
 
 namespace tooling {
 struct CompileCommand;
 }
 
 namespace clangd {
 
 /// A diagnostic with its FixIts.
 struct DiagWithFixIts {
   clangd::Diagnostic Diag;
   llvm::SmallVector<TextEdit, 1> FixIts;
 };
 
 // Stores Preamble and associated data.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble,
                std::vector<serialization::DeclID> TopLevelDeclIDs,
                std::vector<DiagWithFixIts> Diags);
 
   PrecompiledPreamble Preamble;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
   std::vector<DiagWithFixIts> Diags;
 };
 
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
   /// it is reused during parsing.
   static llvm::Optional<ParsedAST>
   Build(const Context &Ctx, std::unique_ptr<clang::CompilerInvocation> CI,
         std::shared_ptr<const PreambleData> Preamble,
         std::unique_ptr<llvm::MemoryBuffer> Buffer,
         std::shared_ptr<PCHContainerOperations> PCHs,
-        IntrusiveRefCntPtr<vfs::FileSystem> VFS);
+        IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM);
 
   ParsedAST(ParsedAST &&Other);
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();
 
   ASTContext &getASTContext();
   const ASTContext &getASTContext() const;
 
   Preprocessor &getPreprocessor();
   const Preprocessor &getPreprocessor() const;
 
   /// This function returns all top-level decls, including those that come
   /// from Preamble. Decls, coming from Preamble, have to be deserialized, so
   /// this call might be expensive.
   ArrayRef<const Decl *> getTopLevelDecls();
 
   const std::vector<DiagWithFixIts> &getDiagnostics() const;
 
+  const IncludeReferenceMap &getIRM() const { return IRM; };
+
 private:
   ParsedAST(std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action,
             std::vector<const Decl *> TopLevelDecls,
-            std::vector<DiagWithFixIts> Diags);
+            std::vector<DiagWithFixIts> Diags, IncludeReferenceMap IRM);
 
 private:
   void ensurePreambleDeclsDeserialized();
 
   // In-memory preambles must outlive the AST, it is important that this member
   // goes before Clang and Action.
   std::shared_ptr<const PreambleData> Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
   // stored in CompilerInstance and cleaned up by
   // FrontendAction.EndSourceFile).
   std::unique_ptr<CompilerInstance> Clang;
   std::unique_ptr<FrontendAction> Action;
 
   // Data, stored after parsing.
   std::vector<DiagWithFixIts> Diags;
   std::vector<const Decl *> TopLevelDecls;
   bool PreambleDeclsDeserialized;
+  IncludeReferenceMap IRM;
 };
 
 // Provides thread-safe access to ParsedAST.
 class ParsedASTWrapper {
 public:
   ParsedASTWrapper(ParsedASTWrapper &&Wrapper);
   ParsedASTWrapper(llvm::Optional<ParsedAST> AST);
 
   /// Runs \p F on wrapped ParsedAST under lock. Ensures it is not accessed
   /// concurrently.
   template <class Func> void runUnderLock(Func F) const {
     std::lock_guard<std::mutex> Lock(Mutex);
     F(AST ? AST.getPointer() : nullptr);
   }
 
 private:
   // This wrapper is used as an argument to std::shared_future (and it returns a
   // const ref in get()), but we need to have non-const ref in order to
   // implement some features.
   mutable std::mutex Mutex;
   mutable llvm::Optional<ParsedAST> AST;
 };
 
 using ASTParsedCallback =
     std::function<void(const Context &Ctx, PathRef Path, ParsedAST *)>;
 
 /// Manages resources, required by clangd. Allows to rebuild file with new
 /// contents, and provides AST and Preamble for it.
 class CppFile : public std::enable_shared_from_this<CppFile> {
 public:
   // We only allow to create CppFile as shared_ptr, because a future returned by
   // deferRebuild will hold references to it.
private:
   /// Utility class, required by clang.
   std::shared_ptr<PCHContainerOperations> PCHs;
   /// This is called after the file is parsed. This can be nullptr if there is
   /// no callback.
   ASTParsedCallback ASTCallback;
 };
 
 /// Get the beginning SourceLocation at a specified \p Pos.
 SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
                                         const FileEntry *FE);
 
+/// Get definition of symbol at a specified \p Pos.
+std::vector<Location> findDefinitions(const Context &Ctx, ParsedAST &AST,
+                                      Position Pos);
+
+std::vector<DocumentHighlight>
+findDocumentHighlights(const Context &Ctx, ParsedAST &AST, Position Pos);
+
 /// For testing/debugging purposes. Note that this method deserializes all
 /// unserialized Decls, so use with care.
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS);
 
 } // namespace clangd
 } // namespace clang
 #endif

clangd/ClangdUnit.cpp

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Format.h"
-#include <algorithm>
-#include <chrono>
 
 using namespace clang::clangd;
 using namespace clang;
 
 namespace {
 
 class DeclTrackingASTConsumer : public ASTConsumer {
 public:
   DeclTrackingASTConsumer(std::vector<const Decl *> &TopLevelDecls)
       : TopLevelDecls(TopLevelDecls) {}
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
     for (const Decl *D : DG) {
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override {
     return llvm::make_unique<DeclTrackingASTConsumer>(/*ref*/ TopLevelDecls);
   }
 
 private:
   std::vector<const Decl *> TopLevelDecls;
 };
 
+class IncludeRefsCollector : public PPCallbacks {
+public:
+  IncludeRefsCollector(SourceManager &SourceMgr, IncludeReferenceMap &IRM)
+      : SourceMgr(SourceMgr), IRM(IRM) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported) override {
+    auto SR = FilenameRange.getAsRange();
+    if (SR.isInvalid() || !File || File->tryGetRealPathName().empty())
+      return;
+
+    if (SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+      // Only inclusion directives in the main file make sense. The user cannot
+      // select directives not in the main file.
+      IRM.insert(
+          {getRange(FilenameRange.getAsRange()), File->tryGetRealPathName()});
+    }
+  }
+
+  Range getRange(SourceRange SR) {
+    Position Begin;
+    Begin.line = SourceMgr.getSpellingLineNumber(SR.getBegin());
+    Begin.character = SourceMgr.getSpellingColumnNumber(SR.getBegin());
+    Position End;
+    End.line = SourceMgr.getSpellingLineNumber(SR.getEnd());
+    End.character = SourceMgr.getSpellingColumnNumber(SR.getEnd());
+    return {Begin, End};
+  }
+
+private:
+  SourceManager &SourceMgr;
+  IncludeReferenceMap &IRM;
+};
+
 class CppFilePreambleCallbacks : public PreambleCallbacks {
 public:
   std::vector<serialization::DeclID> takeTopLevelDeclIDs() {
     return std::move(TopLevelDeclIDs);
   }
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+      : SourceMgr(nullptr), IRM(IRM) {}
+
   void AfterPCHEmitted(ASTWriter &Writer) override {
     TopLevelDeclIDs.reserve(TopLevelDecls.size());
     for (Decl *D : TopLevelDecls) {
       // Invalid top-level decls may not have been serialized.
       if (D->isInvalidDecl())
         continue;
       TopLevelDeclIDs.push_back(Writer.getDeclID(D));
     }
   }
 
   void HandleTopLevelDecl(DeclGroupRef DG) override {
     for (Decl *D : DG) {
       if (isa<ObjCMethodDecl>(D))
         continue;
       TopLevelDecls.push_back(D);
     }
   }
 
+  void BeforeExecute(CompilerInstance &CI) override {
+    SourceMgr = &CI.getSourceManager();
+  }
+
+  std::unique_ptr<PPCallbacks> createPPCallbacks() {
+    assert(SourceMgr && "SourceMgr must be set at this point");
+    return llvm::make_unique<IncludeRefsCollector>(*SourceMgr, IRM);
+  }
+
 private:
   std::vector<Decl *> TopLevelDecls;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
+  SourceManager *SourceMgr;
+  IncludeReferenceMap &IRM;
+  std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
 };
 
 /// Convert from clang diagnostic level to LSP severity.
 static int getSeverity(DiagnosticsEngine::Level L) {
   switch (L) {
   case DiagnosticsEngine::Remark:
     return 4;
   case DiagnosticsEngine::Note:
     return 3;
   case DiagnosticsEngine::Warning:
     return 2;
   case DiagnosticsEngine::Fatal:
   case DiagnosticsEngine::Error:
     return 1;
   case DiagnosticsEngine::Ignored:
     return 0;
   }
   llvm_unreachable("Unknown diagnostic level!");
 }
 
 // Checks whether a location is within a half-open range.
 // Note that clang also uses closed source ranges, which this can't handle!
 bool locationInRange(SourceLocation L, CharSourceRange R,
                      const SourceManager &M) {
   assert(R.isCharRange());
   if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
       M.getFileID(R.getBegin()) != M.getFileID(L))
     return false;
   return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
 }
 
 // Converts a half-open clang source range to an LSP range.
 // Note that clang also uses closed source ranges, which this can't handle!
 Range toRange(CharSourceRange R, const SourceManager &M) {
   // Clang is 1-based, LSP uses 0-based indexes.
   return {{static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1,
            static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1},
           {static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1,
            static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1}};
 }
 
 // Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
 // LSP needs a single range.
 Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto &M = D.getSourceManager();
   auto Loc = M.getFileLoc(D.getLocation());
   // Accept the first range that contains the location.
   for (const auto &CR : D.getRanges()) {
     auto R = Lexer::makeFileCharRange(CR, M, L);
     if (locationInRange(Loc, R, M))
       return toRange(R, M);
   }
   // The range may be given as a fixit hint instead.
   for (const auto &F : D.getFixItHints()) {
     auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
     if (locationInRange(Loc, R, M))
       return toRange(R, M);
   }
   // If no suitable range is found, just use the token at the location.
if (LangOpts)
         Output.push_back(std::move(*D));
   }
 
 private:
   std::vector<DiagWithFixIts> &Output;
   llvm::Optional<LangOptions> LangOpts;
 };
 
 template <class T> bool futureIsReady(std::shared_future<T> const &Future) {
   return Future.wait_for(std::chrono::seconds(0)) == std::future_status::ready;
 }
 
 } // namespace
 
 void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
-llvm::Optional<ParsedAST>
-ParsedAST::Build(const Context &Ctx,
-                 std::unique_ptr<clang::CompilerInvocation> CI,
-                 std::shared_ptr<const PreambleData> Preamble,
-                 std::unique_ptr<llvm::MemoryBuffer> Buffer,
-                 std::shared_ptr<PCHContainerOperations> PCHs,
-                 IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
+llvm::Optional<ParsedAST> ParsedAST::Build(
+    const Context &Ctx, std::unique_ptr<clang::CompilerInvocation> CI,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM) {
 
   std::vector<DiagWithFixIts> ASTDiags;
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
 
   const PrecompiledPreamble *PreamblePCH =
       Preamble ? &Preamble->Preamble : nullptr;
   auto Clang = prepareCompilerInstance(
       std::move(CI), PreamblePCH, std::move(Buffer), std::move(PCHs),
       std::move(VFS), /*ref*/ UnitDiagsConsumer);
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<CompilerInstance> CICleanup(
       Clang.get());
 
   auto Action = llvm::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log(Ctx, "BeginSourceFile() failed when building AST for " +
                  MainInput.getFile());
     return llvm::None;
   }
+
+  Clang->getPreprocessor().addPPCallbacks(
+      llvm::make_unique<IncludeRefsCollector>(Clang->getSourceManager(), IRM));
+
   if (!Action->Execute())
     log(Ctx, "Execute() failed when building AST for " + MainInput.getFile());
-
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
 
   std::vector<const Decl *> ParsedDecls = Action->takeTopLevelDecls();
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
-                   std::move(ParsedDecls), std::move(ASTDiags));
+                   std::move(ParsedDecls), std::move(ASTDiags), std::move(IRM));
 }
 
 namespace {
 
 SourceLocation getMacroArgExpandedLocation(const SourceManager &Mgr,
                                            const FileEntry *FE, Position Pos) {
   SourceLocation InputLoc =
       Mgr.translateFileLineCol(FE, Pos.line + 1, Pos.character + 1);
   return Mgr.getMacroArgExpandedLocation(InputLoc);
 }
-
-
 } // namespace
 
 void ParsedAST::ensurePreambleDeclsDeserialized() {
   if (PreambleDeclsDeserialized || !Preamble)
     return;
 
   std::vector<const Decl *> Resolved;
   Resolved.reserve(Preamble->TopLevelDeclIDs.size());
 
   ExternalASTSource &Source = *getASTContext().getExternalSource();
   for (serialization::DeclID TopLevelDecl : Preamble->TopLevelDeclIDs) {
     // Resolve the declaration ID to an actual declaration, possibly
     // deserializing the declaration in the process.
     if (Decl *D = Source.GetExternalDecl(TopLevelDecl))
       Resolved.push_back(D);
   }
 
PreambleData::PreambleData(PrecompiledPreamble Preamble,
                            std::vector<DiagWithFixIts> Diags)
     : Preamble(std::move(Preamble)),
       TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)) {}
 
 ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
                      std::vector<const Decl *> TopLevelDecls,
-                     std::vector<DiagWithFixIts> Diags)
+                     std::vector<DiagWithFixIts> Diags, IncludeReferenceMap IRM)
     : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Diags(std::move(Diags)),
-      TopLevelDecls(std::move(TopLevelDecls)),
-      PreambleDeclsDeserialized(false) {
+      TopLevelDecls(std::move(TopLevelDecls)), PreambleDeclsDeserialized(false),
+      IRM(std::move(IRM)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
 ParsedASTWrapper::ParsedASTWrapper(ParsedASTWrapper &&Wrapper)
     : AST(std::move(Wrapper.AST)) {}
 
 ParsedASTWrapper::ParsedASTWrapper(llvm::Optional<ParsedAST> AST)
std::unique_ptr<CompilerInvocation> CI;
                                               &IgnoreDiagnostics, false);
       CI =
           createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine, VFS);
       // createInvocationFromCommandLine sets DisableFree.
       CI->getFrontendOpts().DisableFree = false;
     }
     assert(CI && "Couldn't create CompilerInvocation");
 
+    IncludeReferenceMap IRM;
     std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
         llvm::MemoryBuffer::getMemBufferCopy(NewContents, That->FileName);
 
     // A helper function to rebuild the preamble or reuse the existing one. Does
     // not mutate any fields of CppFile, only does the actual computation.
     // Lamdba is marked mutable to call reset() on OldPreamble.
     auto DoRebuildPreamble =
         [&]() mutable -> std::shared_ptr<const PreambleData> {
       auto Bounds =
auto DoRebuildPreamble =
 
       trace::Span Tracer(Ctx, "Preamble");
       SPAN_ATTACH(Tracer, "File", That->FileName);
       std::vector<DiagWithFixIts> PreambleDiags;
       StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags);
       IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
           CompilerInstance::createDiagnostics(
               &CI->getDiagnosticOpts(), &PreambleDiagnosticsConsumer, false);
-      CppFilePreambleCallbacks SerializedDeclsCollector;
+
+      CppFilePreambleCallbacks SerializedDeclsCollector(IRM);
+
       auto BuiltPreamble = PrecompiledPreamble::Build(
           *CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine, VFS, PCHs,
           /*StoreInMemory=*/That->StorePreamblesInMemory,
           SerializedDeclsCollector);
 
       if (BuiltPreamble) {
         log(Ctx, "Built preamble of size " + Twine(BuiltPreamble->getSize()) +
                      " for file " + Twine(That->FileName));
auto FinishRebuild =
     }
 
     // Compute updated AST.
     llvm::Optional<ParsedAST> NewAST;
     {
       trace::Span Tracer(Ctx, "Build");
       SPAN_ATTACH(Tracer, "File", That->FileName);
       NewAST = ParsedAST::Build(Ctx, std::move(CI), std::move(NewPreamble),
-                                std::move(ContentsBuffer), PCHs, VFS);
+                                std::move(ContentsBuffer), PCHs, VFS, IRM);
     }
-
     if (NewAST) {
       Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(),
                          NewAST->getDiagnostics().end());
       if (That->ASTCallback)
         That->ASTCallback(Ctx, That->FileName, NewAST.getPointer());
     } else {
       // Don't report even Preamble diagnostics if we coulnd't build AST.
       Diagnostics.clear();

clangd/Protocol.h

friend bool operator==(const Range &LHS, const Range &RHS) {
     return std::tie(LHS.start, LHS.end) == std::tie(RHS.start, RHS.end);
   }
   friend bool operator<(const Range &LHS, const Range &RHS) {
     return std::tie(LHS.start, LHS.end) < std::tie(RHS.start, RHS.end);
   }
 };
 bool fromJSON(const json::Expr &, Range &);
 json::Expr toJSON(const Range &);
+
+class RangeHash {
+public:
+  std::size_t operator()(const Range &R) const {
+    return ((R.start.line & 0x18) << 3) | ((R.start.character & 0x18) << 1) |
+           ((R.end.line & 0x18) >> 1) | ((R.end.character & 0x18) >> 3);
+  }
+};
+bool fromJSON(const json::Expr &, Range &);
+json::Expr toJSON(const Range &);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Range &);
 
 struct Location {
   /// The text document's URI.
   URI uri;
   Range range;
 
   friend bool operator==(const Location &LHS, const Location &RHS) {

clangd/XRefs.cpp

 namespace clangd {
 using namespace llvm;
 namespace {
 
 /// Finds declarations locations that a given source location refers to.
 class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
   std::vector<const Decl *> Decls;
   std::vector<const MacroInfo *> MacroInfos;
+  std::vector<Location> DeclarationLocations;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
   Preprocessor &PP;
 
 public:
   DeclarationAndMacrosFinder(raw_ostream &OS,
                              const SourceLocation &SearchedLocation,
                              ASTContext &AST, Preprocessor &PP)
       : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
   std::vector<const Decl *> takeDecls() {
     // Don't keep the same declaration multiple times.
     // This can happen when nodes in the AST are visited twice.
     std::sort(Decls.begin(), Decls.end());
     auto Last = std::unique(Decls.begin(), Decls.end());
     Decls.erase(Last, Decls.end());
     return std::move(Decls);
   }
 
+  std::vector<Location> takeLocations() {
+    // Don't keep the same location multiple times.
+    // This can happen when nodes in the AST are visited twice.
+    std::sort(DeclarationLocations.begin(), DeclarationLocations.end());
+    auto Last =
+        std::unique(DeclarationLocations.begin(), DeclarationLocations.end());
+    DeclarationLocations.erase(Last, DeclarationLocations.end());
+    return std::move(DeclarationLocations);
+  }
+
   std::vector<const MacroInfo *> takeMacroInfos() {
     // Don't keep the same Macro info multiple times.
     std::sort(MacroInfos.begin(), MacroInfos.end());
     auto Last = std::unique(MacroInfos.begin(), MacroInfos.end());
     MacroInfos.erase(Last, MacroInfos.end());
     return std::move(MacroInfos);
   }
 
+  const SourceLocation &getSearchedLocation() { return SearchedLocation; };
+
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations, FileID FID,
                       unsigned Offset,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     if (isSearchedLocation(FID, Offset))
       Decls.push_back(D);
     return true;
   }
 
 private:
   bool isSearchedLocation(FileID FID, unsigned Offset) const {
     const SourceManager &SourceMgr = AST.getSourceManager();
     return SourceMgr.getFileOffset(SearchedLocation) == Offset &&
            SourceMgr.getFileID(SearchedLocation) == FID;
   }
 
+  void addDeclarationLocation(const SourceRange &ValSourceRange) {
+    const SourceManager &SourceMgr = AST.getSourceManager();
+    const LangOptions &LangOpts = AST.getLangOpts();
+    SourceLocation LocStart = ValSourceRange.getBegin();
+    SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(),
+                                                       0, SourceMgr, LangOpts);
+    Position Begin;
+    Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
+    Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+    Position End;
+    End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
+    End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+    Range R = {Begin, End};
+    DeclarationLocations.push_back(
+        Location{URI::fromFile(
+                     SourceMgr.getFilename(SourceMgr.getSpellingLoc(LocStart))),
+                 R});
+  }
+
   void finish() override {
     // Also handle possible macro at the searched location.
     Token Result;
     auto &Mgr = AST.getSourceManager();
     if (!Lexer::getRawToken(SearchedLocation, Result, Mgr, AST.getLangOpts(),
                             false)) {
       if (Result.is(tok::raw_identifier)) {
         PP.LookUpIdentifierInfo(Result);
std::vector<Location> findDefinitions(const Context &Ctx, ParsedAST &AST,
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
 
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
                      DeclMacrosFinder, IndexOpts);
 
+  /// Process targets for paths inside #include directive.
+  std::vector<Location> IncludeTargets;
+  for (auto &IncludeLoc : AST.getIRM()) {
+    Range R = IncludeLoc.first;
+    const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+    unsigned CharNumber = SourceMgr.getSpellingColumnNumber(SourceLocationBeg);
+
+    if ((unsigned)R.start.line ==
+            SourceMgr.getSpellingLineNumber(SourceLocationBeg) &&
+        ((unsigned)R.start.character <= CharNumber &&
+         CharNumber <= (unsigned)R.end.character)) {
+      IncludeTargets.push_back(Location{URI::fromFile(IncludeLoc.second),
+                                        Range{Position{0, 0}, Position{0, 0}}});
+      return IncludeTargets;
+    }
+  }
+
   std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
   std::vector<const MacroInfo *> MacroInfos =
       DeclMacrosFinder->takeMacroInfos();
   std::vector<Location> Result;
 
   for (auto Item : Decls) {
     auto L = getDeclarationLocation(AST, Item->getSourceRange());
     if (L)
       Result.push_back(*L);
   }
 
   for (auto Item : MacroInfos) {
     SourceRange SR(Item->getDefinitionLoc(), Item->getDefinitionEndLoc());
     auto L = getDeclarationLocation(AST, SR);
     if (L)
       Result.push_back(*L);
   }
 
+  if (Result.empty())
+    return DeclMacrosFinder->takeLocations();
+  else
     return Result;
 }
 
 namespace {
 
 /// Finds document highlights that a given list of declarations refers to.
 class DocumentHighlightsFinder : public index::IndexDataConsumer {
   std::vector<const Decl *> &Decls;
   std::vector<DocumentHighlight> DocumentHighlights;

unittests/clangd/ClangdTests.cpp

 //===-- ClangdTests.cpp - Clangd unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Context.h"
 #include "TestFS.h"
 #include "clang/Config/config.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
TestDiagConsumer DiagConsumer;
 
     auto FindDefinitionsRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);
 
       Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
       ASSERT_TRUE(!!Server.findDefinitions(Context::empty(),
                                            FilePaths[FileIndex], Pos));
     };
 
     std::vector<std::function<void()>> AsyncRequests = {
         AddDocumentRequest, ForceReparseRequest, RemoveDocumentRequest};
     std::vector<std::function<void()>> BlockingRequests = {
         CodeCompletionRequest, FindDefinitionsRequest};
 
     // Bash requests to ClangdServer in a loop.
if (!ReqStats[I].FileIsRemoved) {
                 ReqStats[I].LastContentsHadErrors);
     }
 
     ASSERT_LE(Stats[I].HitsWithErrors, ReqStats[I].RequestsWithErrors);
     ASSERT_LE(Stats[I].HitsWithoutErrors, ReqStats[I].RequestsWithoutErrors);
   }
 }
 
 TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
 
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true);
 
   auto SourceContents = R"cpp(
TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
   ASSERT_EQ(PathResult.getValue(), Foo3HXX);
 
   // Test if asking for a corresponding file that doesn't exist returns an empty
   // string.
   PathResult = Server.switchSourceHeader(Invalid);
   EXPECT_FALSE(PathResult.hasValue());
 }
 
+TEST_F(ClangdVFSTest, CheckDefinitionIncludes) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
+      /*StorePreamblesInMemory=*/true,
+      EmptyLogger::getInstance());
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents = R"cpp(
+  #include "foo.h"
+  #include "invalid.h"
+  int b = a;
+  // test
+  int foo;
+  #include "foo.h"
+  )cpp";
+  FS.Files[FooCpp] = SourceContents;
+  auto FooH = getVirtualTestFilePath("foo.h");
+  const auto HeaderContents = "int a;";
+
+  FS.Files[FooCpp] = SourceContents;
+  FS.Files[FooH] = HeaderContents;
+
+  Server.addDocument(FooH, HeaderContents);
+  Server.addDocument(FooCpp, SourceContents);
+
+  Position P = Position{1, 11};
+
+  auto ExpectedLocations = Server.findDefinitions(FooCpp, P);
+  ASSERT_TRUE(!!ExpectedLocations);
+  std::vector<Location> Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+  std::string s("file:///");
+  std::string check = Locations[0].uri.uri;
+  check = check.erase(0, s.size() - 1);
+  check = check.substr(0, check.size());
+  ASSERT_EQ(check, FooH);
+  ASSERT_EQ(Locations[0].range.start.line, 0);
+  ASSERT_EQ(Locations[0].range.start.character, 0);
+  ASSERT_EQ(Locations[0].range.end.line, 0);
+  ASSERT_EQ(Locations[0].range.end.character, 0);
+
+  // Test ctrl-clicking on the #include part on the statement
+  Position P2 = Position{1, 3};
+
+  ExpectedLocations = Server.findDefinitions(FooCpp, P2);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+
+  // Test invalid include
+  Position P3 = Position{2, 11};
+
+  ExpectedLocations = Server.findDefinitions(FooCpp, P3);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  // Test include outside of Preamble
+  Position P4  = Position{6, 5};
+
+  ExpectedLocations = Server.findDefinitions(FooCpp, P4);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+}
+
 TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
     NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
         : StartSecondReparse(std::move(StartSecondReparse)) {}
 
     void onDiagnosticsReady(
         PathRef File,