[clangd] Addusing tweak: find insertion point after definition

[adamcz]

Summary

When type/function is defined in the middle of the file, previuosly we
would sometimes insert a "using" line before that definition, leading to
a compilation error. With this fix, we pick a point after such
definition in translation unit.

This is not perfect solution. For example, it still doesn't handle
"using namespace" directives. It is, however, a significant improvement.

[adamcz]

typo in description

[kadircet]

thanks, LGTM

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

163

why not check for this below? as usings are sorted, we expect LastUsingLoc to be after this point. so maybe change the if below to:

if(LastUsingLoc.isValid() && (!MustInsertAfterLoc.isValid() || SM.isBeforeInTranslationUnit(MustInsertAfterLoc, LastUsingLoc)))

200

nit: maybe factor this into a lambda bool IsUsingValidAt(SourceLocation) ?

[adamcz]

addressed review comments

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

200

Good idea. Done.

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

public:
   }
 
 private:
   // All of the following are set by prepare().
   // The qualifier to remove.
   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)
 
 std::string AddUsing::title() const {
   return std::string(llvm::formatv(
       "Add using-declaration for {0} and remove qualifier", Name));
 }
 

 //
 // The insertion point might be a little awkward if the decl we're anchoring to
 // has a comment in an unfortunate place (e.g. directly above function or using
 // decl, or immediately following "namespace {". We should add some helpers for
 // dealing with that and use them in other code modifications as well.
 llvm::Expected<InsertionPointData>
 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
   // two reasons: to skip adding "using" if one already exists and to find best
   // place to add it, if it doesn't exist.
   SourceLocation LastUsingLoc;
   std::vector<const UsingDecl *> Usings;
   UsingFinder(Usings, &Inputs.ASTSelection.commonAncestor()->getDeclContext(),
               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
     // qualified. Otherwise trust what the user typed.
     if (!isFullyQualified(U->getQualifier()))
       AlwaysFullyQualify = false;
 
     if (SM.isBeforeInTranslationUnit(Inputs.Cursor, U->getUsingLoc()))
       // "Usings" is sorted, so we're done.
       break;
     if (U->getQualifier()->getAsNamespace()->getCanonicalDecl() ==
             QualifierToRemove.getNestedNameSpecifier()
                 ->getAsNamespace()
                 ->getCanonicalDecl() &&
         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;
     return Out;
   }
 
   // No relevant "using" statements. Try the nearest namespace level.
   const DeclContext *ParentDeclCtx =
       &Inputs.ASTSelection.commonAncestor()->getDeclContext();
   while (ParentDeclCtx && !ParentDeclCtx->isFileContext()) {
     ParentDeclCtx = ParentDeclCtx->getLexicalParent();
   }
   if (auto *ND = llvm::dyn_cast_or_null<NamespaceDecl>(ParentDeclCtx)) {
     auto Toks = Inputs.AST->getTokens().expandedTokens(ND->getSourceRange());
     const auto *Tok = llvm::find_if(Toks, [](const syntax::Token &Tok) {
       return Tok.kind() == tok::l_brace;
     });
     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";
       return Out;
     }
   }
   // 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(TLDs[0]->getBeginLoc());
+    Out.Loc = SM.getExpansionLoc(TLD->getBeginLoc());
     Out.Suffix = "\n\n";
     return Out;
   }
+  return error("Cannot find place to insert \"using\"");
+}
 
 bool isNamespaceForbidden(const Tweak::Selection &Inputs,
                           const NestedNameSpecifier &Namespace) {
   std::string NamespaceStr = printNamespaceScope(*Namespace.getAsNamespace());
 
   for (StringRef Banned : Config::current().Style.FullyQualifiedNamespaces) {
     StringRef PrefixMatch = NamespaceStr;
     if (PrefixMatch.consume_front(Banned) && PrefixMatch.consume_front("::"))
bool AddUsing::prepare(const Selection &Inputs) {
   }
   if (Node == nullptr)
     return false;
 
   if (auto *D = Node->ASTNode.get<DeclRefExpr>()) {
     if (auto *II = D->getDecl()->getIdentifier()) {
       QualifierToRemove = D->getQualifierLoc();
       Name = II->getName();
+      MustInsertAfterLoc = D->getDecl()->getBeginLoc();
     }
   } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
     if (auto E = T->getAs<ElaboratedTypeLoc>()) {
       QualifierToRemove = E.getQualifierLoc();
 
       auto SpelledTokens =
           TB.spelledForExpanded(TB.expandedTokens(E.getSourceRange()));
       if (!SpelledTokens)
         return false;
       auto SpelledRange = syntax::Token::range(SM, SpelledTokens->front(),
                                                SpelledTokens->back());
       Name = SpelledRange.text(SM);
 
       std::string QualifierToRemoveStr = getNNSLAsString(
           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<TypedefType>(ET->getNamedType().getTypePtr())) {
+          MustInsertAfterLoc = TDT->getDecl()->getBeginLoc();
+        } else if (auto *TD = ET->getAsTagDecl()) {
+          MustInsertAfterLoc = TD->getBeginLoc();
+        }
+      }
     }
   }
 
   // FIXME: This only supports removing qualifiers that are made up of just
   // namespace names. If qualifier contains a type, we could take the longest
   // namespace prefix and remove that.
   if (!QualifierToRemove.hasQualifier() ||
       !QualifierToRemove.getNestedNameSpecifier()->getAsNamespace() ||
std::string QualifierToRemoveStr = getNNSLAsString(
       QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
   tooling::Replacements R;
   if (auto Err = R.add(tooling::Replacement(
           SM, SM.getSpellingLoc(QualifierToRemove.getBeginLoc()),
           QualifierToRemoveStr.length(), ""))) {
     return std::move(Err);
   }
 
-  auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, Name);
+  auto InsertionPoint =
+      findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc);
   if (!InsertionPoint) {
     return InsertionPoint.takeError();
   }
 
   if (InsertionPoint->Loc.isValid()) {
     // Add the using statement at appropriate location.
     std::string UsingText;
     llvm::raw_string_ostream UsingTextStream(UsingText);

clang-tools-extra/clangd/unittests/TweakTests.cpp

 two::c^c C;
 })cpp",
              R"cpp(
 #include "test.hpp"
 using namespace one;
 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<std::string> EditedFiles;
   for (const auto &Case : Cases) {
     for (const auto &SubCase : expandCases(Case.TestSource)) {
       ExtraFiles["test.hpp"] = R"cpp(
 namespace one {
 void oo() {}
 namespace two {
 enum ee {};