[Refactoring] Add Codable refactoring action #41136

louisdh · 2022-02-01T21:24:33Z

Inserts the synthesized implementation.
As part of this, fix some ASTPrinter bugs.

rdar://87904700

louisdh · 2022-02-01T21:27:14Z

lib/Sema/DerivedConformanceCodable.cpp

@@ -1669,7 +1669,7 @@ deriveBodyDecodable_enum_init(AbstractFunctionDecl *initDecl, void *) {
          auto *nestedContainerDecl = createKeyedContainer(
              C, funcDC, C.getKeyedDecodingContainerDecl(),
              caseCodingKeys->getDeclaredInterfaceType(),
-              VarDecl::Introducer::Var, C.Id_nestedContainer);
+              VarDecl::Introducer::Let, C.Id_nestedContainer);


The generated code would otherwise produce a warning:
Variable 'nestedContainer' was never mutated; consider changing to 'let' constant

The example in the doc comment of this method already said let nestedContainer. So this looks like it was a small oversight (which didn't really affect anything so far, I imagine).

Yeah looks like it may have been copy/pasted from deriveBodyEncodable_enum_encode (which does need to be var AFAIK, unlike this case)

louisdh · 2022-02-01T21:28:33Z

test/refactoring/AddCodableImplementation/Outputs/enum/codable.swift.expected

+      let nestedContainer = try container.nestedContainer(keyedBy: Payload.PlainCodingKeys.self, forKey: Payload.CodingKeys.plain)
+
+      self = Payload.plain(try nestedContainer.decode(String.self, forKey: Payload.PlainCodingKeys._0))
+      case .pair:


The indentation is a little weird here, but I don't think that's super important.

Seems like it wouldn't be too hard to fix but... 🤷 See above, I wouldn't have bothered at all 😅

louisdh · 2022-02-01T21:30:45Z

tools/swift-refactor/swift-refactor.cpp

@@ -74,6 +74,7 @@ Action(llvm::cl::desc("kind:"), llvm::cl::init(RefactoringKind::None),
                      "replace-bodies-with-fatalError", "Perform trailing closure refactoring"),
           clEnumValN(RefactoringKind::MemberwiseInitLocalRefactoring, "memberwise-init", "Generate member wise initializer"),
           clEnumValN(RefactoringKind::AddEquatableConformance, "add-equatable-conformance", "Add Equatable conformance"),
+           clEnumValN(RefactoringKind::AddExplicitCodableImplementation, "add-explicit-codable-implementation", "Add Explicit Codable Implementation"),


Note: this behaves different from the existing AddEquatableConformance action. That is, for Codable we really want the synthesized implementation. This action doesn't make your type conform to Codable (which is easy, just make it conform to it manually). It's geared towards "it conforms to Codable, and now I want to insert what the compiler generated".

louisdh · 2022-02-01T21:31:19Z

@swift-ci please test

louisdh · 2022-02-01T21:39:01Z

test/refactoring/AddCodableImplementation/Outputs/basic/codable.swift.expected

+
+    self.firstName = try container.decode(String.self, forKey: User.CodingKeys.firstName)
+    self.lastName = try container.decodeIfPresent(String.self, forKey: User.CodingKeys.lastName)
+    return


The return at the end is redundant of course. I'm not sure how to best handle that in the ASTPrinter. (we'd have to check if it's the last statement in the scope, I suppose).

For now, I think it's fine to just keep it. It compiles, but looks a bit weird.

Is it implicit? If it isn't, could be be? And if so, can we skip printing implicit expressions?

I ended up adding a (very specific) SkipImplicitReturn option to PrintOptions. Skipping all implicit return stmts made an existing test fail, so I wanted to minimize the potential breakage.

Yeah I'm not sure skipping all implicit returns is correct, as implicit just means "created by the compiler", and we shouldn't skip an implicit return in e.g if condition { return }, or one with an expression e.g return 0. Would it be possible to narrow this behavior by having the AST printer only drop an implicit return stmt that is the last element of an AbstractFunctionDecl and has no expression?

having the AST printer only drop an implicit return stmt that is the last element of an AbstractFunctionDecl and has no expression?

Ah, good idea. I've done that and removed the option I added (so it will just always skip the printing of such returns).

bnbarham

Very cool. Thanks for all the work in adding the expression printing too!

There's a few comments but they're all very minor. If you could split the expression printing fixes out into at least a separate commit that would be good, though it was pretty obvious what related to what in this case.

Is the refactoring only valid on the name of the container?

lib/IDE/Refactoring.cpp

bnbarham · 2022-02-01T22:06:06Z

lib/IDE/Refactoring.cpp

+getInsertStartLoc() {
+  SourceLoc MaxLoc = StartLoc;
+  for (auto Mem : Range) {
+    if (Mem->getEndLoc().getOpaquePointerValue() >


SourceManager has a isBeforeInBuffer. Not static since it should be checking the two locations are in the same buffer, though that doesn't seem to be the case right now 🤔.

Another option would be to just use -1 the brace end loc.

This was taken from AddEquatableContext, and I'd rather just keep this as-is. I don't think it has access to a SourceManager here, which would make it slightly more involved to change it as you describe.

You should be able to get it by jumping through a couple of pointers from the DeclContext, e.g DC->getASTContext().SourceMgr

bnbarham · 2022-02-01T22:07:59Z

lib/IDE/Refactoring.cpp

+        : Printer(Printer) {}
+
+  bool walkToDeclPre(Decl *D) override {
+    if (auto valueD = dyn_cast<ValueDecl>(D)) {


We're also not very consistent with naming, but the current style guide is capitalized camel case (as much as I dislike it). The normal convention here would be eg. VD.

Also add the * or & if the variable is a pointer/reference, makes it a little easier to read through.

bnbarham · 2022-02-01T22:09:02Z

lib/IDE/Refactoring.cpp

+  bool walkToDeclPre(Decl *D) override {
+    if (auto valueD = dyn_cast<ValueDecl>(D)) {
+      if (!valueD->isSynthesized()) {
+          return true;


It also says not to use braces or single line ifs but again, there's not really any consistency here so... doesn't really matter.

bnbarham · 2022-02-01T22:12:39Z

lib/IDE/Refactoring.cpp

+          return true;
+      }
+      SmallString<32> Scratch;
+      auto name = valueD->getName().getString(Scratch);


Naming again (there's probably others but I won't mention again).

Also it looks like name is only used as a comparison here, in which case you could use VD->getName()->isCompoundName("init", "from") instead.

ASTContext also has known identifiers for all these, so you could use those. isCompoundName doesn't have an overload to take ArrayRef<Identifier> for the arguments though 🤷 .

And a third way is how DerivedConformanceCodable does it 😆:

if (name.isCompoundName() && name.getBaseName() == ctx.Id_encode) { auto argumentNames = name.getArgumentNames(); if (argumentNames.size() == 1 && argumentNames[0] == ctx.Id_to) // ... }

lib/IDE/Refactoring.cpp

bnbarham · 2022-02-01T23:09:05Z

test/refactoring/AddCodableImplementation/Outputs/enum/codable.swift.expected

+      let nestedContainer = try container.nestedContainer(keyedBy: Payload.PlainCodingKeys.self, forKey: Payload.CodingKeys.plain)
+
+      self = Payload.plain(try nestedContainer.decode(String.self, forKey: Payload.PlainCodingKeys._0))
+      case .pair:


Seems like it wouldn't be too hard to fix but... 🤷 See above, I wouldn't have bothered at all 😅

bnbarham · 2022-02-01T23:13:48Z

test/refactoring/AddCodableImplementation/basic.swift

+  let firstName: String
+  let lastName: String?
+}
+// RUN: %refactor -add-explicit-codable-implementation -source-filename %s -pos=3:8 > %t.result/codable.swift


Alex added a nice refactor-check-compiles that we use for the convert async refactorings. I also prefer CHECK to output + diff, but maybe I'm the only one 😆

Outputting each run to a separate file makes it very easy to spot bugs (and e.g. to copy the code). Having that all inline in comments would make it harder to understand, I think. 🙂

It's not too bad for these, but for many refactorings most of the output doesn't actually matter. I guess this one has a lot compared to the file as a whole though. And personally I find copy pasting often misses bugs 🤷.

Anyway, I'm fine with the separate file, but I would like to check that it compiles.

but I would like to check that it compiles.

As we discussed offline, refactor-check-compiles doesn't currently work when outputting to a file. That might be trivial to fix in the test infrastructure, but not something I want to attempt in this PR.

louisdh · 2022-02-02T04:11:29Z

@swift-ci please test

hamishknight · 2022-02-02T15:54:50Z

lib/Sema/DerivedConformanceCodable.cpp

@@ -1669,7 +1669,7 @@ deriveBodyDecodable_enum_init(AbstractFunctionDecl *initDecl, void *) {
          auto *nestedContainerDecl = createKeyedContainer(
              C, funcDC, C.getKeyedDecodingContainerDecl(),
              caseCodingKeys->getDeclaredInterfaceType(),
-              VarDecl::Introducer::Var, C.Id_nestedContainer);
+              VarDecl::Introducer::Let, C.Id_nestedContainer);


Yeah looks like it may have been copy/pasted from deriveBodyEncodable_enum_encode (which does need to be var AFAIK, unlike this case)

lib/AST/ASTPrinter.cpp

hamishknight · 2022-02-02T16:07:03Z

lib/IDE/Refactoring.cpp

+getInsertStartLoc() {
+  SourceLoc MaxLoc = StartLoc;
+  for (auto Mem : Range) {
+    if (Mem->getEndLoc().getOpaquePointerValue() >


You should be able to get it by jumping through a couple of pointers from the DeclContext, e.g DC->getASTContext().SourceMgr

hamishknight · 2022-02-02T16:36:46Z

lib/IDE/Refactoring.cpp

+  /// Array of all conformed protocols
+  SmallVector<swift::ProtocolDecl *, 2> Protocols;
+
+  /// Range of internal members in declaration
+  DeclRange Range;
+
+  bool conformsToCodableProtocol() {
+    for (ProtocolDecl *Protocol : Protocols) {
+      if (Protocol->getKnownProtocolKind() == KnownProtocolKind::Encodable ||
+          Protocol->getKnownProtocolKind() == KnownProtocolKind::Decodable) {
+        return true;
+      }
+    }
+    return false;
+  }


Could this use ModuleDecl::lookupConformance(Type, ...) instead of searching the conformances manually? That would also let you dig out the init(from:) or encode(to:) witness and check them for isSynthesized if we only want to provide this refactoring in cases where it's a synthesized conformance.

Ah, you're totally right. With this current implementation we'll provide the refactor option when Codable is entirely manually implemented (so there's nothing synthesized).

I realize it's not ideal in this state, but I'd like to fix this in a follow up PR.

Looks like it would appear but do nothing in this state. It's a fairly narrow condition anyway (only on a decl conforming to encodable/decodable), so implementing in a follow up sounds reasonable.

And fix operators not printing when we can't get a `MemberOperatorRef`.

bnbarham · 2022-02-02T20:37:38Z

lib/AST/ASTPrinter.cpp

+      if (FD->hasBody()) {
+        auto *Body = FD->getBody();


We're walking the body here so it would be a bit odd that we don't have one, good to be safe though. But in that case I'd just check for if (auto *Body = FD->getBody()) instead (hasBody just checks the kind).

Inserts the synthesized implementation. As part of this, fix some ASTPrinter bugs. rdar://87904700

louisdh · 2022-02-02T22:43:28Z

@swift-ci please test

swift-ci · 2022-02-03T02:15:25Z

Build failed
Swift Test Linux Platform
Git Sha - 3f3e643

bnbarham · 2022-02-03T03:35:31Z

@swift-ci please test linux

swift-ci · 2022-02-03T08:23:34Z

Build failed
Swift Test Linux Platform
Git Sha - 3f3e643

hamishknight · 2022-02-03T12:56:59Z

lib/AST/ASTPrinter.cpp

+  if (auto metaType = expr->getType()->castTo<AnyMetatypeType>()) {
+    // Don't print `.Type` for an expr.
+    printType(metaType->getInstanceType());
+  } else {
+    printType(expr->getType());
+  }


Just noticed this condition won't actually fail as castTo will never return a null type. Additionally it will crash if the TypeExpr's type has not been set yet (which I don't think can be the case for the Codable synthesis in particular, but in general we should handle it).

We ought to print the instance type if it has been set, or else fallback to printing the TypeRepr instead, e.g:

if (auto ty = expr->getInstanceType()) { printType(ty); } else { expr->getTypeRepr()->print(Printer, Options); }

I'm happy for this to be done in a follow-up though.

bnbarham · 2022-02-03T21:12:35Z

@swift-ci please test Linux platform

louisdh requested review from hamishknight and bnbarham February 1, 2022 21:24

louisdh commented Feb 1, 2022

View reviewed changes

bnbarham reviewed Feb 1, 2022

View reviewed changes

louisdh requested a review from bnbarham February 2, 2022 04:11

hamishknight reviewed Feb 2, 2022

View reviewed changes

[ASTPrinter] Fix expression printing for metatype access

7ebdb8e

And fix operators not printing when we can't get a `MemberOperatorRef`.

bnbarham reviewed Feb 2, 2022

View reviewed changes

louisdh added 3 commits February 2, 2022 14:14

[Refactoring] Add Codable refactoring action

5d36507

Inserts the synthesized implementation. As part of this, fix some ASTPrinter bugs. rdar://87904700

[Refactoring] Don't copy CursorInfo for AddEquatable

da98eb3

[ASTPrinter] Simplify body check

3f3e643

bnbarham approved these changes Feb 2, 2022

View reviewed changes

hamishknight approved these changes Feb 3, 2022

View reviewed changes

bnbarham merged commit ab85807 into swiftlang:main Feb 4, 2022

[Refactoring] Add Codable refactoring action #41136

[Refactoring] Add Codable refactoring action #41136

Uh oh!

Conversation

louisdh commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisdh Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisdh commented Feb 1, 2022

Uh oh!

louisdh Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisdh Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisdh commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisdh commented Feb 2, 2022

Uh oh!

louisdh Feb 1, 2022 •

edited

Loading

louisdh Feb 1, 2022 •

edited

Loading

louisdh Feb 2, 2022 •

edited

Loading