[clangd] **Prototype**: clang-tidy-based tweaks

[hokein]

Summary

clang-tidy has already provided some simple refactoring-like checks (e.g. modernize-use-auto,
modernize-use-using), so we can leverage the power of clang-tidy to implement tweaks.

clang-tidy-based tweaks compared to clang-tidy check:

• For refactoring checks, tweaks maybe a better UI compared to showing as diagnostics (we only show on the UI when users do it intendedly)
• faster, we run the check on a single AST node rather than the whole AST

[hokein]

Update

[hokein]

This is my experiment of playing around tweaks, I think we'll need this.

[sammccall]

This is an intriguing idea, and is at least useful to prototype new tweaks.

I'm not sure whether clang-tidy is the ultimately right API to write tweaks:

• it doesn't have the needed constraints to ensure prepare() is fast (e.g. it emits diagnostics and fixes eagerly)
• the exact set of nodes that it will trigger on may or may not match what we want
• it doesn't produce context-sensitive titles

Maybe we should have this behind a flag, and use it to investigate which tweaks should be implemented natively?

clangd/refactor/tweaks/ClangTidy.cpp

45 ↗

(On Diff #185941)

I can't quite parse this.
"only checks whose matchers match exactly the selected node will work"?

One concern here is that this is basically an implementation detail of a check: checks can choose to e.g. match on a specific node and do a cheap analysis, or match on a higher-level node and do an expensive one. Maybe as long as the specific checks we want work well, it's not an issue. But every check added here needs to be carefully reviewed for performance.

66 ↗

(On Diff #185941)

obviously this is central to the approach, but... prepare needs to be *fast*. It's hard to see how we guarantee that with all this machinery, especially because the match callback contents may change over time.

83 ↗

(On Diff #185941)

looping over all the ancestors might give better results (but leaves less control over performance).

141 ↗

(On Diff #185941)

this should really specify which type is getting converted - seems like a possible weakness of this approach.

unittests/clangd/TweakTests.cpp

231 ↗

(On Diff #185941)

this needs to also trigger if you select "unsigned"

[hokein]

Update the comments

[hokein]

This is an intriguing idea, and is at least useful to prototype new tweaks.

I'm not sure whether clang-tidy is the ultimately right API to write tweaks:

it doesn't have the needed constraints to ensure prepare() is fast (e.g. it emits diagnostics and fixes eagerly)
the exact set of nodes that it will trigger on may or may not match what we want
it doesn't produce context-sensitive titles

Yeah, relying on the checks' implementations is the Achilles' heel of this approach. To reuse the existing code (create fast prototypes), we sacrifice our flexibility unfortunately :(

Another option would be creating a more general refactoring library, and refactoring-like clang-tidy and tweaks can be built on it.

Maybe we should have this behind a flag, and use it to investigate which tweaks should be implemented natively?

It seems to me there are no straight-forward ways to do it since the tweaks' implementations are isolated (maybe via a macro ENABLE_CLANG_TIDY_TWEAKS to control whether we should register these tweaks?).

clangd/refactor/tweaks/ClangTidy.cpp

66 ↗

(On Diff #185941)

Most of the checks' callback are just to detect whether the match result is the interesting case, and generate message & FixIt, I'd say it is cheap, and we only run clang-tidy check on the selected AST node, which should be fast.

83 ↗

(On Diff #185941)

Besides the performance concern, looping over ancestors would extend the clang-tidy check to run on other AST nodes, for the case below, we would also apply the tweak to the ast nodes that are not selected.

typedef int Foo
ty^pedef int Foo2;

141 ↗

(On Diff #185941)

Agree, we may use the check diagnostic messages (e.g. use auto when declaring iterators) as the title, but this depends on the check implementation, probably not an ideal way.

unittests/clangd/TweakTests.cpp

231 ↗

(On Diff #185941)

this would be nice to have, but it is a weak point of this approach -- relying on the check implementation.

[ilya-biryukov]

Another alternative we could consider would be collecting these fixes when producing the clang-tidy diagnostics.
We won't show the diagnostics to the users if they have the check disabled, but we would stash the ranges and the textual fixes somewhere.
I.e. I'm suggesting to reuse something closer to the mechanism of diagnostics fix-its, not the code tweak interfaces.

The advantages are:

1. keeping clang-tidy integration in one place,
2. no extra runs of ast matchers, all tidy checks will run during diagnostics.

WDYT?

[hokein]

Rebase the code, and hide the tweaks.

[hokein]

In D57943#1441129, @ilya-biryukov wrote:

Another alternative we could consider would be collecting these fixes when producing the clang-tidy diagnostics.
We won't show the diagnostics to the users if they have the check disabled, but we would stash the ranges and the textual fixes somewhere.
I.e. I'm suggesting to reuse something closer to the mechanism of diagnostics fix-its, not the code tweak interfaces.

The advantages are:

1. keeping clang-tidy integration in one place,
2. no extra runs of ast matchers, all tidy checks will run during diagnostics.

WDYT?

Hidden diagnostic is definitely another approach, but it'd require more discussion/work/design. As I mentioned before, it is used for experimental, and I'm not sure this is something we want.

I have rebased the branch, and hidden this under the hidden-features flag. Let me know if we want to move forward or not.

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

 # 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
   DumpAST.cpp
   RawStringLiteral.cpp
   SwapIfBranches.cpp
+  ClangTidy.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangDaemon
   clangToolingCore
   )

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

+//===--- ClangTidy.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 "ClangdUnit.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+
+#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../clang-tidy/modernize/UseAutoCheck.h"
+#include "../clang-tidy/modernize/UseUsingCheck.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) {
+  return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
+}
+
+struct StoreClangTidyDiags : public DiagnosticConsumer {
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                        const clang::Diagnostic &Info) override {
+    assert(Info.hasSourceManager() && "Diagnostic must have SourceManager");
+    if (Info.getNumFixItHints() > 0)
+      DiagsWithFixIt.push_back(Info);
+  }
+  std::vector<clang::Diagnostic> DiagsWithFixIt;
+};
+
+/// An **experimental** interface for clang-tidy-based Tweaks, which is used to
+/// prototype new tweaks (without reinventing the wheel).
+/// The tweak will run a clang-tidy check over on an AST node under the cursor.
+/// It is designed for simple refactoring-like clang-tidy checks.
+///
+/// The tweak relies solely on the check's implementation, we don't have much
+/// flexibility:
+///   - only checks whose matcheres match exactly the selected node will work
+///   - every check being added needs to be carefully reviewed to meet the
+///     **fast** constraint of `prepare`
+///   - no context-sensitive titles are supported
+///   - checks don't see any preprocessor events
+///   - no check options are supported yet
+class ClangTidyTweak : public Tweak {
+public:
+  ClangTidyTweak()
+      : DiagsEng(new clang::DiagnosticIDs(), new clang::DiagnosticOptions(),
+                 &ClangTidyDiags, /*ShouldOwnClient=*/false) {}
+
+  virtual std::unique_ptr<tidy::ClangTidyCheck>
+  createCheck(tidy::ClangTidyContext *) = 0;
+
+  bool prepare(const Selection &Inputs) override;
+
+  Expected<Effect> apply(const Selection &Inputs) override;
+
+  // For experimental only!
+  bool hidden() const override { return true; }
+
+  Intent intent() const override { return Refactor; }
+
+protected:
+  StoreClangTidyDiags ClangTidyDiags;
+  DiagnosticsEngine DiagsEng; // Extend the lifetime of Diagnostics.
+};
+
+bool ClangTidyTweak::prepare(const Selection &Inputs) {
+  if (!Inputs.ASTSelection.commonAncestor())
+    return false;
+  if (Inputs.AST.getASTContext().getDiagnostics().hasErrorOccurred())
+    return false;
+  DiagsEng.setSourceManager(&Inputs.AST.getASTContext().getSourceManager());
+  tidy::ClangTidyContext CTContext(
+      llvm::make_unique<tidy::DefaultOptionsProvider>(
+          tidy::ClangTidyGlobalOptions(),
+          tidy::ClangTidyOptions::getDefaults()));
+  CTContext.setDiagnosticsEngine(&DiagsEng);
+  CTContext.setASTContext(&Inputs.AST.getASTContext());
+
+  auto Check = createCheck(&CTContext);
+  ast_matchers::MatchFinder CTFinder;
+  Check->registerMatchers(&CTFinder);
+  // Run the check on the AST node under the cursor.
+  CTFinder.match(Inputs.ASTSelection.commonAncestor()->ASTNode,
+                 Inputs.AST.getASTContext());
+  return !ClangTidyDiags.DiagsWithFixIt.empty();
+}
+
+Expected<ClangTidyTweak::Effect>
+ClangTidyTweak::apply(const Selection & /*Inputs*/) {
+  tooling::Replacements FixReplacements;
+  for (const auto &Diag : ClangTidyDiags.DiagsWithFixIt) {
+    for (auto &FixIt : Diag.getFixItHints()) {
+      if (!isInsideMainFile(FixIt.RemoveRange.getBegin(),
+                            Diag.getSourceManager()))
+        continue;
+      tooling::Replacement Fix(Diag.getSourceManager(), FixIt.RemoveRange,
+                               FixIt.CodeToInsert);
+
+      llvm::Error Err = FixReplacements.add(Fix);
+      if (Err)
+        return std::move(Err);
+    }
+  }
+  return Effect::applyEdit(FixReplacements);
+}
+
+/// Convert typedef to using.
+/// Before:
+///   typedef int Foo;
+///   ^^^^^^^     ^^^
+/// After:
+///   using Foo = int;
+class ConvertTypedef : public ClangTidyTweak {
+public:
+  const char *id() const override final;
+
+  std::unique_ptr<tidy::ClangTidyCheck>
+  createCheck(tidy::ClangTidyContext *Context) override {
+    return llvm::make_unique<tidy::modernize::UseUsingCheck>(
+        "modernize-use-using", Context);
+  }
+
+  std::string title() const override { return "Convert typedef to using"; }
+};
+
+/// Use auto.
+/// Before:
+///  [[std::vector<int>::iterator I = my_container.begin();]]
+/// After
+///  auto I = my_container.begin();
+class UseAuto : public ClangTidyTweak {
+public:
+  const char *id() const override final;
+
+  std::unique_ptr<tidy::ClangTidyCheck>
+  createCheck(tidy::ClangTidyContext *Context) override {
+    return llvm::make_unique<tidy::modernize::UseAutoCheck>(
+        "modernize-use-auto", Context);
+  }
+
+  std::string title() const override { return "Covert type to auto"; }
+};
+
+REGISTER_TWEAK(ConvertTypedef)
+REGISTER_TWEAK(UseAuto)
+
+} // namespace
+} // namespace clangd
+} // namespace clang

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

TEST(TweakTest, DumpRecordLayout) {
   checkNotAvailable(ID, "struct ^X;");
   checkNotAvailable(ID, "template <typename T> struct ^X { T t; };");
   checkNotAvailable(ID, "enum ^X {};");
 
   const char *Input = "struct ^X { int x; int y; }";
   EXPECT_THAT(getMessage(ID, Input), ::testing::HasSubstr("0 |   int x"));
 }
 
+TEST(TweakTest, ConvertTypedef) {
+  llvm::StringLiteral ID = "ConvertTypedef";
+  checkAvailable(ID, R"cpp(^t^y^p^e^d^e^f^ int ^F^o^o;)cpp");
+  llvm::StringLiteral Input = "^typedef int Foo;";
+  llvm::StringLiteral Output = "using Foo = int;";
+  checkTransform(ID, Input, Output);
+}
+
+TEST(TweakTest, UseAuto) {
+  llvm::StringLiteral ID = "UseAuto";
+  llvm::StringLiteral Input =
+      "void f() { [[unsigned c = static_cast<unsigned>(1);]] }";
+  llvm::StringLiteral Output =
+      "void f() { auto c = static_cast<unsigned>(1); }";
+  checkTransform(ID, Input, Output);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang