[clangd] Add code action to generate a constructor for a C++ class

[njames93]

Shouldn't the constructors that need std::move also ensure the <utility> header is also included, or at least transitively included. We don't want to generate constructors that wont compile.

[njames93]

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

204

If C is default constructible, would it be nice to skip here instead of failing?

212

Some codebases may not want these to be explicit, would it be wiser to use config to control this?

[avogelsgesang]

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

27–29

// e.g. given `struct S{ int x; unique_ptr<double> y; };`, produces:
//   struct S {
//     int x; unique_ptr<double> y;
//     S(int x, unique_ptr<double> y) : x(x), y(std::move(y)) {}
//   };

or just

// e.g. given `struct S{ int x; unique_ptr<double> y; };`, produces:
//     S(int x, unique_ptr<double> y) : x(x), y(std::move(y)) {}

The tweak does not remove the members, as currently suggested by the comment

50

do we also need to exclude anonymous class declarations here? (Not sure if those are also modelled as CXXRecordDecl in the clang AST...)

134

Is this visitor able to look through using MyType = int; and typedef?
I think we should also add a test case

[njames93]

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

27–29

That's just a bad comment, the tweak won't remove the members

50

Good point, should also ensure there is a test case for this as well.

[avogelsgesang]

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

50

On 2nd thought: To trigger this tweak, I click on the class name, and since anonymous structs don't have class names, I think the tweak can't even be triggered.

Still probably worth a check here, just to be sure. Or at least an assert+ comment in the code

[avogelsgesang]

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

27–29

I know. Let's fix the comment :)

[avogelsgesang]

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

39

Define constructor (first letter uppercase)
At least this seems to be the convention for most other tweaks.

[avogelsgesang]

Can we add this tweak as the default "fix-it" action for

Class '...' does not declare any constructor to initialize its non-modifiable members

on structs like

struct X {
   int& a;
}

?

[sammccall]

address comments
(sorry for long turnaround)

[sammccall]

In D116514#3311780, @avogelsgesang wrote:

Can we add this tweak as the default "fix-it" action for

Class '...' does not declare any constructor to initialize its non-modifiable members

on structs like

struct X {
   int& a;
}

?

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.
It's a useful idea but needs a little design I guess.
(-Wswitch kind of works by always marking the tweak as a "fix", which semantically doesn't work here)

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

50

Good catch, it's also possible to trigger the code action on the class's braces.
Excluded and added test.

134

We call getCanonicalType() before visiting.

204

Yeah, if it's default constructible but *not movable* then it's probably reasonable.

(The default constructor should be public, but actually this applies to all constructors, so I added this condition everywhere)

212

I'm not sure that adding an option solves a problem, so I'd rather not add one yet.

In most cases, the programmer should be deciding whether the constructor is explicit or not on a case-by-case basis. Having a configurable adds complexity without relieving significant burden.

We do have to have some default, and explicit for single-arg constructors is IMO a better default (recommended by cppcoreguidelines, google style guide)

[avogelsgesang]

LGTM (for whatever that's worth - still pretty unexperienced with the clang AST)

Thanks for picking this up again! Looking forward to have this available in clangd! :)

[avogelsgesang]

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.

Is this a clangd-specific implementation issue or a general short-coming in the LSP protocol?

[sammccall]

Thanks!

In D116514#3423420, @avogelsgesang wrote:

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.

Is this a clangd-specific implementation issue or a general short-coming in the LSP protocol?

It's a clangd thing, tweaks and diagnostics just don't know about each other at all (diagnostics come fully-formed out of the clang internals, tweaks are an isolated traversal of the AST (possibly a different version).

We can't really change the way the diagnostics are generated, so we'd have to store them and do some pattern matching.

(Clangd does have many fixes that are tightly integrated with diagnostics, these are implemented inside clang. Something as big as generating a constructor is out of scope for clang)

[sammccall]

In D116514#3230255, @njames93 wrote:

Shouldn't the constructors that need std::move also ensure the <utility> header is also included, or at least transitively included. We don't want to generate constructors that wont compile.

I've added this just as a FIXME for now, because we're missing "framework" support for adding includes and composing separate edits in tweaks.
I think this is probably only a small obstacle in practice, it seems pretty rare to write std::move and need to add the #include for it to compile.

clang-tools-extra/clangd/AST.h

 /// macro (e.g. a change in definition offset can result in a different USR). We
 /// could change these semantics in the future by reimplementing this funcure
 /// (e.g. avoid USR for macros).
 SymbolID getSymbolID(const llvm::StringRef MacroName, const MacroInfo *MI,
                      const SourceManager &SM);
 
 /// Returns a QualType as string. The result doesn't contain unwritten scopes
 /// like anonymous/inline namespace.
-std::string printType(const QualType QT, const DeclContext &CurContext);
+std::string printType(const QualType QT, const DeclContext &CurContext,
+                      llvm::StringRef Placeholder = "");
 
 /// Indicates if \p D is a template instantiation implicitly generated by the
 /// compiler, e.g.
 ///     template <class T> struct vector {};
 ///     vector<int> v; // 'vector<int>' is an implicit instantiation
 bool isImplicitTemplateInstantiation(const NamedDecl *D);
 /// Indicates if \p D is an explicit template specialization, e.g.
 ///   template <class T> struct vector {};

clang-tools-extra/clangd/AST.cpp

SymbolID getSymbolID(const llvm::StringRef MacroName, const MacroInfo *MI,
   if (MI == nullptr)
     return {};
   llvm::SmallString<128> USR;
   if (index::generateUSRForMacro(MacroName, MI->getDefinitionLoc(), SM, USR))
     return {};
   return SymbolID(USR);
 }
 
-std::string printType(const QualType QT, const DeclContext &CurContext) {
+std::string printType(const QualType QT, const DeclContext &CurContext,
+                      const llvm::StringRef Placeholder) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   PrintingPolicy PP(CurContext.getParentASTContext().getPrintingPolicy());
   PP.SuppressTagKeyword = true;
   PP.SuppressUnwrittenScope = true;
 
   class PrintCB : public PrintingCallbacks {
   public:
     PrintCB(const DeclContext *CurContext) : CurContext(CurContext) {}
     virtual ~PrintCB() {}
     virtual bool isScopeVisible(const DeclContext *DC) const override {
       return DC->Encloses(CurContext);
     }
 
   private:
     const DeclContext *CurContext;
   };
   PrintCB PCB(&CurContext);
   PP.Callbacks = &PCB;
 
-  QT.print(OS, PP);
+  QT.print(OS, PP, Placeholder);
   return OS.str();
 }
 
 bool hasReservedName(const Decl &D) {
   if (const auto *ND = llvm::dyn_cast<NamedDecl>(&D))
     if (const auto *II = ND->getIdentifier())
       return isReservedName(II->getName());
   return false;

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

add_clang_library(clangDaemonTweaks OBJECT
   AnnotateHighlightings.cpp
   DumpAST.cpp
   DefineInline.cpp
   DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp
   ExtractVariable.cpp
+  MemberwiseConstructor.cpp
   ObjCLocalizeStringLiteral.cpp
   ObjCMemberwiseInitializer.cpp
   PopulateSwitch.cpp
   RawStringLiteral.cpp
   RemoveUsingNamespace.cpp
   SpecialMembers.cpp
   SwapIfBranches.cpp
 

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

+//===--- MemberwiseConstructor.cpp - Generate C++ constructor -------------===//
+//
+// 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 "ParsedAST.h"
+#include "refactor/InsertionPoint.h"
+#include "refactor/Tweak.h"
+#include "support/Logger.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/TypeVisitor.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// A tweak that adds a C++ constructor which initializes each member.
+//
+// Given:
+//   struct S{ int x; unique_ptr<double> y; };
+// the tweak inserts the constructor:
+//   S(int x, unique_ptr<double> y) : x(x), y(std::move(y)) {}
+//
+// We place the constructor inline, other tweaks are available to outline it.
+class MemberwiseConstructor : public Tweak {
+public:
+  const char *id() const override final;
+  llvm::StringLiteral kind() const override {
+    return CodeAction::REFACTOR_KIND;
+  }
+  std::string title() const override {
+    return llvm::formatv("Define constructor");
+  }
+
+  bool prepare(const Selection &Inputs) override {
+    // This tweak assumes move semantics.
+    if (!Inputs.AST->getLangOpts().CPlusPlus11)
+      return false;
+
+    // Trigger only on class definitions.
+    if (auto *N = Inputs.ASTSelection.commonAncestor())
+      Class = N->ASTNode.get<CXXRecordDecl>();
+    if (!Class || !Class->isThisDeclarationADefinition() || Class->isUnion() ||
+        Class->getDeclName().isEmpty())
+      return false;
+
+    dlog("MemberwiseConstructor for {0}?", Class->getName());
+    // For now, don't support nontrivial initialization of bases.
+    for (const CXXBaseSpecifier &Base : Class->bases()) {
+      const auto *BaseClass = Base.getType()->getAsCXXRecordDecl();
+      if (!BaseClass || !BaseClass->hasDefaultConstructor()) {
+        dlog("  can't construct base {0}", Base.getType().getAsString());
+        return false;
+      }
+    }
+
+    // We don't want to offer the tweak if there's a similar constructor.
+    // For now, only offer it if all constructors are special members.
+    for (const CXXConstructorDecl *CCD : Class->ctors()) {
+      if (!CCD->isDefaultConstructor() && !CCD->isCopyOrMoveConstructor()) {
+        dlog("  conflicting constructor");
+        return false;
+      }
+    }
+
+    // Examine the fields to see which ones we should initialize.
+    for (const FieldDecl *D : Class->fields()) {
+      switch (FieldAction A = considerField(D)) {
+      case Fail:
+        dlog("  difficult field {0}", D->getName());
+        return false;
+      case Skip:
+        dlog("  (skipping field {0})", D->getName());
+        break;
+      default:
+        Fields.push_back({D, A});
+        break;
+      }
+    }
+    // Only offer the tweak if we have some fields to initialize.
+    if (Fields.empty()) {
+      dlog("  no fields to initialize");
+      return false;
+    }
+
+    return true;
+  }
+
+  Expected<Effect> apply(const Selection &Inputs) override {
+    std::string Code = buildCode();
+    // Prefer to place the new constructor...
+    std::vector<Anchor> Anchors = {
+        // Below special constructors.
+        {[](const Decl *D) {
+           if (const auto *CCD = llvm::dyn_cast<CXXConstructorDecl>(D))
+             return CCD->isDefaultConstructor();
+           return false;
+         },
+         Anchor::Below},
+        // Above other constructors
+        {[](const Decl *D) { return llvm::isa<CXXConstructorDecl>(D); },
+         Anchor::Above},
+        // At the top of the public section
+        {[](const Decl *D) { return true; }, Anchor::Above},
+    };
+    auto Edit = insertDecl(Code, *Class, std::move(Anchors), AS_public);
+    if (!Edit)
+      return Edit.takeError();
+    return Effect::mainFileEdit(Inputs.AST->getSourceManager(),
+                                tooling::Replacements{std::move(*Edit)});
+  }
+
+private:
+  enum FieldAction {
+    Fail,    // Disallow the tweak, we can't handle this field.
+    Skip,    // Do not initialize this field, but allow the tweak anyway.
+    Move,    // Pass by value and std::move into place
+    Copy,    // Pass by value and copy into place
+    CopyRef, // Pass by const ref and copy into place
+  };
+  FieldAction considerField(const FieldDecl *Field) const {
+    if (Field->hasInClassInitializer())
+      return Skip;
+    if (!Field->getIdentifier())
+      return Fail;
+
+    // Decide what to do based on the field type.
+    class Visitor : public TypeVisitor<Visitor, FieldAction> {
+    public:
+      Visitor(const ASTContext &Ctx) : Ctx(Ctx) {}
+      const ASTContext &Ctx;
+
+      // If we don't understand the type, assume we can't handle it.
+      FieldAction VisitType(const Type *T) { return Fail; }
+      FieldAction VisitRecordType(const RecordType *T) {
+        if (const auto *D = T->getAsCXXRecordDecl())
+          return considerClassValue(*D);
+        return Fail;
+      }
+      FieldAction VisitBuiltinType(const BuiltinType *T) {
+        if (T->isInteger() || T->isFloatingPoint() || T->isNullPtrType())
+          return Copy;
+        return Fail;
+      }
+      FieldAction VisitObjCObjectPointerType(const ObjCObjectPointerType *) {
+        return Ctx.getLangOpts().ObjCAutoRefCount ? Copy : Fail;
+      }
+      FieldAction VisitAttributedType(const AttributedType *T) {
+        return Visit(T->getModifiedType().getCanonicalType().getTypePtr());
+      }
+#define ALWAYS(T, Action)                                                      \
+  FieldAction Visit##T##Type(const T##Type *) { return Action; }
+      // Trivially copyable types (pointers and numbers).
+      ALWAYS(Pointer, Copy);
+      ALWAYS(MemberPointer, Copy);
+      ALWAYS(Reference, Copy);
+      ALWAYS(Complex, Copy);
+      ALWAYS(Enum, Copy);
+      // These types are dependent (when canonical) and likely to be classes.
+      // Move is a reasonable generic option.
+      ALWAYS(DependentName, Move);
+      ALWAYS(UnresolvedUsing, Move);
+      ALWAYS(TemplateTypeParm, Move);
+      ALWAYS(TemplateSpecialization, Move);
+    };
+#undef ALWAYS
+    return Visitor(Class->getASTContext())
+        .Visit(Field->getType().getCanonicalType().getTypePtr());
+  }
+
+  // Decide what to do with a field of type C.
+  static FieldAction considerClassValue(const CXXRecordDecl &C) {
+    // We can't always tell if C is copyable/movable without doing Sema work.
+    // We assume operations are possible unless we can prove not.
+    bool CanCopy = C.hasUserDeclaredCopyConstructor() ||
+                   C.needsOverloadResolutionForCopyConstructor() ||
+                   !C.defaultedCopyConstructorIsDeleted();
+    bool CanMove = C.hasUserDeclaredMoveConstructor() ||
+                   (C.needsOverloadResolutionForMoveConstructor() ||
+                    !C.defaultedMoveConstructorIsDeleted());
+    bool CanDefaultConstruct = C.hasDefaultConstructor();
+    if (C.hasUserDeclaredCopyConstructor() ||
+        C.hasUserDeclaredMoveConstructor()) {
+      for (const CXXConstructorDecl *CCD : C.ctors()) {
+        bool IsUsable = !CCD->isDeleted() && CCD->getAccess() == AS_public;
+        if (CCD->isCopyConstructor())
+          CanCopy = CanCopy && IsUsable;
+        if (CCD->isMoveConstructor())
+          CanMove = CanMove && IsUsable;
+        if (CCD->isDefaultConstructor())
+          CanDefaultConstruct = IsUsable;
+      }
+    }
+    dlog("  {0} CanCopy={1} CanMove={2} TriviallyCopyable={3}", C.getName(),
+         CanCopy, CanMove, C.isTriviallyCopyable());
+    if (CanCopy && C.isTriviallyCopyable())
+      return Copy;
+    if (CanMove)
+      return Move;
+    if (CanCopy)
+      return CopyRef;
+    // If it's neither copyable nor movable, then default construction is
+    // likely to make sense (example: std::mutex).
+    if (CanDefaultConstruct)
+      return Skip;
+    return Fail;
+  }
+
+  std::string buildCode() const {
+    std::string S;
+    llvm::raw_string_ostream OS(S);
+
+    if (Fields.size() == 1)
+      OS << "explicit ";
+    OS << Class->getName() << "(";
+    const char *Sep = "";
+    for (const FieldInfo &Info : Fields) {
+      OS << Sep;
+      QualType ParamType = Info.Field->getType().getLocalUnqualifiedType();
+      if (Info.Action == CopyRef)
+        ParamType = Class->getASTContext().getLValueReferenceType(
+            ParamType.withConst());
+      OS << printType(ParamType, *Class,
+                      /*Placeholder=*/paramName(Info.Field));
+      Sep = ", ";
+    }
+    OS << ")";
+    Sep = " : ";
+    for (const FieldInfo &Info : Fields) {
+      OS << Sep << Info.Field->getName() << "(";
+      if (Info.Action == Move)
+        OS << "std::move("; // FIXME: #include <utility> too
+      OS << paramName(Info.Field);
+      if (Info.Action == Move)
+        OS << ")";
+      OS << ")";
+      Sep = ", ";
+    }
+    OS << " {}\n";
+
+    return S;
+  }
+
+  llvm::StringRef paramName(const FieldDecl *Field) const {
+    return Field->getName().trim("_");
+  }
+
+  const CXXRecordDecl *Class = nullptr;
+  struct FieldInfo {
+    const FieldDecl *Field;
+    FieldAction Action;
+  };
+  std::vector<FieldInfo> Fields;
+};
+REGISTER_TWEAK(MemberwiseConstructor)
+
+} // namespace
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   tweaks/DefineOutlineTests.cpp
   tweaks/DumpASTTests.cpp
   tweaks/DumpRecordLayoutTests.cpp
   tweaks/DumpSymbolTests.cpp
   tweaks/ExpandAutoTypeTests.cpp
   tweaks/ExpandMacroTests.cpp
   tweaks/ExtractFunctionTests.cpp
   tweaks/ExtractVariableTests.cpp
+  tweaks/MemberwiseConstructorTests.cpp
   tweaks/ObjCLocalizeStringLiteralTests.cpp
   tweaks/ObjCMemberwiseInitializerTests.cpp
   tweaks/PopulateSwitchTests.cpp
   tweaks/RawStringLiteralTests.cpp
   tweaks/RemoveUsingNamespaceTests.cpp
   tweaks/ShowSelectionTreeTests.cpp
   tweaks/SwapIfBranchesTests.cpp
   tweaks/TweakTesting.cpp

clang-tools-extra/clangd/unittests/tweaks/MemberwiseConstructorTests.cpp

+//===-- MemberwiseConstructorTests.cpp ------------------------------------===//
+//
+// 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 "TweakTesting.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using testing::AllOf;
+using testing::Eq;
+using testing::HasSubstr;
+using testing::Not;
+
+TWEAK_TEST(MemberwiseConstructor);
+
+TEST_F(MemberwiseConstructorTest, Availability) {
+  EXPECT_AVAILABLE("^struct ^S ^{ int x, y; };");
+  EXPECT_UNAVAILABLE("struct S { ^int ^x, y; }; struct ^S;");
+  EXPECT_UNAVAILABLE("struct ^S {};");
+  EXPECT_UNAVAILABLE("union ^S { int x; };");
+  EXPECT_UNAVAILABLE("struct ^S { int x = 0; };");
+  EXPECT_UNAVAILABLE("struct ^S { struct { int x; }; };");
+  EXPECT_UNAVAILABLE("struct ^{ int x; } e;");
+}
+
+TEST_F(MemberwiseConstructorTest, Edits) {
+  Header = R"cpp(
+    struct Move {
+      Move(Move&&) = default;
+      Move(const Move&) = delete;
+    };
+    struct Copy {
+      Copy(Copy&&) = delete;
+      Copy(const Copy&);
+    };
+  )cpp";
+  EXPECT_EQ(apply("struct ^S{Move M; Copy C; int I; int J=4;};"),
+            "struct S{"
+            "S(Move M, const Copy &C, int I) : M(std::move(M)), C(C), I(I) {}\n"
+            "Move M; Copy C; int I; int J=4;};");
+}
+
+TEST_F(MemberwiseConstructorTest, FieldTreatment) {
+  Header = R"cpp(
+    struct MoveOnly {
+      MoveOnly(MoveOnly&&) = default;
+      MoveOnly(const MoveOnly&) = delete;
+    };
+    struct CopyOnly {
+      CopyOnly(CopyOnly&&) = delete;
+      CopyOnly(const CopyOnly&);
+    };
+    struct CopyTrivial {
+      CopyTrivial(CopyTrivial&&) = default;
+      CopyTrivial(const CopyTrivial&) = default;
+    };
+    struct Immovable {
+      Immovable(Immovable&&) = delete;
+      Immovable(const Immovable&) = delete;
+    };
+    template <typename T>
+    struct Traits { using Type = typename T::Type; };
+    using IntAlias = int;
+  )cpp";
+
+  auto Fail = Eq("unavailable");
+  auto Move = HasSubstr(": Member(std::move(Member))");
+  auto CopyRef = AllOf(HasSubstr("S(const "), HasSubstr(": Member(Member)"));
+  auto Copy = AllOf(Not(HasSubstr("S(const ")), HasSubstr(": Member(Member)"));
+  auto With = [](llvm::StringRef Type) {
+    return ("struct ^S { " + Type + " Member; };").str();
+  };
+
+  EXPECT_THAT(apply(With("Immovable")), Fail);
+  EXPECT_THAT(apply(With("MoveOnly")), Move);
+  EXPECT_THAT(apply(With("CopyOnly")), CopyRef);
+  EXPECT_THAT(apply(With("CopyTrivial")), Copy);
+  EXPECT_THAT(apply(With("int")), Copy);
+  EXPECT_THAT(apply(With("IntAlias")), Copy);
+  EXPECT_THAT(apply(With("Immovable*")), Copy);
+  EXPECT_THAT(apply(With("Immovable&")), Copy);
+
+  EXPECT_THAT(apply("template <typename T>" + With("T")), Move);
+  EXPECT_THAT(apply("template <typename T>" + With("typename Traits<T>::Type")),
+              Move);
+  EXPECT_THAT(apply("template <typename T>" + With("T*")), Copy);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang