[clangd] Add a tweak for adding "using" statement.

[adamcz]

Summary

This triggers on types and function calls with namespace qualifiers. The
action is to remove the qualifier and instead add a "using" statement at
appropriate place.

It is not always clear where to add the "using" line. Right now we find
the nearest "using" line and add it there, thus keeping with local
convention. If there are no usings, we put it at the deepest relevant
namespace level.

This is an initial version only. There are several improvements that
can be made:

• Support for qualifiers that are not purely namespace (e.g. record

types, etc).

• Removing qualifier from other instances of the same type/call.
• Smarter placement of the "using" line.

[sammccall]

This seems really well-thought-out.
I'm being (even) more verbose than usual about the interesting AST details. Please do push back/defer fixing anything that adds a lot of complexity and doesn't seem important.

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

21

function calls is obsolete I think, I'd just call these references or DeclRefExpr

23

You could put some general comments about the scope/choices here (applicable only to namespaces, insertion strategy)

23

Removing qualifier from other instances of the same type/call.

I think this is valuable enough to be worth calling out in the top-level comment as not-yet-done.
The other extensions you mention seem like we might never get around to them and it'd be fine.

35

is this redundant with Loc.isValid()?

If so, I'd either just use Loc (with a comment), or use optional<SourceLocation> with a comment, to emphasize the relationship.

38

can we get away with always inserting \n and relying on clang-format to clean it up?

45

this doesn't use any state - make it static or a free function?

49

nit: I quite like the abbreviated names for locals but this deserves a real name like Qualifier or QualifierToRemove.

55

Can we make this dynamic? The more clearly we communicate what is targeted, the more confidence the user will have to use it.

title() may only be called after a successful prepare(), so we can use the prepared state.

Maybe something like Add using-declaration for Bar, and remove qualifier?

57

nit: not actually a statement :-(

If "using-declaration" is overly laywerly (and easily confused with using-directive), then maybe we should just spell it out: Add "using foo::Bar" ...

69

no need for the first two conditions, getFileID will give you an invalid fileid and a macro fileid respectively

72

nit: we conventionally leave out the {} on one-line if bodies etc.

73

This check is sufficient for correctness, but we're still going to traverse all the parts of the AST that can't pass this check.

I think you can override TraverseDecl. If the Decl is a DeclContext and it doesn't enclose the selectiondc, then return (pruning that subtree). Otherwise call RecursiveASTVisitor::TraverseDecl and continue as before.

(I think you still want this check though - pruning decls is enough for performance but may not be for correctness)

90

I like the idea here, but I'm not sure it quite works. e.g. any typeloc has a directly enclosing typeloc, the inner one can't be targeted. So what about x::S*? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?

• looping up from NNS until you get to something else makes sense
• unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think you ever want to unwrap multiple levels?
• i don't think these cases overlap at all, you want one or the other

Am I missing something?

124

I think you also want to check that name is non-null, which would be the case for non-identifier names like foo::operator+(...).

133

One possible hiccup is insertion splitting a comment from its target. Cases I can think of:

• namespace { // anonymous -> namespace { using foo::bar; // anonymous. This seems rare/unimportant.
• /* docs */ int firstTopLevelDecl -> /* docs */ using foo::bar; int firstTopLevelDecl. This seems common/bad.
• /* for widgets */using foo::Qux; -> /* for widgets */ using foo::bar; using foo::Qux. This seems rare/unimportant.

I think you could handle the decl case by calling ASTContext::getLocalCommentForDeclUncached() and using the getBeginLoc() of the returned comment if any.

That said, this is probably something that we should be doing throughout clangd and we should have some common function in AST.h or so to take care of it. It seems fine to leave a comment and we can think about this later. (Or file a bug where we can dump a list of places that should be accounting for the comment range)

145

This is basically always going to be already sorted. AST traversal isn't strictly left-to-right but pretty close (I think the major exceptions are weird objc things). So you could leave this out.

149

ah, neat :-)
but you could check this in the recursive AST visitor instead, and use it to prune the tree.

151

Hmm, I *think* it's probably true that NNS->getAsNamespace() always refers to the canonical (first) NamespaceDecl. If not, you can getCanonicalDecl() on both to ensure this.

162

the tweak infrastructure should do this in any case, it runs clang-format around changed areas (assuming using-decl sorting is on, which it is in the default styles).

do you not see this behaviour? (unit-tests won't exhibit it as the clang-format step is deliberately excluded from them)

207

(Sorry if some of this is obvious: TL;DR: we should use TokenBuffer to handle macros in this case)

Whenever we try to use SourceLocations to reason about written source code, we have to think about macros. Some libraries try to encapsulate this, but the road to confusion is paved with good intentions, and it's often best to decide on the policy and be explicit about it.

For example, when provided a macro location, the tooling::Replacement constructor uses its spelling location.
Given:

// imagine we're trying to abstract away namespaces for C or something
#define NS(X) foo::X
NS(Bar) boo;

Running this action on N would result in changing the definition of the NS macro to delete the "foo::", as that's where the qualifier was spelled!

It would be reasonable (but actually not trivial!) to try to detect any macro usage and bail out. The general case of "is there some sequence of source-code tokens that correspond to this range of preprocessed tokens and nothing else" is pretty hard.

However TokenBuffer does have APIs for this exact question, as it was designed for writing refactorings that replace nodes. I think you want:

• expandedTokens(NNSL.getSourceRange())
• spelledForExpanded(ExpandedTokens)
• Token::range(SM, Spelled.front(), Spelled.back())
• Replacement("fname", Range.Offset, Range.Length, "")

(ugh, that's really awkward. Maybe we should have a helper in TokenBuffer that combines 1-3)

208

BTW, this is an instance of a common pattern where we have the location of the start of a token (in this case, the last token in the range) and want the token bounds, and end up running the lexer in raw mode to get it! The giveaway is LangOpts which it needs it order to lex.

This is probably not terrible here but it's fiddly and inefficient (when done everywhere) and gets things wrong sometimes. TokenBuffer stores the expanded token stream, the spelled tokens from the main file, and the correspondence between the two, and should generally be used where possible. It's stored in the ParsedAST.

227

Again macros.
This seems a bit more pressing, there's a nontrivial chance that you're inserting inside a namespace which is introduced by invoking a FOO_NS_BEGIN macro.

Unfortunately I don't see a really simple way to insert after this macro - using the expansion location fails if macro doesn't end after the }. (e.g. ANON_NAMESPACE(f::X y;))
(Also you'd need to take care of inserting a space after the macro invocation to avoid FOO_NS_BEGINusing.)

I'd suggest simply skipping over any candidate locations (namespaces or using declarations) that aren't in the main file, using the expansion location of the first-top-level-decl fallback, and asserting here that the location is in the main file. (We use raw assert() in LLVM)

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

2436

alternately: using ::one::two::ff.
Correct more often, uglier/longer, probably a style preference but not worth having an option for. Pick one :-)

[adamcz]

Addressed review comments

[adamcz]

clang-format

[adamcz]

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

35

Yes, it's redundant. I went back and forth on this one, original version used Loc.isValid(). I decided I like being explicit better than having slightly more complicated semantics of the Loc field, but I wasn't too confident about this. Having second opinion helps :-)

I switched to isValid()

38

Nope, that doesn't work. Auto-formatting doesn't seem to remove blank lines between using statements, for example. It's likely to allow you grouping them in some way.

45

It uses Name and NNSL . Would you prefer this as free function, with both passed as arguments. My initial thought was that, since prepare() and apply() already share these via class members this was fine, but I'm certainly not against making this a free function.

57

I think spelling it out may be too long sometimes, namespaces are often long.

Changed to using-declaration

72

Uhh...is that a hard rule? I personally hate that, it's just asking for Apple SSL-style bugs
https://www.imperialviolet.org/2014/02/22/applebug.html

90

If you have a class foo::bar::cc and a struct st inside it, and then do:
foo::bar::c^c::st *p;
you end up with:
PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> RecordTypeLoc
in which case you need to go up from type to NNSL and then up again, from NNSL to something that's not NNSL.

You have a good point with the PointerTypeLoc, that's a bug. We should stop going up the tree as soon as we find ElaboratedTypeLoc. I added a test for that.

133

Added a comment. I'd be happy to fix that, but I feel it's best done in a separate change.

145

Done. I also added a comment to mention that we assume it's sorted.

162

Didn't know that was the default. I'll drop the comment in that case.

207

You have a good point that this doesn't work well with macros, but I'm not entirely sure how you think this should work.

I'd argue that code actions should refer to the thing under the cursor, not it's expansion. That would be confusing to the user, as they would not be able to understand what the action offered is, nor how it would affect other places. So in your example of
#define NS(X) foo::X
I'd argue that we should not offer the action.
We should, however, support something like:
EXPECT_TRUE(fo^o::bar());
In this case, the qualifier is spelled under the cursor and the fact that EXPECT_TRUE is a macro and not a function should not matter to the user.

I updated the code to support that and added tests.
We can use isMacroID() and isMacroArgExpansion() to filter out macros, except for macro arguments. Note that we can't use spelledForExpanded(), since the qualifier might not be the entire expansion of the macro, thus spelledForExpanded will return None.

Let me know if any of what I did here now doesn't make sense.

227

We already skip over "using" not in main file. I added check for namespace and fixed the above-first-top-level-decl to be above getExpandedLoc() of that, which works for something like a macro declaring a namespace. It's not ideal, but I think it should be good enough, at least for now. Also added a test.

Not sure what the point of assert() here would be? Wouldn't it be far better to return an error, since it's trivial at this point and it won't crash the whole server, including the background index and such? I get that assert() can be useful in places where returning error is difficult and for things that should never, ever happen, but here seems like an overkill.

Anyway, editMainFile() already returns an error when the edit is not in the main file. IMHO that's good enough, what do you think?

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

2436

The current code keeps the :: if it was there, but won't add it, so basically we respect whatever the user typed. Seems OK to me, do you agree?

I changed one test to include this aspect.

[sammccall]

LG, thanks! Let me know if/when I should land this for you.

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

45

Oops, right. I think since this is the complicated part and it has a fairly simple interface, making its inputs/outputs explicit is nice hygiene.

(prepare and apply do communicate through class state, which feels a bit hacky to me. We didn't come up with anything nicer but still flexible+simple when designing this. It is conventional and documented at least)

72

There's no hard rule, but we do use that style consistently and consistency has value too.

The clang-tidy check readability-misleading-indentation catches that bug. Can I suggest:

• (for us) adding it to .clang-tidy in llvm-project? This will affect clangd and the phabricator linter
• (for the world) we could start a list of on-by-default clang-tidy checks for clangd users with no .clang-tidy config, and include this check

90

foo::bar::c^c::st *p;

But this isn't a case we support, right? We only support extraction when the NNS consists entirely of namespaces, and such NNSes don't have any children apart from the qualifier NNS.

105

This is just SourceManager::isMacroBodyExpansion(), I think.

163

I think the endloc check should rather be SM.isWrittenInSameFile - it's no good if e.g. they're both macro arg expansions, but different ones.

171

nit: you've mentioned the two cases I don't think we should bother fixing, but not the crucial one: we're anchoring to a regular decl, and we're going to insert in between the decl and its doc comment.

207

in your example of #define NS(X) foo::X I'd argue that we should not offer the action.

Indeed, sorry I didn't spell that out - I was highlighting it because the original code silently did something (bad) in this case.

can't use spelledForExpanded(), since the qualifier might not be the entire expansion of the macro

Ah, D75446 hasn't landed yet. @kadircet, want to land this soon? :-)

227

Behaviour looks good.

Not sure what the point of assert() here would be?

This is a consistency check - we're relying on an invariant we think we've established elsewhere. If this goes wrong, it's a programmer error, and for better or worse, LLVM defines those as asserts.

Practical benefits:

• developers are more likely to notice when these are violated (assuming an -UNDEBUG build)
• we get a stacktrace, which is often useful (not really here). Returning an error doesn't give us that.
• we don't pay for the check in release builds
• it documents the distinction between invariant-breaking bugs and dynamically-possible error conditions
• consistency (in principle we could change all our err-handling, but we can't change this in clang)

245

yikes, this copies the tokenbuffer, I didn't think that was even possible!
auto&

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

2436

Actually yeah, that's nice and neatly avoids having to make and defend a policy :-)

[sammccall]

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

245

Copy constructor deleted in 94938d7d41cd11c4539ff93b801fe53cb4fddba2

[kadircet]

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

207

done, let me know if it doesn't work :D

249

why not use spelledForExpanded in here as well?

[adamcz]

review round 2

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

90

Not right now, but I left a comment that we should improve that. The way it is now, code is correct, always, and improving it is just a matter or removing the "no record types" check and replacing with a bit more code. Seems better to me.

207

Works great, thanks!

227

OK, I added the assert(). I'm not against asserts in general, I just find them often insufficient and not a replacement for better error handling. In this case, it certainly won't hurt.

245

Haha, that's a lovely bug. Thanks for fixing the ctor.

249

It didn't work before. Thanks for improving it!

[sammccall]

Still LG, should I land this?

[adamcz]

In D76432#1952798, @sammccall wrote:

Still LG, should I land this?

Yes please :)

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

+//===--- AddUsing.cpp --------------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AST.h"
+#include "FindTarget.h"
+#include "Logger.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// Tweak for removing full namespace qualifier under cursor on DeclRefExpr and
+// types and adding "using" statement instead.
+//
+// Only qualifiers that refer exclusively to namespaces (no record types) are
+// supported. There is some guessing of appropriate place to insert the using
+// declaration. If we find any existing usings, we insert it there. If not, we
+// insert right after the inner-most relevant namespace declaration. If there is
+// none, or there is, but it was declared via macro, we insert above the first
+// top level decl.
+//
+// Currently this only removes qualifier from under the cursor. In the future,
+// we should improve this to remove qualifier from all occurences of this
+// symbol.
+class AddUsing : public Tweak {
+public:
+  const char *id() const override;
+
+  bool prepare(const Selection &Inputs) override;
+  Expected<Effect> apply(const Selection &Inputs) override;
+  std::string title() const override;
+  Intent intent() const override { return Refactor; }
+
+private:
+  struct InsertionPointData {
+    // Location to insert the "using" statement. If invalid then the statement
+    // should not be inserted at all (it already exists).
+    SourceLocation Loc;
+    // Extra suffix to place after the "using" statement. Depending on what the
+    // insertion point is anchored to, we may need one or more \n to ensure
+    // proper formatting.
+    std::string Suffix;
+  };
+  // Finds the best place to insert the "using" statement. Returns invalid
+  // SourceLocation if the "using" statement already exists.
+  llvm::Expected<InsertionPointData>
+  findInsertionPoint(const Selection &Inputs);
+
+  // The qualifier to remove. Set by prepare().
+  NestedNameSpecifierLoc QualifierToRemove;
+  // The name following QualifierToRemove. Set by prepare().
+  llvm::StringRef Name;
+};
+REGISTER_TWEAK(AddUsing)
+
+std::string AddUsing::title() const {
+  return std::string(llvm::formatv(
+      "Add using-declaration for {0} and remove qualifier.", Name));
+}
+
+// Locates all "using" statements relevant to SelectionDeclContext.
+class UsingFinder : public RecursiveASTVisitor<UsingFinder> {
+public:
+  UsingFinder(std::vector<const UsingDecl *> &Results,
+              const DeclContext *SelectionDeclContext, const SourceManager &SM)
+      : Results(Results), SelectionDeclContext(SelectionDeclContext), SM(SM) {}
+
+  bool VisitUsingDecl(UsingDecl *D) {
+    auto Loc = D->getUsingLoc();
+    if (SM.getFileID(Loc) != SM.getMainFileID()) {
+      return true;
+    }
+    if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
+      Results.push_back(D);
+    }
+    return true;
+  }
+
+  bool TraverseDecl(Decl *Node) {
+    // There is no need to go deeper into nodes that do not enclose selection,
+    // since "using" there will not affect selection, nor would it make a good
+    // insertion point.
+    if (Node->getDeclContext()->Encloses(SelectionDeclContext)) {
+      return RecursiveASTVisitor<UsingFinder>::TraverseDecl(Node);
+    }
+    return true;
+  }
+
+private:
+  std::vector<const UsingDecl *> &Results;
+  const DeclContext *SelectionDeclContext;
+  const SourceManager &SM;
+};
+
+// Returns true iff Loc is in a macro and is not the expansion of the macro
+// argument.
+bool isInMacroNotArg(const SourceManager &SM, const SourceLocation Loc) {
+  return Loc.isMacroID() && !SM.isMacroArgExpansion(Loc);
+}
+
+bool AddUsing::prepare(const Selection &Inputs) {
+  auto &SM = Inputs.AST->getSourceManager();
+  auto *Node = Inputs.ASTSelection.commonAncestor();
+  if (Node == nullptr)
+    return false;
+
+  // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+  // we find the "main" node we care about, which would be ElaboratedTypeLoc or
+  // DeclRefExpr.
+  for (; Node->Parent; Node = Node->Parent) {
+    if (Node->ASTNode.get<NestedNameSpecifierLoc>()) {
+      continue;
+    } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
+      if (T->getAs<ElaboratedTypeLoc>()) {
+        break;
+      } else if (Node->Parent->ASTNode.get<TypeLoc>() ||
+                 Node->Parent->ASTNode.get<NestedNameSpecifierLoc>()) {
+        // Node is TypeLoc, but it's parent is either TypeLoc or
+        // NestedNameSpecifier. In both cases, we want to go up, to find
+        // the outermost TypeLoc.
+        continue;
+      }
+    }
+    break;
+  }
+  if (Node == nullptr)
+    return false;
+
+  if (auto *D = Node->ASTNode.get<DeclRefExpr>()) {
+    QualifierToRemove = D->getQualifierLoc();
+    Name = D->getDecl()->getName();
+  } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
+    if (auto E = T->getAs<ElaboratedTypeLoc>()) {
+      QualifierToRemove = E.getQualifierLoc();
+      Name =
+          E.getType().getUnqualifiedType().getBaseTypeIdentifier()->getName();
+    }
+  }
+
+  // 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() ||
+      Name.empty()) {
+    return false;
+  }
+
+  // Macros are difficult. We only want to offer code action when what's spelled
+  // under the cursor is a namespace qualifier. If it's a macro that expands to
+  // a qualifier, user would not know what code action will actually change.
+  // On the other hand, if the qualifier is part of the macro argument, we
+  // should still support that.
+  if (isInMacroNotArg(SM, QualifierToRemove.getBeginLoc()) ||
+      isInMacroNotArg(SM, QualifierToRemove.getEndLoc())) {
+    return false;
+  }
+
+  return true;
+}
+
+// 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 using, or
+// immediately following "namespace {". We should add some helpers for dealing
+// with that and use them in other code modifications as well.
+llvm::Expected<AddUsing::InsertionPointData>
+AddUsing::findInsertionPoint(const Selection &Inputs) {
+  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());
+
+  for (auto &U : Usings) {
+    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()) {
+    InsertionPointData Out;
+    Out.Loc = LastUsingLoc;
+    return Out;
+  }
+
+  // No relevant "using" statements. Try the nearest namespace level.
+  const auto *NS = Inputs.ASTSelection.commonAncestor()
+                       ->getDeclContext()
+                       .getEnclosingNamespaceContext();
+  if (auto *ND = dyn_cast<NamespaceDecl>(NS)) {
+    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 llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Namespace with no {");
+    }
+    if (!Tok->endLocation().isMacroID()) {
+      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.
+  auto TLDs = Inputs.AST->getLocalTopLevelDecls();
+  if (TLDs.empty()) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Cannot find place to insert \"using\"");
+  }
+  InsertionPointData Out;
+  Out.Loc = SM.getExpansionLoc(TLDs[0]->getBeginLoc());
+  Out.Suffix = "\n\n";
+  return Out;
+}
+
+Expected<Tweak::Effect> AddUsing::apply(const Selection &Inputs) {
+  auto &SM = Inputs.AST->getSourceManager();
+  auto TB = Inputs.AST->getTokens();
+  // Determine the length of the qualifier under the cursor, then remove it.
+  auto ExpandedTokens = TB.expandedTokens(QualifierToRemove.getSourceRange());
+  const auto *SpelledBeginTok =
+      TB.spelledTokenAt(SM.getSpellingLoc(ExpandedTokens.front().location()));
+  const auto *SpelledEndTok =
+      TB.spelledTokenAt(SM.getSpellingLoc(ExpandedTokens.back().location()));
+  unsigned Length =
+      syntax::Token::range(SM, *SpelledBeginTok, *SpelledEndTok).length();
+  tooling::Replacements R;
+  if (auto Err = R.add(
+          tooling::Replacement(SM, SpelledBeginTok->location(), Length, ""))) {
+    return std::move(Err);
+  }
+
+  auto InsertionPoint = findInsertionPoint(Inputs);
+  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);
+    UsingTextStream << "using ";
+    QualifierToRemove.getNestedNameSpecifier()->print(
+        UsingTextStream, Inputs.AST->getASTContext().getPrintingPolicy());
+    UsingTextStream << Name << ";" << InsertionPoint->Suffix;
+
+    if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
+                                              UsingTextStream.str()))) {
+      return std::move(Err);
+    }
+  }
+
+  return Effect::mainFileEdit(Inputs.AST->getASTContext().getSourceManager(),
+                              std::move(R));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt

 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
 
 set(LLVM_LINK_COMPONENTS
   support
   )
 
 # A target containing all code tweaks (i.e. mini-refactorings) provided by
 # clangd.
 # Built as an object library to make sure the linker does not remove global
 # constructors that register individual tweaks in a global registry.
 # To enable these tweaks in executables or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
+  AddUsing.cpp
   AnnotateHighlightings.cpp
   DumpAST.cpp
   DefineInline.cpp
   DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp
   ExtractVariable.cpp

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

std::pair<StringRef, StringRef> Cases[] = {
           "fail: define outline: Can't move out of line as function has a "
           "macro `override` specifier.\ndefine outline: Can't move out of line "
           "as function has a macro `final` specifier."},
   };
   for (const auto &Case : Cases) {
     EXPECT_EQ(apply(Case.first), Case.second);
   }
 }
+
+TWEAK_TEST(AddUsing);
+TEST_F(AddUsingTest, Prepare) {
+  const std::string Header = R"cpp(
+#define NS(name) one::two::name
+namespace one {
+void oo() {}
+namespace two {
+enum ee {};
+void ff() {}
+class cc {
+public:
+  struct st {};
+  static void mm() {}
+};
+}
+})cpp";
+
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^f^f(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^::^o^o(); }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o^:^:^e^e E; }");
+  EXPECT_AVAILABLE(Header + "void fun() { o^n^e^:^:^t^w^o:^:^c^c C; }");
+  EXPECT_UNAVAILABLE(Header +
+                     "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^m^m(); }");
+  EXPECT_UNAVAILABLE(Header +
+                     "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header +
+                     "void fun() { o^n^e^:^:^t^w^o^:^:^c^c^:^:^s^t inst; }");
+  EXPECT_UNAVAILABLE(Header + "void fun() { N^S(c^c) inst; }");
+}
+
+TEST_F(AddUsingTest, Apply) {
+  FileName = "test.cpp";
+  struct {
+    llvm::StringRef TestSource;
+    llvm::StringRef ExpectedSource;
+  } Cases[]{{
+                // Function, no other using, namespace.
+                R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ^o^n^e^:^:^t^w^o^:^:^f^f();
+}
+})cpp",
+                R"cpp(
+#include "test.hpp"
+namespace {using one::two::ff;
+
+void fun() {
+  ff();
+}
+})cpp",
+            },
+            // Type, no other using, namespace.
+            {
+                R"cpp(
+#include "test.hpp"
+namespace {
+void fun() {
+  ::on^e::t^wo::c^c inst;
+}
+})cpp",
+                R"cpp(
+#include "test.hpp"
+namespace {using ::one::two::cc;
+
+void fun() {
+  cc inst;
+}
+})cpp",
+            },
+            // Type, no other using, no namespace.
+            {
+                R"cpp(
+#include "test.hpp"
+
+void fun() {
+  on^e::t^wo::e^e inst;
+})cpp",
+                R"cpp(
+#include "test.hpp"
+
+using one::two::ee;
+
+void fun() {
+  ee inst;
+})cpp"},
+            // Function, other usings.
+            {
+                R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ee;
+
+namespace {
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+                R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+using one::two::ff;using one::two::ee;
+
+namespace {
+void fun() {
+  ff();
+}
+})cpp",
+            },
+            // Function, other usings inside namespace.
+            {
+                R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::two::ff;
+
+void fun() {
+  o^ne::o^o();
+}
+})cpp",
+                R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+namespace {
+
+using one::oo;using one::two::ff;
+
+void fun() {
+  oo();
+}
+})cpp"},
+            // Using comes after cursor.
+            {
+                R"cpp(
+#include "test.hpp"
+
+namespace {
+
+void fun() {
+  one::t^wo::ff();
+}
+
+using one::two::cc;
+
+})cpp",
+                R"cpp(
+#include "test.hpp"
+
+namespace {using one::two::ff;
+
+
+void fun() {
+  ff();
+}
+
+using one::two::cc;
+
+})cpp"},
+            // Pointer type.
+            {R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::two::c^c *p;
+})cpp",
+             R"cpp(
+#include "test.hpp"
+
+using one::two::cc;
+
+void fun() {
+  cc *p;
+})cpp"},
+            // Namespace declared via macro.
+            {R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+NS_BEGIN(foo)
+
+void fun() {
+  one::two::f^f();
+}
+})cpp",
+             R"cpp(
+#include "test.hpp"
+#define NS_BEGIN(name) namespace name {
+
+using one::two::ff;
+
+NS_BEGIN(foo)
+
+void fun() {
+  ff();
+}
+})cpp"},
+            // Inside macro argument.
+            {R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+void fun() {
+  CALL(one::t^wo::ff);
+})cpp",
+             R"cpp(
+#include "test.hpp"
+#define CALL(name) name()
+
+using one::two::ff;
+
+void fun() {
+  CALL(ff);
+})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 {};
+void ff() {}
+class cc {
+public:
+  struct st { struct nested {}; };
+  static void mm() {}
+};
+}
+})cpp";
+      EXPECT_EQ(apply(SubCase, &EditedFiles), Case.ExpectedSource);
+    }
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang