ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
File paths in URIForFile can come from index or local AST. Path from
index goes through URI transformation and the final path is resolved by URI
scheme and could be potentially different from the original path. Hence, we
should do the same transformation for all paths. We do this in URIForFile, which
now converts a path to URI and back to a canonicalized path.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54845
Files:
clangd/FindSymbols.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/URI.cpp
clangd/URI.h
clangd/XRefs.cpp
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/TestFS.cpp
unittests/clangd/URITests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -396,21 +396,21 @@
auto Locations =
runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
- EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+ EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
SourceAnnotations.range()}));
// Go to a definition in header_in_preamble.h.
Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(*Locations,
- ElementsAre(Location{URIForFile{HeaderInPreambleH},
+ ElementsAre(Location{URIForFile(HeaderInPreambleH),
HeaderInPreambleAnnotations.range()}));
// Go to a definition in header_not_in_preamble.h.
Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(*Locations,
- ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+ ElementsAre(Location{URIForFile(HeaderNotInPreambleH),
HeaderNotInPreambleAnnotations.range()}));
}
@@ -1047,7 +1047,7 @@
Annotations SourceAnnotations(SourceContents);
FS.Files[FooCpp] = SourceAnnotations.code();
auto FooH = testPath("foo.h");
- auto FooHUri = URIForFile{FooH};
+ URIForFile FooHUri(FooH);
const char *HeaderContents = R"cpp([[]]#pragma once
int a;
Index: unittests/clangd/URITests.cpp
===================================================================
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -152,6 +152,28 @@
testPath("a"));
}
+std::string resolvePathOrDie(StringRef AbsPath, StringRef HintPath = "") {
+ auto Path = URI::resolvePath(AbsPath, HintPath);
+ if (!Path)
+ llvm_unreachable(toString(Path.takeError()).c_str());
+ return *Path;
+}
+
+TEST(URITest, ResolvePath) {
+ StringRef FilePath =
+#ifdef _WIN32
+ "c:\\x\\y\\z";
+#else
+ "/a/b/c";
+#endif
+ EXPECT_EQ(resolvePathOrDie(FilePath), FilePath);
+ EXPECT_EQ(resolvePathOrDie(testPath("x"), testPath("hint")), testPath("x"));
+ // HintPath is not in testRoot(); resolution fails.
+ auto Resolve = URI::resolvePath(testPath("x"), FilePath);
+ EXPECT_FALSE(Resolve);
+ llvm::consumeError(Resolve.takeError());
+}
+
TEST(URITest, Platform) {
auto Path = testPath("x");
auto U = URI::create(Path, "file");
Index: unittests/clangd/TestFS.cpp
===================================================================
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -90,7 +90,10 @@
Expected<std::string> getAbsolutePath(StringRef /*Authority*/, StringRef Body,
StringRef HintPath) const override {
- assert(HintPath.startswith(testRoot()));
+ if (!HintPath.startswith(testRoot()))
+ return make_error<StringError>(
+ "Hint path doesn't start with test root: " + HintPath,
+ inconvertibleErrorCode());
if (!Body.consume_front("/"))
return make_error<StringError>(
"Body of an unittest: URI must start with '/'",
Index: unittests/clangd/ClangdUnitTests.cpp
===================================================================
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -235,9 +235,9 @@
toLSPDiags(
D,
#ifdef _WIN32
- URIForFile("c:\\path\\to\\foo\\bar\\main.cpp"),
+ URIForFile(StringRef("c:\\path\\to\\foo\\bar\\main.cpp")),
#else
- URIForFile("/path/to/foo/bar/main.cpp"),
+ URIForFile(StringRef("/path/to/foo/bar/main.cpp")),
#endif
ClangdDiagnosticOptions(),
[&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -457,7 +457,7 @@
auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
EXPECT_TRUE(bool(Locations));
- EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+ EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
FooSource.range("one")}));
// Undefine MACRO, close baz.cpp.
@@ -473,7 +473,7 @@
Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
EXPECT_TRUE(bool(Locations));
- EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+ EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
FooSource.range("two")}));
}
Index: clangd/XRefs.cpp
===================================================================
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -52,16 +52,17 @@
return None;
auto Uri = URI::parse(Loc.FileURI);
if (!Uri) {
- log("Could not parse URI: {0}", Loc.FileURI);
+ elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
return None;
}
- auto Path = URI::resolve(*Uri, HintPath);
- if (!Path) {
- log("Could not resolve URI: {0}", Loc.FileURI);
+ auto U = URIForFile::fromURI(*Uri, HintPath);
+ if (!U) {
+ elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError());
return None;
}
+
Location LSPLoc;
- LSPLoc.uri = URIForFile(*Path);
+ LSPLoc.uri = std::move(*U);
LSPLoc.range.start.line = Loc.Start.line();
LSPLoc.range.start.character = Loc.Start.column();
LSPLoc.range.end.line = Loc.End.line();
@@ -224,7 +225,8 @@
sourceLocToPosition(SourceMgr, LocEnd)};
}
-Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc) {
+Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc,
+ StringRef HintPath) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
if (!F)
@@ -235,34 +237,39 @@
return None;
}
Location L;
- L.uri = URIForFile(*FilePath);
+ L.uri = URIForFile(*FilePath, HintPath);
L.range = getTokenRange(AST, TokLoc);
return L;
}
} // namespace
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
const SymbolIndex *Index) {
- const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+ const auto &SM = AST.getASTContext().getSourceManager();
+ auto MainFilePath = getRealPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+ if (!MainFilePath) {
+ elog("Failed to get a path for the main file, so no references");
+ return {};
+ }
std::vector<Location> Result;
// Handle goto definition for #include.
for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line)
- Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
+ Result.push_back(Location{URIForFile(Inc.Resolved, *MainFilePath), {}});
}
if (!Result.empty())
return Result;
// Identified symbols at a specific position.
SourceLocation SourceLocationBeg =
- getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+ getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
for (auto Item : Symbols.Macros) {
auto Loc = Item.Info->getDefinitionLoc();
- auto L = makeLocation(AST, Loc);
+ auto L = makeLocation(AST, Loc, *MainFilePath);
if (L)
Result.push_back(*L);
}
@@ -312,7 +319,7 @@
auto &Candidate = ResultCandidates[R.first->second];
auto Loc = findNameLoc(D);
- auto L = makeLocation(AST, Loc);
+ auto L = makeLocation(AST, Loc, *MainFilePath);
// The declaration in the identified symbols is a definition if possible
// otherwise it is declaration.
bool IsDef = getDefinition(D) == D;
@@ -330,8 +337,8 @@
QueryRequest.IDs.insert(It.first);
std::string HintPath;
const FileEntry *FE =
- SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
- if (auto Path = getRealPath(FE, SourceMgr))
+ SM.getFileEntryForID(SM.getMainFileID());
+ if (auto Path = getRealPath(FE, SM))
HintPath = *Path;
// Query the index and populate the empty slot.
Index->lookup(QueryRequest, [&HintPath, &ResultCandidates,
Index: clangd/URI.h
===================================================================
--- clangd/URI.h
+++ clangd/URI.h
@@ -64,6 +64,13 @@
static llvm::Expected<std::string> resolve(const URI &U,
llvm::StringRef HintPath = "");
+ /// Resolves \p AbsPath into a canonical path of its URI, by converting
+ /// \p AbsPath to URI and resolving the URI to get th canonical path.
+ /// This ensures that paths with the same URI are resolved into consistent
+ /// file path.
+ static llvm::Expected<std::string> resolvePath(llvm::StringRef AbsPath,
+ llvm::StringRef HintPath = "");
+
/// Gets the preferred spelling of this file for #include, if there is one,
/// e.g. <system_header.h>, "path/to/x.h".
///
Index: clangd/URI.cpp
===================================================================
--- clangd/URI.cpp
+++ clangd/URI.cpp
@@ -231,6 +231,13 @@
return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
}
+Expected<std::string> URI::resolvePath(StringRef AbsPath, StringRef HintPath) {
+ auto U = create(AbsPath);
+ // "file" scheme doesn't do anything fancy; it would resolve to the same
+ // \p AbsPath.
+ return U.scheme() == "file" ? AbsPath : resolve(U, HintPath);
+}
+
Expected<std::string> URI::includeSpelling(const URI &Uri) {
auto S = findSchemeByName(Uri.Scheme);
if (!S)
Index: clangd/Protocol.h
===================================================================
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -66,9 +66,15 @@
}
};
+// URI in "file" scheme for a file.
struct URIForFile {
URIForFile() = default;
- explicit URIForFile(std::string AbsPath);
+
+ /// \p AbsPath is resolved to a canonical path corresponding to its URI.
+ URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = "");
+
+ static llvm::Expected<URIForFile> fromURI(const URI &U,
+ llvm::StringRef HintPath);
/// Retrieves absolute path to the file.
llvm::StringRef file() const { return File; }
@@ -89,6 +95,8 @@
}
private:
+ explicit URIForFile(std::string &&File) : File(std::move(File)) {}
+
std::string File;
};
Index: clangd/Protocol.cpp
===================================================================
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -27,29 +27,45 @@
char LSPError::ID;
-URIForFile::URIForFile(std::string AbsPath) {
+URIForFile::URIForFile(StringRef AbsPath, StringRef HintPath) {
assert(sys::path::is_absolute(AbsPath) && "the path is relative");
- File = std::move(AbsPath);
+ auto Resolved = URI::resolvePath(AbsPath, HintPath);
+ if (!Resolved) {
+ elog("URIForFile: failed to resolve path {0} with hint path {1}: "
+ "{2}.\nUsing unresolved path.",
+ AbsPath, HintPath, Resolved.takeError());
+ File = AbsPath;
+ } else {
+ File = *Resolved;
+ }
+}
+
+Expected<URIForFile> URIForFile::fromURI(const URI &U, StringRef HintPath) {
+ auto Resolved = URI::resolve(U, HintPath);
+ if (!Resolved)
+ return Resolved.takeError();
+ return URIForFile(std::move(*Resolved));
}
bool fromJSON(const json::Value &E, URIForFile &R) {
if (auto S = E.getAsString()) {
- auto U = URI::parse(*S);
- if (!U) {
- elog("Failed to parse URI {0}: {1}", *S, U.takeError());
+ auto Parsed = URI::parse(*S);
+ if (!Parsed) {
+ elog("Failed to parse URI {0}: {1}", *S, Parsed.takeError());
return false;
}
- if (U->scheme() != "file" && U->scheme() != "test") {
+ if (Parsed->scheme() != "file" && Parsed->scheme() != "test") {
elog("Clangd only supports 'file' URI scheme for workspace files: {0}",
*S);
return false;
}
- auto Path = URI::resolve(*U);
- if (!Path) {
- log("{0}", Path.takeError());
+ // "file" and "test" schemes do not require hint path.
+ auto U = URIForFile::fromURI(*Parsed, /*HintPath=*/"");
+ if (!U) {
+ elog("{0}", U.takeError());
return false;
}
- R = URIForFile(*Path);
+ R = std::move(*U);
return true;
}
return false;
Index: clangd/FindSymbols.cpp
===================================================================
--- clangd/FindSymbols.cpp
+++ clangd/FindSymbols.cpp
@@ -140,7 +140,7 @@
return;
}
Location L;
- L.uri = URIForFile((*Path));
+ L.uri = URIForFile(*Path, HintPath);
Position Start, End;
Start.line = CD.Start.line();
Start.character = CD.Start.column();
@@ -196,9 +196,9 @@
const FileEntry *F = SM.getFileEntryForID(SM.getMainFileID());
if (!F)
return;
- auto FilePath = getRealPath(F, SM);
- if (FilePath)
- MainFileUri = URIForFile(*FilePath);
+ auto MainFilePath = getRealPath(F, SM);
+ if (MainFilePath)
+ MainFileUri = URIForFile(*MainFilePath);
}
bool shouldIncludeSymbol(const NamedDecl *ND) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
