diff --git a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp @@ -48,6 +48,10 @@ NestedNameSpecifierLoc QualifierToRemove; // The name following QualifierToRemove. llvm::StringRef Name; + // If valid, the insertion point for "using" statement must come after this. + // This is relevant when the type is defined in the main file, to make sure + // the type/function is already defined at the point where "using" is added. + SourceLocation MustInsertAfterLoc; }; REGISTER_TWEAK(AddUsing) @@ -120,7 +124,8 @@ llvm::Expected findInsertionPoint(const Tweak::Selection &Inputs, const NestedNameSpecifierLoc &QualifierToRemove, - const llvm::StringRef Name) { + const llvm::StringRef Name, + const SourceLocation MustInsertAfterLoc) { auto &SM = Inputs.AST->getSourceManager(); // Search for all using decls that affect this point in file. We need this for @@ -132,6 +137,11 @@ SM) .TraverseAST(Inputs.AST->getASTContext()); + auto IsValidPoint = [&](const SourceLocation Loc) { + return MustInsertAfterLoc.isInvalid() || + SM.isBeforeInTranslationUnit(MustInsertAfterLoc, Loc); + }; + bool AlwaysFullyQualify = true; for (auto &U : Usings) { // Only "upgrade" to fully qualified is all relevant using decls are fully @@ -149,12 +159,13 @@ U->getName() == Name) { return InsertionPointData(); } + // Insertion point will be before last UsingDecl that affects cursor // position. For most cases this should stick with the local convention of // add using inside or outside namespace. LastUsingLoc = U->getUsingLoc(); } - if (LastUsingLoc.isValid()) { + if (LastUsingLoc.isValid() && IsValidPoint(LastUsingLoc)) { InsertionPointData Out; Out.Loc = LastUsingLoc; Out.AlwaysFullyQualify = AlwaysFullyQualify; @@ -175,7 +186,7 @@ if (Tok == Toks.end() || Tok->endLocation().isInvalid()) { return error("Namespace with no {"); } - if (!Tok->endLocation().isMacroID()) { + if (!Tok->endLocation().isMacroID() && IsValidPoint(Tok->endLocation())) { InsertionPointData Out; Out.Loc = Tok->endLocation(); Out.Suffix = "\n"; @@ -183,15 +194,17 @@ } } // No using, no namespace, no idea where to insert. Try above the first - // top level decl. + // top level decl after MustInsertAfterLoc. auto TLDs = Inputs.AST->getLocalTopLevelDecls(); - if (TLDs.empty()) { - return error("Cannot find place to insert \"using\""); + for (const auto &TLD : TLDs) { + if (!IsValidPoint(TLD->getBeginLoc())) + continue; + InsertionPointData Out; + Out.Loc = SM.getExpansionLoc(TLD->getBeginLoc()); + Out.Suffix = "\n\n"; + return Out; } - InsertionPointData Out; - Out.Loc = SM.getExpansionLoc(TLDs[0]->getBeginLoc()); - Out.Suffix = "\n\n"; - return Out; + return error("Cannot find place to insert \"using\""); } bool isNamespaceForbidden(const Tweak::Selection &Inputs, @@ -254,6 +267,7 @@ if (auto *II = D->getDecl()->getIdentifier()) { QualifierToRemove = D->getQualifierLoc(); Name = II->getName(); + MustInsertAfterLoc = D->getDecl()->getBeginLoc(); } } else if (auto *T = Node->ASTNode.get()) { if (auto E = T->getAs()) { @@ -271,6 +285,15 @@ QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy()); if (!Name.consume_front(QualifierToRemoveStr)) return false; // What's spelled doesn't match the qualifier. + + if (const auto *ET = E.getTypePtr()) { + if (const auto *TDT = + dyn_cast(ET->getNamedType().getTypePtr())) { + MustInsertAfterLoc = TDT->getDecl()->getBeginLoc(); + } else if (auto *TD = ET->getAsTagDecl()) { + MustInsertAfterLoc = TD->getBeginLoc(); + } + } } } @@ -312,7 +335,8 @@ return std::move(Err); } - auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, Name); + auto InsertionPoint = + findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc); if (!InsertionPoint) { return InsertionPoint.takeError(); } diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -2825,7 +2825,66 @@ namespace {using two::cc; cc C; -})cpp"}}; +})cpp"}, + // Type defined in main file, make sure using is after that. + {R"cpp( +namespace xx { + struct yy {}; +} + +x^x::yy X; +)cpp", + R"cpp( +namespace xx { + struct yy {}; +} + +using xx::yy; + +yy X; +)cpp"}, + // Type defined in main file via "using", insert after that. + {R"cpp( +#include "test.hpp" + +namespace xx { + using yy = one::two::cc; +} + +x^x::yy X; +)cpp", + R"cpp( +#include "test.hpp" + +namespace xx { + using yy = one::two::cc; +} + +using xx::yy; + +yy X; +)cpp"}, + // Using must come after function definition. + {R"cpp( +namespace xx { + void yy(); +} + +void fun() { + x^x::yy(); +} +)cpp", + R"cpp( +namespace xx { + void yy(); +} + +using xx::yy; + +void fun() { + yy(); +} +)cpp"}}; llvm::StringMap EditedFiles; for (const auto &Case : Cases) { for (const auto &SubCase : expandCases(Case.TestSource)) {