Skip to content

[Refactoring] Add Codable refactoring action #41136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/AST/Decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,8 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated<Decl> {
void print(raw_ostream &OS) const;
void print(raw_ostream &OS, const PrintOptions &Opts) const;

void printInherited(ASTPrinter &Printer, const PrintOptions &Options) const;

/// Pretty-print the given declaration.
///
/// \param Printer ASTPrinter object.
Expand Down
4 changes: 4 additions & 0 deletions include/swift/AST/PrintOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,10 @@ struct PrintOptions {
/// Whether to print inheritance lists for types.
bool PrintInherited = true;

/// Whether to print a space before the `:` of an inheritance list in a type
/// decl.
bool PrintSpaceBeforeInheritance = true;

/// Whether to print feature checks for compatibility with older Swift
/// compilers that might parse the result.
bool PrintCompatibilityFeatureChecks = false;
Expand Down
2 changes: 2 additions & 0 deletions include/swift/IDE/RefactoringKinds.def
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ CURSOR_REFACTORING(MemberwiseInitLocalRefactoring, "Generate Memberwise Initiali

CURSOR_REFACTORING(AddEquatableConformance, "Add Equatable Conformance", add.equatable.conformance)

CURSOR_REFACTORING(AddExplicitCodableImplementation, "Add Explicit Codable Implementation", add.explicit-codable-implementation)

CURSOR_REFACTORING(ConvertCallToAsyncAlternative, "Convert Call to Async Alternative", convert.call-to-async)

CURSOR_REFACTORING(ConvertToAsync, "Convert Function to Async", convert.func-to-async)
Expand Down
31 changes: 28 additions & 3 deletions lib/AST/ASTPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,7 @@ class PrintAST : public ASTVisitor<PrintAST> {
Decl *attachingTo);
void printWhereClauseFromRequirementSignature(ProtocolDecl *proto,
Decl *attachingTo);
void printInherited(const Decl *decl);

void printGenericSignature(GenericSignature genericSig,
unsigned flags);
Expand Down Expand Up @@ -889,7 +890,6 @@ class PrintAST : public ASTVisitor<PrintAST> {
bool openBracket = true, bool closeBracket = true);
void printGenericDeclGenericParams(GenericContext *decl);
void printDeclGenericRequirements(GenericContext *decl);
void printInherited(const Decl *decl);
void printBodyIfNecessary(const AbstractFunctionDecl *decl);

void printEnumElement(EnumElementDecl *elt);
Expand Down Expand Up @@ -2372,7 +2372,10 @@ void PrintAST::printInherited(const Decl *decl) {
if (TypesToPrint.empty())
return;

Printer << " : ";
if (Options.PrintSpaceBeforeInheritance) {
Printer << " ";
}
Printer << ": ";

interleave(TypesToPrint, [&](InheritedEntry inherited) {
if (inherited.isUnchecked)
Expand Down Expand Up @@ -4151,7 +4154,12 @@ void PrintAST::visitLoadExpr(LoadExpr *expr) {
}

void PrintAST::visitTypeExpr(TypeExpr *expr) {
printType(expr->getType());
if (auto metaType = expr->getType()->castTo<AnyMetatypeType>()) {
// Don't print `.Type` for an expr.
printType(metaType->getInstanceType());
} else {
printType(expr->getType());
}
Comment on lines +4157 to +4162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void PrintAST::visitArrayExpr(ArrayExpr *expr) {
Expand Down Expand Up @@ -4234,6 +4242,8 @@ void PrintAST::visitBinaryExpr(BinaryExpr *expr) {
Printer << " ";
if (auto operatorRef = expr->getFn()->getMemberOperatorRef()) {
Printer << operatorRef->getDecl()->getBaseName();
} else if (auto *operatorRef = dyn_cast<DeclRefExpr>(expr->getFn())) {
Printer << operatorRef->getDecl()->getBaseName();
}
Printer << " ";
visit(expr->getRHS());
Expand Down Expand Up @@ -4544,6 +4554,16 @@ void PrintAST::visitBraceStmt(BraceStmt *stmt) {
}

void PrintAST::visitReturnStmt(ReturnStmt *stmt) {
if (!stmt->hasResult()) {
if (auto *FD = dyn_cast<AbstractFunctionDecl>(Current)) {
if (auto *Body = FD->getBody()) {
if (Body->getLastElement().dyn_cast<Stmt *>() == stmt) {
// Don't print empty return.
return;
}
}
}
}
Printer << tok::kw_return;
if (stmt->hasResult()) {
Printer << " ";
Expand Down Expand Up @@ -4733,6 +4753,11 @@ bool Decl::print(ASTPrinter &Printer, const PrintOptions &Opts) const {
return printer.visit(const_cast<Decl *>(this));
}

void Decl::printInherited(ASTPrinter &Printer, const PrintOptions &Opts) const {
PrintAST printer(Printer, Opts);
printer.printInherited(this);
}

bool Decl::shouldPrintInContext(const PrintOptions &PO) const {
// Skip getters/setters. They are part of the variable or subscript.
if (isa<AccessorDecl>(this))
Expand Down
176 changes: 174 additions & 2 deletions lib/IDE/Refactoring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3354,7 +3354,8 @@ class AddEquatableContext {
AddEquatableContext() : DC(nullptr), Adopter(), ProtocolsLocations(),
Protocols(), StoredProperties(), Range(nullptr, nullptr) {};

static AddEquatableContext getDeclarationContextFromInfo(ResolvedCursorInfo Info);
static AddEquatableContext
getDeclarationContextFromInfo(const ResolvedCursorInfo &Info);

std::string getInsertionTextForProtocol();

Expand Down Expand Up @@ -3468,7 +3469,7 @@ getProtocolRequirements() {
}

AddEquatableContext AddEquatableContext::
getDeclarationContextFromInfo(ResolvedCursorInfo Info) {
getDeclarationContextFromInfo(const ResolvedCursorInfo &Info) {
if (Info.isInvalid()) {
return AddEquatableContext();
}
Expand Down Expand Up @@ -3526,6 +3527,177 @@ performChange() {
return false;
}

class AddCodableContext {

/// Declaration context
DeclContext *DC;

/// Start location of declaration context brace
SourceLoc StartLoc;

/// 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;
}
Comment on lines +3538 to +3552
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public:
AddCodableContext(NominalTypeDecl *Decl)
: DC(Decl), StartLoc(Decl->getBraces().Start),
Protocols(Decl->getAllProtocols()), Range(Decl->getMembers()){};

AddCodableContext(ExtensionDecl *Decl)
: DC(Decl), StartLoc(Decl->getBraces().Start),
Protocols(Decl->getExtendedNominal()->getAllProtocols()),
Range(Decl->getMembers()){};

AddCodableContext() : DC(nullptr), Protocols(), Range(nullptr, nullptr){};

static AddCodableContext
getDeclarationContextFromInfo(const ResolvedCursorInfo &Info);

void printInsertionText(const ResolvedCursorInfo &CursorInfo,
SourceManager &SM, llvm::raw_ostream &OS);

bool isValid() { return StartLoc.isValid() && conformsToCodableProtocol(); }

SourceLoc getInsertStartLoc();
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

MaxLoc.getOpaquePointerValue()) {
MaxLoc = Mem->getEndLoc();
}
}
return MaxLoc;
}

/// Walks an AST and prints the synthesized Codable implementation.
class SynthesizedCodablePrinter : public ASTWalker {
private:
ASTPrinter &Printer;

public:
SynthesizedCodablePrinter(ASTPrinter &Printer) : Printer(Printer) {}

bool walkToDeclPre(Decl *D) override {
auto *VD = dyn_cast<ValueDecl>(D);
if (!VD)
return false;

if (!VD->isSynthesized()) {
return true;
}
SmallString<32> Scratch;
auto name = VD->getName().getString(Scratch);
// Print all synthesized enums,
// since Codable can synthesize multiple enums (for associated values).
auto shouldPrint =
isa<EnumDecl>(VD) || name == "init(from:)" || name == "encode(to:)";
if (!shouldPrint) {
// Some other synthesized decl that we don't want to print.
return false;
}

Printer.printNewline();

if (auto enumDecl = dyn_cast<EnumDecl>(D)) {
// Manually print enum here, since we don't want to print synthesized
// functions.
Printer << "enum " << enumDecl->getNameStr();
PrintOptions Options;
Options.PrintSpaceBeforeInheritance = false;
enumDecl->printInherited(Printer, Options);
Printer << " {";
for (Decl *EC : enumDecl->getAllElements()) {
Printer.printNewline();
Printer << " ";
EC->print(Printer, Options);
}
Printer.printNewline();
Printer << "}";
return false;
}

PrintOptions Options;
Options.SynthesizeSugarOnTypes = true;
Options.FunctionDefinitions = true;
Options.VarInitializers = true;
Options.PrintExprs = true;
Options.TypeDefinitions = true;
Options.ExcludeAttrList.push_back(DAK_HasInitialValue);

Printer.printNewline();
D->print(Printer, Options);

return false;
}
};

void AddCodableContext::printInsertionText(const ResolvedCursorInfo &CursorInfo,
SourceManager &SM,
llvm::raw_ostream &OS) {
StringRef ExtraIndent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, is this why you get the start as the last member rather than the end brace of the decl? I didn't worry about indentation in the convert async refactorings and just assumed that clients can fix indents on the returned result.

StringRef CurrentIndent =
Lexer::getIndentationForLine(SM, getInsertStartLoc(), &ExtraIndent);
std::string Indent;
if (getInsertStartLoc() == StartLoc) {
Indent = (CurrentIndent + ExtraIndent).str();
} else {
Indent = CurrentIndent.str();
}
Comment on lines +3657 to +3661
Copy link
Contributor

@bnbarham bnbarham Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is part of the reason I think it's just better for clients to handle the indentation - we have no idea what the indent level is, so we're just guessing it's 4 spaces here 🤷.

If you want to keep it I'd extract getInsertStartLoc into a variable, but I'm not convinced we should bother.


ExtraIndentStreamPrinter Printer(OS, Indent);
Printer.printNewline();
SynthesizedCodablePrinter Walker(Printer);
DC->getAsDecl()->walk(Walker);
}

AddCodableContext AddCodableContext::getDeclarationContextFromInfo(
const ResolvedCursorInfo &Info) {
if (Info.isInvalid()) {
return AddCodableContext();
}
if (!Info.IsRef) {
if (auto *NomDecl = dyn_cast<NominalTypeDecl>(Info.ValueD)) {
return AddCodableContext(NomDecl);
}
}
// TODO: support extensions
// (would need to get synthesized nodes from the main decl,
// and only if it's in the same file?)
return AddCodableContext();
}

bool RefactoringActionAddExplicitCodableImplementation::isApplicable(
const ResolvedCursorInfo &Tok, DiagnosticEngine &Diag) {
return AddCodableContext::getDeclarationContextFromInfo(Tok).isValid();
}

bool RefactoringActionAddExplicitCodableImplementation::performChange() {
auto Context = AddCodableContext::getDeclarationContextFromInfo(CursorInfo);

SmallString<64> Buffer;
llvm::raw_svector_ostream OS(Buffer);
Context.printInsertionText(CursorInfo, SM, OS);

EditConsumer.insertAfter(SM, Context.getInsertStartLoc(), OS.str());
return false;
}

static CharSourceRange
findSourceRangeToWrapInCatch(const ResolvedCursorInfo &CursorInfo,
SourceFile *TheFile,
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/DerivedConformanceCodable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


auto *nestedContainerCall = createNestedContainerKeyedByForKeyCall(
C, funcDC, containerExpr, caseCodingKeys, codingKeyCase);
Expand Down
5 changes: 5 additions & 0 deletions test/expr/print/callexpr.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,8 @@ test(a: 0, b: 0, c: 0)
// CHECK: test(a: 0)
// CHECK: test(a: 0, b: 0)
// CHECK: test(a: 0, b: 0, c: 0)

class Constants { static var myConst = 0 }
func test(foo: Int) { }
// CHECK-LABEL: test(foo: Constants.myConst)
test(foo: Constants.myConst)
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@

struct User: Codable {
let firstName: String
let lastName: String?

enum CodingKeys: CodingKey {
case firstName
case lastName
}

init(from decoder: Decoder) throws {
let container: KeyedDecodingContainer<User.CodingKeys> = try decoder.container(keyedBy: User.CodingKeys.self)

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

}

func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: User.CodingKeys.self)

try container.encode(self.firstName, forKey: User.CodingKeys.firstName)
try container.encodeIfPresent(self.lastName, forKey: User.CodingKeys.lastName)
}
}

struct User_D: Decodable {
let firstName: String
let lastName: String?
}

struct User_E: Encodable {
let firstName: String
let lastName: String?
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@

struct User: Codable {
let firstName: String
let lastName: String?
}

struct User_D: Decodable {
let firstName: String
let lastName: String?

enum CodingKeys: CodingKey {
case firstName
case lastName
}

init(from decoder: Decoder) throws {
let container: KeyedDecodingContainer<User_D.CodingKeys> = try decoder.container(keyedBy: User_D.CodingKeys.self)

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

}
}

struct User_E: Encodable {
let firstName: String
let lastName: String?
}
Loading