-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
||
|
@@ -3468,7 +3469,7 @@ getProtocolRequirements() { | |
} | ||
|
||
AddEquatableContext AddEquatableContext:: | ||
getDeclarationContextFromInfo(ResolvedCursorInfo Info) { | ||
getDeclarationContextFromInfo(const ResolvedCursorInfo &Info) { | ||
if (Info.isInvalid()) { | ||
return AddEquatableContext(); | ||
} | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I realize it's not ideal in this state, but I'd like to fix this in a follow up PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() > | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Another option would be to just use -1 the brace end loc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was taken from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The generated code would otherwise produce a warning: The example in the doc comment of this method already said There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah looks like it may have been copy/pasted from |
||
|
||
auto *nestedContainerCall = createNestedContainerKeyedByForKeyCall( | ||
C, funcDC, containerExpr, caseCodingKeys, codingKeyCase); | ||
|
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? | ||
} |
There was a problem hiding this comment.
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 theTypeExpr
'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:I'm happy for this to be done in a follow-up though.