Skip to content

[clangd] Allow "move function body out-of-line" in non-header files #69704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 3, 2024
Merged

[clangd] Allow "move function body out-of-line" in non-header files #69704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 93 additions & 57 deletions clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
// looked up in the context containing the function/method.
// FIXME: Drop attributes in function signature.
llvm::Expected<std::string>
getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
const syntax::TokenBuffer &TokBuf,
const HeuristicResolver *Resolver) {
auto &AST = FD->getASTContext();
auto &SM = AST.getSourceManager();
auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
if (!TargetContext)
return error("define outline: couldn't find a context for target");

llvm::Error Errors = llvm::Error::success();
tooling::Replacements DeclarationCleanups;
Expand Down Expand Up @@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
}
const NamedDecl *ND = Ref.Targets.front();
const std::string Qualifier =
getQualification(AST, *TargetContext,
getQualification(AST, TargetContext,
SM.getLocForStartOfFile(SM.getMainFileID()), ND);
if (auto Err = DeclarationCleanups.add(
tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
Expand All @@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
if (const auto *Destructor = llvm::dyn_cast<CXXDestructorDecl>(FD)) {
if (auto Err = DeclarationCleanups.add(tooling::Replacement(
SM, Destructor->getLocation(), 0,
getQualification(AST, *TargetContext,
getQualification(AST, TargetContext,
SM.getLocForStartOfFile(SM.getMainFileID()),
Destructor))))
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
Expand Down Expand Up @@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
}

struct InsertionPoint {
std::string EnclosingNamespace;
const DeclContext *EnclosingNamespace = nullptr;
size_t Offset;
};
// Returns the most natural insertion point for \p QualifiedName in \p Contents.
// This currently cares about only the namespace proximity, but in feature it
// should also try to follow ordering of declarations. For example, if decls
// come in order `foo, bar, baz` then this function should return some point
// between foo and baz for inserting bar.
llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
llvm::StringRef QualifiedName,
const LangOptions &LangOpts) {
auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);

assert(!Region.EligiblePoints.empty());
// FIXME: This selection can be made smarter by looking at the definition
// locations for adjacent decls to Source. Unfortunately pseudo parsing in
// getEligibleRegions only knows about namespace begin/end events so we
// can't match function start/end positions yet.
auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
if (!Offset)
return Offset.takeError();
return InsertionPoint{Region.EnclosingNamespace, *Offset};
}

// Returns the range that should be deleted from declaration, which always
// contains function body. In addition to that it might contain constructor
Expand Down Expand Up @@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
}

bool prepare(const Selection &Sel) override {
// Bail out if we are not in a header file.
// FIXME: We might want to consider moving method definitions below class
// definition even if we are inside a source file.
if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
Sel.AST->getLangOpts()))
return false;

SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());

// Bail out if the selection is not a in-line function definition.
if (!Source || !Source->doesThisDeclarationHaveABody() ||
Source->isOutOfLine())
Expand All @@ -429,19 +401,24 @@ class DefineOutline : public Tweak {
if (Source->getTemplateSpecializationInfo())
return false;

if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
// Bail out in templated classes, as it is hard to spell the class name,
// i.e if the template parameter is unnamed.
if (MD->getParent()->isTemplated())
return false;

// The refactoring is meaningless for unnamed classes and definitions
// within unnamed namespaces.
for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
if (ND->getDeclName().isEmpty())
return false;
}
auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source);
if (!MD) {
// Can't outline free-standing functions in the same file.
return !SameFile;
}

// Bail out in templated classes, as it is hard to spell the class name,
// i.e if the template parameter is unnamed.
if (MD->getParent()->isTemplated())
return false;

// The refactoring is meaningless for unnamed classes and namespaces,
// unless we're outlining in the same file
for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
if (ND->getDeclName().isEmpty() &&
(!SameFile || !llvm::dyn_cast<NamespaceDecl>(ND)))
return false;
}
}

Expand All @@ -453,8 +430,8 @@ class DefineOutline : public Tweak {

Expected<Effect> apply(const Selection &Sel) override {
const SourceManager &SM = Sel.AST->getSourceManager();
auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);

auto CCFile = SameFile ? Sel.AST->tuPath().str()
: getSourceFile(Sel.AST->tuPath(), Sel);
if (!CCFile)
return error("Couldn't find a suitable implementation file.");
assert(Sel.FS && "FS Must be set in apply");
Expand All @@ -464,8 +441,7 @@ class DefineOutline : public Tweak {
if (!Buffer)
return llvm::errorCodeToError(Buffer.getError());
auto Contents = Buffer->get()->getBuffer();
auto InsertionPoint = getInsertionPoint(
Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
auto InsertionPoint = getInsertionPoint(Contents, Sel);
if (!InsertionPoint)
return InsertionPoint.takeError();

Expand Down Expand Up @@ -499,17 +475,77 @@ class DefineOutline : public Tweak {
HeaderUpdates = HeaderUpdates.merge(*DelInline);
}

auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
if (!HeaderFE)
return HeaderFE.takeError();

Effect->ApplyEdits.try_emplace(HeaderFE->first,
std::move(HeaderFE->second));
if (SameFile) {
tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
R = R.merge(HeaderUpdates);
} else {
auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
if (!HeaderFE)
return HeaderFE.takeError();
Effect->ApplyEdits.try_emplace(HeaderFE->first,
std::move(HeaderFE->second));
}
return std::move(*Effect);
}

// Returns the most natural insertion point for \p QualifiedName in \p
// Contents. This currently cares about only the namespace proximity, but in
// feature it should also try to follow ordering of declarations. For example,
// if decls come in order `foo, bar, baz` then this function should return
// some point between foo and baz for inserting bar.
// FIXME: The selection can be made smarter by looking at the definition
// locations for adjacent decls to Source. Unfortunately pseudo parsing in
// getEligibleRegions only knows about namespace begin/end events so we
// can't match function start/end positions yet.
llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
const Selection &Sel) {
// If the definition goes to the same file and there is a namespace,
// we should (and, in the case of anonymous namespaces, need to)
// put the definition into the original namespace block.
if (SameFile) {
auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext();
if (!Klass)
return error("moving to same file not supported for free functions");
const SourceLocation EndLoc = Klass->getBraceRange().getEnd();
const auto &TokBuf = Sel.AST->getTokens();
auto Tokens = TokBuf.expandedTokens();
auto It = llvm::lower_bound(
Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) {
return Tok.location() < EndLoc;
});
while (It != Tokens.end()) {
if (It->kind() != tok::semi) {
++It;
continue;
}
unsigned Offset = Sel.AST->getSourceManager()
.getDecomposedLoc(It->endLocation())
.second;
return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
}
return error(
"failed to determine insertion location: no end of class found");
}

auto Region = getEligiblePoints(
Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());

assert(!Region.EligiblePoints.empty());
auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
if (!Offset)
return Offset.takeError();

auto TargetContext =
findContextForNS(Region.EnclosingNamespace, Source->getDeclContext());
if (!TargetContext)
return error("define outline: couldn't find a context for target");

return InsertionPoint{*TargetContext, *Offset};
}

private:
const FunctionDecl *Source = nullptr;
bool SameFile = false;
};

REGISTER_TWEAK(DefineOutline)
Expand Down
73 changes: 71 additions & 2 deletions clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,47 @@ TWEAK_TEST(DefineOutline);

TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
FileName = "Test.cpp";
// Not available unless in a header file.
// Not available for free function unless in a header file.
EXPECT_UNAVAILABLE(R"cpp(
[[void [[f^o^o]]() [[{
return;
}]]]])cpp");

// Available in soure file.
EXPECT_AVAILABLE(R"cpp(
struct Foo {
void f^oo() {}
};
)cpp");

// Available within named namespace in source file.
EXPECT_AVAILABLE(R"cpp(
namespace N {
struct Foo {
void f^oo() {}
};
} // namespace N
)cpp");

// Available within anonymous namespace in source file.
EXPECT_AVAILABLE(R"cpp(
namespace {
struct Foo {
void f^oo() {}
};
} // namespace
)cpp");

// Not available for out-of-line method.
EXPECT_UNAVAILABLE(R"cpp(
class Bar {
void baz();
};

[[void [[Bar::[[b^a^z]]]]() [[{
return;
}]]]])cpp");

FileName = "Test.hpp";
// Not available unless function name or fully body is selected.
EXPECT_UNAVAILABLE(R"cpp(
Expand Down Expand Up @@ -100,7 +135,7 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
};
)cpp");

// Not available on definitions within unnamed namespaces
// Not available on definitions in header file within unnamed namespaces
EXPECT_UNAVAILABLE(R"cpp(
namespace {
struct Foo {
Expand Down Expand Up @@ -349,6 +384,40 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}

TEST_F(DefineOutlineTest, InCppFile) {
FileName = "Test.cpp";

struct {
llvm::StringRef Test;
llvm::StringRef ExpectedSource;
} Cases[] = {
{
R"cpp(
namespace foo {
namespace {
struct Foo { void ba^r() {} };
struct Bar { void foo(); };
void Bar::foo() {}
}
}
)cpp",
R"cpp(
namespace foo {
namespace {
struct Foo { void bar() ; };void Foo::bar() {}
struct Bar { void foo(); };
void Bar::foo() {}
}
}
)cpp"},
};

for (const auto &Case : Cases) {
SCOPED_TRACE(Case.Test);
EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource);
}
}

TEST_F(DefineOutlineTest, HandleMacros) {
llvm::StringMap<std::string> EditedFiles;
ExtraFiles["Test.cpp"] = "";
Expand Down