[clang-tools-extra] r364537 - [clangd] Fix a case where we fail to detect a header-declared symbol in rename.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 27 06:24:10 PDT 2019
Author: hokein
Date: Thu Jun 27 06:24:10 2019
New Revision: 364537
URL: http://llvm.org/viewvc/llvm-project?rev=364537&view=rev
Log:
[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
Modified:
clang-tools-extra/trunk/clangd/refactor/Rename.cpp
clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp
Modified: clang-tools-extra/trunk/clangd/refactor/Rename.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/Rename.cpp?rev=364537&r1=364536&r2=364537&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/refactor/Rename.cpp (original)
+++ clang-tools-extra/trunk/clangd/refactor/Rename.cpp Thu Jun 27 06:24:10 2019
@@ -67,15 +67,14 @@ llvm::Optional<std::string> filePath(con
}
// 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> renamable
// 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::S
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:
Modified: clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp?rev=364537&r1=364536&r2=364537&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/RenameTests.cpp Thu Jun 27 06:24:10 2019
@@ -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(),
More information about the cfe-commits
mailing list