[clangd] Code action for creating an ObjC initializer

[dgoldman]

Summary

The code action creates an initializer for the selected
ivars/properties, defaulting to all if only the interface/implementation
container is selected.

We add it based on the position of the first non initializer that we
see, and default to adding it where the @end token is.

We also use the ObjC parameter form of (nullable id) instead of
(id _Nullable) if the property has the nullable attribute.

[dgoldman]

Remove debug log statement

[dgoldman]

Minor change to how we get the loc of @end

[dgoldman]

Add another minor test case

[sammccall]

This looks like a useful feature, I had been thinking about adding the equivalent for C++...

High level feedback:

• this should generate both the declaration and definition
• i doubt the interaction around subsetting fields is going to be satisfying
• don't spend complexity on lots of options/micromanaging formatting until basic functionality is solid

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

67

If we're going to care about this (which makes sense), this should be a rangeIncludingComment() in AST.h or so

86

the two returns are inconsistent, this one is the first position outside the range, the other return is the last token (not character) in the range

92

We try not to spend complexity on formatting beyond what clang-format cares about.

If you're inserting before a member (or @end), always inserting 2 newlines seems close enough? (clang-format will strip consecutive blank lines)

114

This algorithm doesn't make much sense to me.
You want to insert before the first non-init instance method, falling back to the end.
So why do you need to do more than search for the first non-init instance method, and find the location before it? In particular why are we looking at init methods and locations after them?

142

Name.consume_front("_"); (FWIW I don't think this needs a comment)

200

I understand the desire to select the particular fields, but given the limitation of the interaction I wonder whether these cases are useful & discoverable enough to be worth the complexity.

• how often is an initializer that sets one variable of many useful?
• out of the times when a subset of variables should be initialized, how often are they contiguous in the code?

I think it would likely be better to start with initializing all members...

219

while here, also grab the InterfaceDecl into another field so you don't have to go casehunting again in initParams?

225

can you rename this method to something clearer?
unclear whether "init" stands for "initialize" or "initializer" here, which would refer to pretty different things!

252

Inferring the container was selected by looking at HadBadNode/Params seems very indirect.
Why not handle this case at the top? if (Container == N->ASTNode.get<ObjcContainerDecl>()) return getAllParams(Iface)

283

!Container isn't possible here

286

nit: why isn't this a return value?

291

Wait, so if the user triggers the action on @interface then they'll get a decl with no impl, and if they trigger on @implementation they'll get the opposite?

Seems like we should at least try to generate both, no? (May not be possible if we're triggering on @interface and index lookup fails, but still...)

292

For building strings like this with formatting, we usually use raw_string_ostream and formatv.
This would make the stuff starting at 303 easier to read

296

This is hard to read, use a raw string

299

nit: why braces? (and elsewhere)

321

you need to verify that Loc is actually in the main file :-(

[dgoldman]

Some review fixes, still need to rebase for new changes

[dgoldman]

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

114

Yeah, I think that makes more sense, changed.

200

I didn't realize that VS Code only lets you select a range, not selecting multiple points (e.g. multiple cursors). I still think this could be useful and it isn't that hard to support, although yes, ideally we could do something like IntelliJ where it presents you a UI to select fields. The main use case of this atm is for simple model objects, and for those I think it could be useful to make some params optional although the contiguous limitation is definitely restrictive.

FWIW I don't think code actions in general are particularly discoverable.

286

IIUC this (having the impl vector be an out param) seems to be the convention - SmallVectorImpl(const SmallVectorImpl &) = delete;, but I can change the return type to instead be SmallVector<MethodParameter, 8>, WDYT?

291

Yeah, I decided to avoid this due to the complexity for a v1, could look into adding it in a follow up. For sourcekit-lsp, it would only work if you've opened the impl first (since it will only have the dynamic index available). Is there anything I could look at which does something similar?

292

Yep that helps quite a bit

[dgoldman]

Rebase + use new helpers

[dgoldman]

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

92

Just went with that for now, can revisit later.

[sammccall]

Alright, the implementation looks good.

I think just a question of what to do when firing on @implementation. I'd suggest one of:

• generating the interface too, you have almost allthe code
• not supporting @implementation yet

We can also keep the current behavior if you feel strongly about it, but I guess it will confuse people.

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

265

If there are no non-init instance methods this will insert at the very bottom of the @implementation. You can add a second anchor to cover this case.

A policy that might make sense here is:

{{IsInitMethod, Below}, {Anything, Above}}

But up to you

286

Please do.

There are a bunch of reasons that vector out-params are sometimes used in LLVM:

• caller rather than callee should choose the SmallVector size
• repeated calls to append to an existing vector
• return value is something else
• some old pre-C++11 code

but I think none apply here

291

This seems like it's going to be a surprising limitation. You can mark it the tweak as "hidden" if you don't want to expose it to users yet.

Is there much complexity in generating both if targeting the @implementation? It seems you've already got both codepaths and just need to run both. If that's too complicated, it doesn't really seem like a good idea to support this on implementations, generating a definition for a member that doesn't exist is confusing.

(I'd suggest changing the description to "declare" memberwise initializer whenever you're not generating the definition.)

For sourcekit-lsp, it would only work if you've opened the impl first

The general approach is to find the target file and then pseudoparse the bits you need, as you won't have an AST in any case. So it will still work without index if the file can be found using filename heuristics.

Is there anything I could look at which does something similar?

DefineOutline does something similar: it triggers on a declaration and edits the declaration and inserts a definition.

[dgoldman]

Support generating interface and impl

[dgoldman]

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

265

I think this should be fine to start off, we probably want to be after class methods.

291

I changed it to add the declaration and impl if you select the implementation, could also do the same for selecting the interface if the impl is still visible, WDYT? Also had to change some editing logic, LMK what you think.

[dgoldman]

PTAL, let me know what you think regarding the edits, had to make sure it works even if we generate multiple edits in the same file.

[sammccall]

clang-tools-extra/clangd/refactor/Tweak.h

85 ↗

(On Diff #397956)

I don't think mergingEdit is a great API (a few issues, including not enough use cases to know if it's right).

Rather than try to polish it here, can we just simulate it in the caller? DefineInline.cpp has an example of this.

[dgoldman]

mergingEdit --> manual file checks

[dgoldman]

friendly ping

[sammccall]

Thanks, and sorry for the long delay.

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

283

Not sure I see the point of using a SmallVector here instead of writing directly into Effect.ApplyEdits?

296

Maybe a comment like:

// If we see the class implementation, add the initializer there too.
// FIXME: merging the edits is awkward, do this elsewhere

(I don't think we should fix it in this patch, but we should at some point)

302

nit: I'd invert the condition here, to avoid the "else" block being a double-negative

[dgoldman]

Fixes for review

[thakis]

Looks like this breaks tests: http://45.33.8.238/linux/71327/step_9.txt

Please take a look and revert for now if it takes a while to fix

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

add_clang_library(clangDaemonTweaks OBJECT
   DumpAST.cpp
   DefineInline.cpp
   DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp
   ExtractVariable.cpp
   ObjCLocalizeStringLiteral.cpp
+  ObjCMemberwiseInitializer.cpp
   PopulateSwitch.cpp
   RawStringLiteral.cpp
   RemoveUsingNamespace.cpp
   SwapIfBranches.cpp
 
   LINK_LIBS
   clangAST
   clangBasic

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

+//===--- ObjCMemberwiseInitializer.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 "ParsedAST.h"
+#include "SourceCode.h"
+#include "refactor/InsertionPoint.h"
+#include "refactor/Tweak.h"
+#include "support/Logger.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/AST/PrettyPrinter.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/LangOptions.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/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+static std::string capitalize(std::string Message) {
+  if (!Message.empty())
+    Message[0] = llvm::toUpper(Message[0]);
+  return Message;
+}
+
+static std::string getTypeStr(const QualType &OrigT, const Decl &D,
+                              unsigned PropertyAttributes) {
+  QualType T = OrigT;
+  PrintingPolicy Policy(D.getASTContext().getLangOpts());
+  Policy.SuppressStrongLifetime = true;
+  std::string Prefix = "";
+  // If the nullability is specified via a property attribute, use the shorter
+  // `nullable` form for the method parameter.
+  if (PropertyAttributes & ObjCPropertyAttribute::kind_nullability) {
+    if (auto Kind = AttributedType::stripOuterNullability(T)) {
+      switch (Kind.getValue()) {
+      case NullabilityKind::Nullable:
+        Prefix = "nullable ";
+        break;
+      case NullabilityKind::NonNull:
+        Prefix = "nonnull ";
+        break;
+      case NullabilityKind::Unspecified:
+        Prefix = "null_unspecified ";
+        break;
+      case NullabilityKind::NullableResult:
+        T = OrigT;
+        break;
+      }
+    }
+  }
+  return Prefix + T.getAsString(Policy);
+}
+
+struct MethodParameter {
+  // Parameter name.
+  llvm::StringRef Name;
+
+  // Type of the parameter.
+  std::string Type;
+
+  // Assignment target (LHS).
+  std::string Assignee;
+
+  MethodParameter(const ObjCIvarDecl &ID) {
+    // Convention maps `@property int foo` to ivar `int _foo`, so drop the
+    // leading `_` if there is one.
+    Name = ID.getName();
+    Name.consume_front("_");
+    Type = getTypeStr(ID.getType(), ID, ObjCPropertyAttribute::kind_noattr);
+    Assignee = ID.getName().str();
+  }
+  MethodParameter(const ObjCPropertyDecl &PD) {
+    Name = PD.getName();
+    Type = getTypeStr(PD.getType(), PD, PD.getPropertyAttributes());
+    if (const auto *ID = PD.getPropertyIvarDecl())
+      Assignee = ID->getName().str();
+    else // Could be a dynamic property or a property in a header.
+      Assignee = ("self." + Name).str();
+  }
+  static llvm::Optional<MethodParameter> parameterFor(const Decl &D) {
+    if (const auto *ID = dyn_cast<ObjCIvarDecl>(&D))
+      return MethodParameter(*ID);
+    if (const auto *PD = dyn_cast<ObjCPropertyDecl>(&D))
+      if (PD->isInstanceProperty())
+        return MethodParameter(*PD);
+    return llvm::None;
+  }
+};
+
+static void getAllParams(const ObjCInterfaceDecl *ID,
+                         SmallVectorImpl<MethodParameter> &Out) {
+  // Currently we only generate based on the ivars and properties declared
+  // in the interface. We could consider expanding this to include visible
+  // categories + class extensions in the future (see
+  // all_declared_ivar_begin).
+  llvm::DenseSet<llvm::StringRef> Names;
+  for (const auto *Ivar : ID->ivars()) {
+    MethodParameter P(*Ivar);
+    if (Names.insert(P.Name).second)
+      Out.push_back(P);
+  }
+  for (const auto *Prop : ID->properties()) {
+    MethodParameter P(*Prop);
+    if (Names.insert(P.Name).second)
+      Out.push_back(P);
+  }
+}
+
+/// Generate an initializer for an Objective-C class based on selected
+/// properties and instance variables.
+class ObjCMemberwiseInitializer : public Tweak {
+public:
+  const char *id() const override final;
+  llvm::StringLiteral kind() const override {
+    return CodeAction::REFACTOR_KIND;
+  }
+
+  bool prepare(const Selection &Inputs) override;
+  Expected<Tweak::Effect> apply(const Selection &Inputs) override;
+  std::string title() const override;
+
+private:
+  void paramsForSelection(const SelectionTree::Node *N,
+                          SmallVectorImpl<MethodParameter> &Out);
+
+  /// Either a `ObjCImplementationDecl` or `ObjCInterfaceDecl`.
+  const ObjCContainerDecl *Container = nullptr;
+
+  const ObjCInterfaceDecl *Interface = nullptr;
+};
+
+REGISTER_TWEAK(ObjCMemberwiseInitializer)
+
+bool ObjCMemberwiseInitializer::prepare(const Selection &Inputs) {
+  const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+  if (!N)
+    return false;
+  const Decl *D = N->ASTNode.get<Decl>();
+  if (!D)
+    return false;
+  const auto &LangOpts = Inputs.AST->getLangOpts();
+  // Require ObjC w/ arc enabled since we don't emit retains.
+  if (!LangOpts.ObjC || !LangOpts.ObjCAutoRefCount)
+    return false;
+
+  // We support the following selected decls:
+  // - ObjCInterfaceDecl/ObjCImplementationDecl only - generate for all
+  //   properties and ivars
+  //
+  // - Specific ObjCPropertyDecl(s)/ObjCIvarDecl(s) - generate only for those
+  //   selected. Note that if only one is selected, the common ancestor will be
+  //   the ObjCPropertyDecl/ObjCIvarDecl itself instead of the container.
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D)) {
+    // Ignore forward declarations (@class Name;).
+    if (!ID->isThisDeclarationADefinition())
+      return false;
+    Container = ID;
+    Interface = ID;
+  } else if (const auto *ID = dyn_cast<ObjCImplementationDecl>(D)) {
+    Container = ID;
+    Interface = ID->getClassInterface();
+  } else if (isa<ObjCPropertyDecl, ObjCIvarDecl>(D)) {
+    const auto *DC = D->getDeclContext();
+    if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(DC)) {
+      Container = ID;
+      Interface = ID;
+    } else if (const auto *ID = dyn_cast<ObjCImplementationDecl>(DC)) {
+      Container = ID;
+      Interface = ID->getClassInterface();
+    }
+  }
+  return Container != nullptr && Interface != nullptr;
+}
+
+void ObjCMemberwiseInitializer::paramsForSelection(
+    const SelectionTree::Node *N, SmallVectorImpl<MethodParameter> &Params) {
+  // Base case: selected a single ivar or property.
+  if (const auto *D = N->ASTNode.get<Decl>()) {
+    if (auto Param = MethodParameter::parameterFor(*D)) {
+      Params.push_back(Param.getValue());
+      return;
+    }
+  }
+  if (Container == N->ASTNode.get<ObjCContainerDecl>() && N->Children.empty())
+    return getAllParams(Interface, Params);
+
+  llvm::DenseSet<llvm::StringRef> Names;
+  // Check for selecting multiple ivars/properties.
+  for (const auto *CNode : N->Children) {
+    const Decl *D = CNode->ASTNode.get<Decl>();
+    if (!D)
+      continue;
+    if (auto P = MethodParameter::parameterFor(*D))
+      if (Names.insert(P->Name).second)
+        Params.push_back(P.getValue());
+  }
+}
+
+Expected<Tweak::Effect>
+ObjCMemberwiseInitializer::apply(const Selection &Inputs) {
+  const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+  if (!N)
+    return error("Invalid selection");
+
+  SmallVector<MethodParameter, 8> Params;
+  paramsForSelection(N, Params);
+
+  bool GenerateImpl = isa<ObjCImplementationDecl>(Container);
+  std::string Code;
+  llvm::raw_string_ostream Stream(Code);
+
+  if (Params.empty()) {
+    if (GenerateImpl) {
+      Stream <<
+          R"cpp(- (instancetype)init {
+  self = [super init];
+  if (self) {
+
+  }
+  return self;
+})cpp";
+    } else {
+      Stream << "- (instancetype)init;";
+    }
+  } else {
+    const auto &First = Params.front();
+    Stream << llvm::formatv("- (instancetype)initWith{0}:({1}){2}",
+                            capitalize(First.Name.trim().str()), First.Type,
+                            First.Name);
+    for (auto It = Params.begin() + 1; It != Params.end(); ++It)
+      Stream << llvm::formatv(" {0}:({1}){0}", It->Name, It->Type);
+
+    if (GenerateImpl) {
+      Stream <<
+          R"cpp( {
+  self = [super init];
+  if (self) {)cpp";
+      for (const auto &Param : Params)
+        Stream << llvm::formatv("\n    {0} = {1};", Param.Assignee, Param.Name);
+      Stream <<
+          R"cpp(
+  }
+  return self;
+})cpp";
+    } else {
+      Stream << ";";
+    }
+  }
+  Stream << "\n\n";
+
+  // Insert before the first non-init instance method.
+  std::vector<Anchor> Anchors = {
+      {[](const Decl *D) {
+         if (const auto *MD = llvm::dyn_cast<ObjCMethodDecl>(D)) {
+           return MD->getMethodFamily() != OMF_init && MD->isInstanceMethod();
+         }
+         return false;
+       },
+       Anchor::Above}};
+  auto Edit = insertDecl(Code, *Container, std::move(Anchors));
+  if (!Edit)
+    return Edit.takeError();
+  return Effect::mainFileEdit(Inputs.AST->getSourceManager(),
+                              tooling::Replacements{std::move(*Edit)});
+}
+
+std::string ObjCMemberwiseInitializer::title() const {
+  return "Generate memberwise initializer";
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang

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

add_unittest(ClangdUnitTests ClangdTests
   tweaks/DumpASTTests.cpp
   tweaks/DumpRecordLayoutTests.cpp
   tweaks/DumpSymbolTests.cpp
   tweaks/ExpandAutoTypeTests.cpp
   tweaks/ExpandMacroTests.cpp
   tweaks/ExtractFunctionTests.cpp
   tweaks/ExtractVariableTests.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
   tweaks/TweakTests.cpp
 

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

+//===-- ObjCMemberwiseInitializerTests.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 "TestTU.h"
+#include "TweakTesting.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(ObjCMemberwiseInitializer);
+
+TEST_F(ObjCMemberwiseInitializerTest, TestAvailability) {
+  FileName = "TestTU.m";
+
+  // Ensure the action can't be triggered since arc is disabled.
+  EXPECT_UNAVAILABLE(R"cpp(
+    @interface Fo^o
+    @end
+  )cpp");
+
+  ExtraArgs.push_back("-fobjc-arc");
+
+  // Ensure the action can be initiated on the interface and implementation,
+  // but not on the forward declaration.
+  EXPECT_AVAILABLE(R"cpp(
+    @interface Fo^o
+    @end
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp(
+    @interface Foo
+    @end
+
+    @implementation F^oo
+    @end
+  )cpp");
+  EXPECT_UNAVAILABLE("@class Fo^o;");
+
+  // Ensure that the action can be triggered on ivars and properties,
+  // including selecting both.
+  EXPECT_AVAILABLE(R"cpp(
+    @interface Foo {
+      id _fi^eld;
+    }
+    @end
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp(
+    @interface Foo
+    @property(nonatomic) id fi^eld;
+    @end
+  )cpp");
+  EXPECT_AVAILABLE(R"cpp(
+    @interface Foo {
+      id _fi^eld;
+    }
+    @property(nonatomic) id pr^op;
+    @end
+  )cpp");
+
+  // Ensure that the action can't be triggered on property synthesis
+  // and methods.
+  EXPECT_UNAVAILABLE(R"cpp(
+    @interface Foo
+    @property(nonatomic) id prop;
+    @end
+
+    @implementation Foo
+    @dynamic pr^op;
+    @end
+  )cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+    @interface Foo
+    @end
+
+    @implementation Foo
+    - (void)fo^o {}
+    @end
+  )cpp");
+}
+
+TEST_F(ObjCMemberwiseInitializerTest, Test) {
+  FileName = "TestTU.m";
+  ExtraArgs.push_back("-fobjc-arc");
+
+  const char *Input = R"cpp(
+@interface Foo {
+  id [[_field;
+}
+@property(nonatomic) id prop]];
+@property(nonatomic) id notSelected;
+@end)cpp";
+  const char *Output = R"cpp(
+@interface Foo {
+  id _field;
+}
+@property(nonatomic) id prop;
+@property(nonatomic) id notSelected;
+- (instancetype)initWithField:(id)field prop:(id)prop;
+
+@end)cpp";
+  EXPECT_EQ(apply(Input), Output);
+
+  Input = R"cpp(
+@interface Foo
+@property(nonatomic, nullable) id somePrettyLongPropertyName;
+@property(nonatomic, nonnull) id someReallyLongPropertyName;
+@end
+
+@implementation F^oo
+
+- (instancetype)init {
+  return self;
+}
+
+@end)cpp";
+  Output = R"cpp(
+@interface Foo
+@property(nonatomic, nullable) id somePrettyLongPropertyName;
+@property(nonatomic, nonnull) id someReallyLongPropertyName;
+@end
+
+@implementation Foo
+
+- (instancetype)init {
+  return self;
+}
+
+- (instancetype)initWithSomePrettyLongPropertyName:(nullable id)somePrettyLongPropertyName someReallyLongPropertyName:(nonnull id)someReallyLongPropertyName {
+  self = [super init];
+  if (self) {
+    _somePrettyLongPropertyName = somePrettyLongPropertyName;
+    _someReallyLongPropertyName = someReallyLongPropertyName;
+  }
+  return self;
+}
+
+@end)cpp";
+  EXPECT_EQ(apply(Input), Output);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang