[clangd] Fix a case where we fail to detect a header-declared symbol in rename.
Summary: Failing case: ``` #include "foo.h" void fo^o() {} ``` getRenameDecl() returns the decl of the symbol under the cursor (which is in the current main file), instead, we use the canonical decl to determine whether a symbol is declared in #included header. Reviewers: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D63872 llvm-svn: 364537
This commit is contained in:
parent
71d3869f60
commit
93a825c8fb
|
@ -67,15 +67,14 @@ llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
|
|||
}
|
||||
|
||||
// Query the index to find some other files where the Decl is referenced.
|
||||
llvm::Optional<std::string> getOtherRefFile(const NamedDecl &Decl,
|
||||
StringRef MainFile,
|
||||
llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
|
||||
const SymbolIndex &Index) {
|
||||
RefsRequest Req;
|
||||
// We limit the number of results, this is a correctness/performance
|
||||
// tradeoff. We expect the number of symbol references in the current file
|
||||
// is smaller than the limit.
|
||||
Req.Limit = 100;
|
||||
if (auto ID = getSymbolID(&Decl))
|
||||
if (auto ID = getSymbolID(&D))
|
||||
Req.IDs.insert(*ID);
|
||||
llvm::Optional<std::string> OtherFile;
|
||||
Index.refs(Req, [&](const Ref &R) {
|
||||
|
@ -97,16 +96,16 @@ enum ReasonToReject {
|
|||
};
|
||||
|
||||
// Check the symbol Decl is renameable (per the index) within the file.
|
||||
llvm::Optional<ReasonToReject> renamableWithinFile(const NamedDecl &Decl,
|
||||
llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
|
||||
StringRef MainFile,
|
||||
const SymbolIndex *Index) {
|
||||
if (llvm::isa<NamespaceDecl>(&Decl))
|
||||
if (llvm::isa<NamespaceDecl>(&RenameDecl))
|
||||
return ReasonToReject::UnsupportedSymbol;
|
||||
auto &ASTCtx = Decl.getASTContext();
|
||||
auto &ASTCtx = RenameDecl.getASTContext();
|
||||
const auto &SM = ASTCtx.getSourceManager();
|
||||
bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
|
||||
bool DeclaredInMainFile =
|
||||
SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation()));
|
||||
SM.isWrittenInMainFile(SM.getExpansionLoc(RenameDecl.getLocation()));
|
||||
|
||||
// If the symbol is declared in the main file (which is not a header), we
|
||||
// rename it.
|
||||
|
@ -115,18 +114,19 @@ llvm::Optional<ReasonToReject> renamableWithinFile(const NamedDecl &Decl,
|
|||
|
||||
// Below are cases where the symbol is declared in the header.
|
||||
// If the symbol is function-local, we rename it.
|
||||
if (Decl.getParentFunctionOrMethod())
|
||||
if (RenameDecl.getParentFunctionOrMethod())
|
||||
return None;
|
||||
|
||||
if (!Index)
|
||||
return ReasonToReject::NoIndexProvided;
|
||||
|
||||
bool IsIndexable =
|
||||
SymbolCollector::shouldCollectSymbol(Decl, ASTCtx, {}, false);
|
||||
bool IsIndexable = isa<NamedDecl>(RenameDecl) &&
|
||||
SymbolCollector::shouldCollectSymbol(
|
||||
cast<NamedDecl>(RenameDecl), ASTCtx, {}, false);
|
||||
// If the symbol is not indexable, we disallow rename.
|
||||
if (!IsIndexable)
|
||||
return ReasonToReject::NonIndexable;
|
||||
auto OtherFile = getOtherRefFile(Decl, MainFile, *Index);
|
||||
auto OtherFile = getOtherRefFile(RenameDecl, MainFile, *Index);
|
||||
// If the symbol is indexable and has no refs from other files in the index,
|
||||
// we rename it.
|
||||
if (!OtherFile)
|
||||
|
@ -154,7 +154,8 @@ renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
|
|||
|
||||
const auto *RenameDecl = Rename->getRenameDecl();
|
||||
assert(RenameDecl && "symbol must be found at this point");
|
||||
if (auto Reject = renamableWithinFile(*RenameDecl, File, Index)) {
|
||||
if (auto Reject =
|
||||
renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index)) {
|
||||
auto Message = [](ReasonToReject Reason) {
|
||||
switch (Reason) {
|
||||
case NoIndexProvided:
|
||||
|
|
|
@ -78,7 +78,6 @@ TEST(RenameTest, SingleFile) {
|
|||
for (const Test &T : Tests) {
|
||||
Annotations Code(T.Before);
|
||||
auto TU = TestTU::withCode(Code.code());
|
||||
TU.HeaderCode = "void foo();"; // outside main file, will not be touched.
|
||||
auto AST = TU.build();
|
||||
auto RenameResult =
|
||||
renameWithinFile(AST, testPath(TU.Filename), Code.point(), "abcde");
|
||||
|
@ -92,58 +91,68 @@ TEST(RenameTest, SingleFile) {
|
|||
}
|
||||
|
||||
TEST(RenameTest, Renameable) {
|
||||
// Test cases where the symbol is declared in header.
|
||||
struct Case {
|
||||
const char* HeaderCode;
|
||||
const char *Code;
|
||||
const char* ErrorMessage; // null if no error
|
||||
bool IsHeaderFile;
|
||||
};
|
||||
const bool HeaderFile = true;
|
||||
Case Cases[] = {
|
||||
{R"cpp(// allow -- function-local
|
||||
void f(int [[Lo^cal]]) {
|
||||
[[Local]] = 2;
|
||||
}
|
||||
)cpp",
|
||||
nullptr},
|
||||
nullptr, HeaderFile},
|
||||
|
||||
{R"cpp(// allow -- symbol is indexable and has no refs in index.
|
||||
void [[On^lyInThisFile]]();
|
||||
)cpp",
|
||||
nullptr},
|
||||
nullptr, HeaderFile},
|
||||
|
||||
{R"cpp(// disallow -- symbol is indexable and has other refs in index.
|
||||
void f() {
|
||||
Out^side s;
|
||||
}
|
||||
)cpp",
|
||||
"used outside main file"},
|
||||
"used outside main file", HeaderFile},
|
||||
|
||||
{R"cpp(// disallow -- symbol is not indexable.
|
||||
namespace {
|
||||
class Unin^dexable {};
|
||||
}
|
||||
)cpp",
|
||||
"not eligible for indexing"},
|
||||
"not eligible for indexing", HeaderFile},
|
||||
|
||||
{R"cpp(// disallow -- namespace symbol isn't supported
|
||||
namespace fo^o {}
|
||||
)cpp",
|
||||
"not a supported kind"},
|
||||
"not a supported kind", HeaderFile},
|
||||
|
||||
{R"cpp(// foo is declared outside the file.
|
||||
void fo^o() {}
|
||||
)cpp", "used outside main file", !HeaderFile/*cc file*/},
|
||||
};
|
||||
const char *CommonHeader = "class Outside {};";
|
||||
TestTU OtherFile = TestTU::withCode("Outside s;");
|
||||
const char *CommonHeader = R"cpp(
|
||||
class Outside {};
|
||||
void foo();
|
||||
)cpp";
|
||||
TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
|
||||
OtherFile.HeaderCode = CommonHeader;
|
||||
OtherFile.Filename = "other.cc";
|
||||
// The index has a "Outside" reference.
|
||||
// The index has a "Outside" reference and a "foo" reference.
|
||||
auto OtherFileIndex = OtherFile.index();
|
||||
|
||||
for (const auto& Case : Cases) {
|
||||
Annotations T(Case.HeaderCode);
|
||||
// We open the .h file as the main file.
|
||||
Annotations T(Case.Code);
|
||||
TestTU TU = TestTU::withCode(T.code());
|
||||
TU.Filename = "test.h";
|
||||
TU.HeaderCode = CommonHeader;
|
||||
// Parsing the .h file as C++ include.
|
||||
TU.ExtraArgs.push_back("-xobjective-c++-header");
|
||||
if (Case.IsHeaderFile) {
|
||||
// We open the .h file as the main file.
|
||||
TU.Filename = "test.h";
|
||||
// Parsing the .h file as C++ include.
|
||||
TU.ExtraArgs.push_back("-xobjective-c++-header");
|
||||
}
|
||||
auto AST = TU.build();
|
||||
|
||||
auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
|
||||
|
|
Loading…
Reference in New Issue