[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

151

Please use our descriptive Path typedef.

unittests/clangd/ClangdTests.cpp

765

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

139

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

139

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

32

We probably don't need it this include

36

What's the purpose of this class?

147

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

36

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

36

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

32

remove

111

remove

118

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

120

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

127

Remove (see previous comment)

130

remove

147

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};
}

151

IncludeReferenceMap &IRM;

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

169

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

259

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

275

IncludeReferenceMap &IRM

306

unnecessary std::move

316

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.

562

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

648

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

65

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>;

66

findIncludeTargetAtLoc is never used, remove

70

No need to keep this.

71

No need to keep this.

96

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.

123

this wrongly came back?

unittests/clangd/ClangdTests.cpp

625–626

why is this check removed?

754

Need to test includes outside preamble.

781

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;

787

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?

796

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

111

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.

117

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

153

Adding BeforeExecute would allow to get rid of this vector.

204

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

275

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).

303

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

331

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

65

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

87

clang-format please

87

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

117

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

153

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

276

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

96

Make it all const?

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

280

This function moved without need and lost a comment.

[Nebiroth]

clangd/ClangdUnit.cpp

87

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

153

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

276

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

117

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

144

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

153

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

276

Thanks for clarifying, LG.

280

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

clangd/ClangdUnit.h

280

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.

142

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;

143

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

148

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

150

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

153

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

[ilya-biryukov]

clangd/XRefs.cpp

148

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

117

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).

280

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

150

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

117

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.

280

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

150

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

117

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

150

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

150

Looks good, thanks!

unittests/clangd/ClangdTests.cpp

781

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

781

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

99

NIT: replace FilenameRange.getAsRange() with SR

121

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

private:
  SourceManager *SourceMgr = nullptr;

152

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

279

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

clangd/ClangdUnit.h

51

LG

97

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

clangd/XRefs.cpp

146

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

149

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)

153

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;
 };
 
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
 // Stores Preamble and associated data.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble,
                std::vector<serialization::DeclID> TopLevelDeclIDs,
-               std::vector<DiagWithFixIts> Diags);
+               std::vector<DiagWithFixIts> Diags, IncludeReferenceMap IRM);
 
   PrecompiledPreamble Preamble;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
   std::vector<DiagWithFixIts> Diags;
+  IncludeReferenceMap IRM;
 };
 
 /// 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);
 
   ParsedAST(ParsedAST &&Other);
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();
 
   ASTContext &getASTContext();
   const ASTContext &getASTContext() const;
 
   Preprocessor &getPreprocessor();
   std::shared_ptr<Preprocessor> getPreprocessorPtr();
   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);
 
 /// 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/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;
 };
 
+// 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}};
+}
+
+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(
+          {toRange(FilenameRange, SourceMgr), File->tryGetRealPathName()});
+    }
+  }
+
+private:
+  SourceManager &SourceMgr;
+  IncludeReferenceMap &IRM;
+};
+
 class CppFilePreambleCallbacks : public PreambleCallbacks {
 public:
   std::vector<serialization::DeclID> takeTopLevelDeclIDs() {
     return std::move(TopLevelDeclIDs);
   }
 
+  IncludeReferenceMap takeIncludeReferenceMap() { return std::move(IRM); }
+
+  CppFilePreambleCallbacks() : SourceMgr(nullptr) {}
+
   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() override {
+    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;
 };
 
 /// 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.
   auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
   if (!R.isValid()) // Fall back to location only, let the editor deal with it.
     R = CharSourceRange::getCharRange(Loc);
   return toRange(R, M);
 }
 
 TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
                     const LangOptions &L) {
   TextEdit Result;
   Result.range = toRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M);
   Result.newText = FixIt.CodeToInsert;
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) {
 
   std::vector<DiagWithFixIts> ASTDiags;
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
+  IncludeReferenceMap IRM;
+  // Copy over the includes from the preamble, then combine with the
+  // non-preamble includes below.
+  if (Preamble)
+    IRM = Preamble->IRM;
 
   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);
   }

 }
 
 const std::vector<DiagWithFixIts> &ParsedAST::getDiagnostics() const {
   return Diags;
 }
 
 PreambleData::PreambleData(PrecompiledPreamble Preamble,
                            std::vector<serialization::DeclID> TopLevelDeclIDs,
-                           std::vector<DiagWithFixIts> Diags)
+                           std::vector<DiagWithFixIts> Diags,
+                           IncludeReferenceMap IRM)
     : Preamble(std::move(Preamble)),
-      TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)) {}
+      TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)),
+      IRM(std::move(IRM)) {}
 
 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;
       CI =
           createInvocationFromCommandLine(ArgStrs, CommandLineDiagsEngine, VFS);
       // createInvocationFromCommandLine sets DisableFree.
       CI->getFrontendOpts().DisableFree = false;
     }
     assert(CI && "Couldn't create CompilerInvocation");
 
     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 =
           ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
auto DoRebuildPreamble =
 
       if (BuiltPreamble) {
         log(Ctx, "Built preamble of size " + Twine(BuiltPreamble->getSize()) +
                      " for file " + Twine(That->FileName));
 
         return std::make_shared<PreambleData>(
             std::move(*BuiltPreamble),
             SerializedDeclsCollector.takeTopLevelDeclIDs(),
-            std::move(PreambleDiags));
+            std::move(PreambleDiags),
+            SerializedDeclsCollector.takeIncludeReferenceMap());
       } else {
         log(Ctx,
             "Could not build a preamble for file " + Twine(That->FileName));
         return nullptr;
       }
     };
 
     // Compute updated Preamble.
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);
     }
 
     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.

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);
+  }
+};
 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/SourceCode.h

 //===--- SourceCode.h - Manipulating source code as strings -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Various code that examines C++ source code without using heavy AST machinery
 // (and often not even the lexer). To be used sparingly!
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
+#include "clang/Basic/SourceLocation.h"
 
 namespace clang {
+class SourceManager;
+
 namespace clangd {
 
 /// Turn a [line, column] pair into an offset in Code.
 size_t positionToOffset(llvm::StringRef Code, Position P);
 
 /// Turn an offset in Code into a [line, column] pair.
 Position offsetToPosition(llvm::StringRef Code, size_t Offset);
 
+/// Turn a SourceLocation into a [line, column] pair.
+Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
+
 } // namespace clangd
 } // namespace clang
 #endif

clangd/SourceCode.cpp

 //===--- SourceCode.h - Manipulating source code as strings -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include "SourceCode.h"
 
+#include "clang/Basic/SourceManager.h"
+
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
 size_t positionToOffset(StringRef Code, Position P) {
   if (P.line < 0)
     return 0;
   size_t StartOfLine = 0;
Position offsetToPosition(StringRef Code, size_t Offset) {
   StringRef Before = Code.substr(0, Offset);
   int Lines = Before.count('\n');
   size_t PrevNL = Before.rfind('\n');
   size_t StartOfLine = (PrevNL == StringRef::npos) ? 0 : (PrevNL + 1);
   // FIXME: officially character counts UTF-16 code units, not UTF-8 bytes!
   return {Lines, static_cast<int>(Offset - StartOfLine)};
 }
 
+Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) {
+  return {static_cast<int>(SM.getSpellingLineNumber(Loc)) - 1,
+          static_cast<int>(SM.getSpellingColumnNumber(Loc)) - 1};
+}
+
 } // namespace clangd
 } // namespace clang
 

clangd/XRefs.cpp

 //===--- XRefs.cpp ----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 #include "XRefs.h"
+#include "SourceCode.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 namespace clang {
 namespace clangd {
 using namespace llvm;
 namespace {
 
 /// Finds declarations locations that a given source location refers to.
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<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);
   }
 
   bool
getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
   SourceLocation LocStart = ValSourceRange.getBegin();
 
   const FileEntry *F =
       SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
   if (!F)
     return llvm::None;
   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;
+  Position Begin = sourceLocToPosition(SourceMgr, LocStart);
+  Position End = sourceLocToPosition(SourceMgr, LocEnd);
   Range R = {Begin, End};
   Location L;
 
   StringRef FilePath = F->tryGetRealPathName();
   if (FilePath.empty())
     FilePath = F->getName();
   L.uri = URI::fromFile(FilePath);
   L.range = R;
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();
+    Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
+
+    if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+        Pos.character <= 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)
handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
     DocumentHighlights.push_back(getDocumentHighlight(SR, Kind));
     return true;
   }
 
 private:
   DocumentHighlight getDocumentHighlight(SourceRange SR,
                                          DocumentHighlightKind Kind) {
     const SourceManager &SourceMgr = AST.getSourceManager();
-    SourceLocation LocStart = SR.getBegin();
-    Position Begin;
-    Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-    Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-    Position End;
-    End.line = SourceMgr.getSpellingLineNumber(SR.getEnd()) - 1;
-    End.character = SourceMgr.getSpellingColumnNumber(SR.getEnd()) - 1;
+    Position Begin = sourceLocToPosition(SourceMgr, SR.getBegin());
+    Position End = sourceLocToPosition(SourceMgr, SR.getEnd());
     Range R = {Begin, End};
     DocumentHighlight DH;
     DH.range = R;
     DH.kind = Kind;
     return DH;
   }
 };
 

unittests/clangd/ClangdTests.cpp

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);
+
+  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(Context::empty(), FooH, HeaderContents);
+  Server.addDocument(Context::empty(), FooCpp, SourceContents);
+
+  Position P = Position{1, 11};
+
+  auto ExpectedLocations = Server.findDefinitions(Context::empty(), 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 include in preamble
+  Position P2 = Position{1, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P2);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(!Locations.empty());
+
+  // Test invalid include
+  Position P3 = Position{2, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), FooCpp, P3);
+  ASSERT_TRUE(!!ExpectedLocations);
+  Locations = ExpectedLocations->Value;
+  EXPECT_TRUE(Locations.empty());
+
+  // Test include outside of Preamble
+  Position P4 = Position{6, 11};
+
+  ExpectedLocations = Server.findDefinitions(Context::empty(), 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,