Explicit Codable & CodingKey Derived Conformance #8125

itaiferber · 2017-03-15T22:22:35Z

What's in this pull request?
This PR integrates preliminary compiler work to support derived conformance for the newly introduced Codable and CodingKey types which are part of #8124.

This PR focuses on internal compiler features for improving the experience of using the features provided in #8124.

A swift-evolution proposal will be introduced momentarily and will link to this PR.

NOTE: This PR should not be merged until #8124 is.

This work is not complete. Remaining top-level features which will be iterated upon:

Need to not generate decode expressions for immutable vars with default initializers
Provide failure diagnostics
Add unit tests:
- Imported types
- Collections (when possible)
- Generic types
- Nested types
Synthesize CodingKeys type even w/o other derivation

Integrate synthesis of Codable and CodingKey requirements along with preliminary unit tests

slavapestov

Nice work overall, I only have a few nitpicks so far.

Can you add some tests for generic types? I only see non generics here.

slavapestov · 2017-03-16T04:14:22Z

lib/Sema/DerivedConformanceCodable.cpp

+
+  auto funcDC = cast<DeclContext>(encodeDecl);
+  ASTContext &C = funcDC->getASTContext();
+  auto &tc = *(TypeChecker *)C.getLazyResolver();


Please just pass down a TypeChecker instead of doing an unsafe cast here

Unfortunately, we can't. 😕 This is in the body synthesizer for the encode(to:) method, which can only accept the AbstractFunctionDecl whose body it's synthesizing.

The only way we can do this more safely that I can tell is to eagerly generate the body directly in deriveCodable_encode below, instead of using a body synthesizer. Is that better?

I see. I think the conformance itself is lazily synthesized, so there should not be much of a perf win from lazily synthesizing the bodies. However @jrose-apple should comment on this.

FWIW, I followed the exact model that RawRepresentable, Equatable, and Hashable use. If we eagerly synthesize the body here we should probably do it for those as well.

Those don't need to cast the resolver to a TypeChecker though...

I guess a refactor might be in order here. The TypeChecker is only used to be passed into getOrSynthesizeCodingKeysDecl, which at this point should only really be performing a lookup (because it would have already been derived here otherwise). Thinking about it, this is even strictly unnecessary; I should be able to just lookupDirect the CodingKeys decl since at this point it should be guaranteed to be there.

I realized the type checker is actually used further down to tell if the type's superclass is Codable, so we can't get rid of it. Should we not synthesize the body lazily, then?

If you're just doing a conformance check and not trying to record a dependency or emit a diagnostic, you can call ModuleDecl::lookupConformance() instead of TypeChecker::conformsToProtocol().

ModuleDecl::lookupConformance requires a lazy resolver: Optional<ProtocolConformanceRef> lookupConformance(Type type, ProtocolDecl *protocol, LazyResolver *resolver)
Should be safe to pass C.getLazyResolver() without a cast then, yeah?

Doing this allows me to get rid of tc altogether:

static bool superclassIsCodable(ClassDecl *type) { assert(type && "Cannot get superclass of null type."); if (!type->hasSuperclass()) return false; auto &C = type->getASTContext(); auto *codableProto = C.getProtocol(KnownProtocolKind::Codable); auto *superclassDecl = type->getSuperclassDecl(); auto *superclassModule = superclassDecl->getModuleContext(); return (bool)superclassModule->lookupConformance(type->getSuperclass(), codableProto, C.getLazyResolver()); }

Unless this is unsafe for some reason, I'm switching to this

slavapestov · 2017-03-16T04:17:52Z

test/decl/protocol/special/coding/Inputs/class_codable_simple_multi1.swift

@@ -0,0 +1,25 @@
+// REQUIRES: objc_interop


Do these really require objc interop or does this all work on Linux with swift-corelibs-foundation too?

These require the import of Foundation, which necessitates the inclusion of objc_interop, no?

On Linux, objc_interop is off, but you might still have a Foundation module (swift-corelibs-foundation is pure Swift and doesn't use the ObjC runtime as you're no doubt aware).

I think tests should not pull in Foundation on Linux though, however we might still be able to test the conformance derivation by cooking up a 'mock' Foundation module that only provides the necessary protocols and nothing else.

Doug mentioned this too. Keep in mind though that these protocols use Data, which may pull in other things which really do require ObjC; this may prohibit us from being able to mock this in a truly objc_interop-free way...

So to clarify -- this code does work on Linux with corelibs-foundation right? It's just that it's hard to test in this scenario?

Would adding some tests that rely on the conformance being derived to swift-corelibs-foundation be an acceptable solution then?

Yes, I believe so. swift-corelibs-foundation will require some changes to bring it in line with the implementation of JSONEncoder and JSONDecoder specifically, but the primary issue in testing is not one of actual ObjC interop, but one of the testing framework.

All of my implementations should be able to land in swift-corelibs-foundation as well with no changes.

slavapestov · 2017-03-16T04:19:08Z

lib/Sema/DerivedConformanceCodable.cpp

+// Returns whether the type represented by the given ClassDecl inherits from a
+// type which is Codable.
+static bool classInheritsCodable(TypeChecker &tc, ClassDecl *type) {
+  if (!type || !type->hasSuperclass())


What if the class conforms to Codable but has no superclass?

That's handled by default below in getOrSynthesizeCodingKeysDecl, deriveCodable_encode, and deriveCodable_init. This is used to determine whether a class has a Codable superclass so we can emit a super key, and call try super.encode(to: container.superEncoder()) and try super.init(from: container.superDecoder()); if the class is Codable but its superclass is not, we don't need to emit those things.

Perhaps a better name for this function would just be superclassIsCodable or similar?

Yes, that sounds like a better name. Also, can you remove the '!type' check or replace it with an assertion? It's not clear to me that this can happen and we should not let it happen silently if it does.

Yes, I can remove that; there's no real need for it.

slavapestov · 2017-03-16T04:20:13Z

lib/Sema/DerivedConformanceCodable.cpp

+    //
+    // Validate the decl eagerly.
+    if (!varDecl->hasType())
+      tc.validateDecl(varDecl);


hasType/getType will assert if the VarDecl is in a different module. This can happen if you define the Codable conformance in an extension. Can you test this case?

You should use getInterfaceType() instead

I caught this case in testing exactly that — asking the TypeChecker to validate the decl eagerly evaluates the type so that past the evaluateDecl line, hasType() == true. However, if getInterfaceType() allows us to avoid doing this eager evaluation, then we should use that. Looking into it now.

You should not call validateDecl() on deserialized decks. They are already fully validated

validateDecl() is the only way I see that we can do this. getInterfaceType() also asserts, since the decl has no interface type set either: Assertion failed: (hasInterfaceType() && "No interface type was set"), function getInterfaceType, file /Users/itai/Development/swift-oss/swift/lib/AST/Decl.cpp, line 1769.

Is there another way to force evaluation of the type without validating the decl?
FWIW, if the decl is already validated, why does it not have a type? Just curious — trying to learn about the process here

validateDecl() is the right thing to call if the decl is in the same module and has not yet been type checked.

What's the solution here, then? The var decl has no type, nor an interface type, but we need the type to be evaluated in order to continue.

slavapestov · 2017-03-16T04:21:06Z

lib/Sema/DerivedConformanceCodable.cpp

+  // Bind Keyed*Container to Keyed*Container<KeyType>
+  Type boundType[1] = { keyType };
+  auto containerType = BoundGenericType::get(keyedContainerDecl, Type(),
+                                             C.AllocateCopy(boundType));


I don't think the AllocateCopy is necessary

I think it is — BoundGenericType::get takes an ArrayRef referencing the bound types, which it does not copy. The ArrayRef will then be pointing to whatever it was created with, in this case, an array on the stack. We need to allocate a copy or else we'll have a dangling pointer. No?

You're right. It's hard to remember which ArrayRefs are copied and which ones are not. It's a bit of a mess...

Don't worry, I discovered this myself — lots of fun trying to diagnose the undefined behavior... ;)

In general in these cases, the right thing to do is to use a SmallVector or a TinyPtrVector.

I originally used SmallVectors all over the place and was seeing strange behavior, until @DougGregor corrected me.

So, we are storing the ArrayRef here? Sorry I did a drive by. This brings up an interesting question though. If everyone is already copying here, it may make sense to just change this API to always copy. Internally if we run into overhead, we can optimize later. I am not saying that you should fix this in this PR. Just speaking out loud.

BoundGenericType::get takes an ArrayRef without copying it. Most decls do, in fact, because they likely assume that these things were already allocated earlier in the parsing process (which they were), so it's safe to take a reference without copying. Here, however, I'm synthesizing it all from scratch, so it does need to be allocated because these references are long-lived.

I understand. What I was saying is that in the past this has been a source of bugs. Having a more conservative API here from a memory management perspective would be good since those bugs would be defined away. Like I said before, I do not think that this PR is the place to make such a change and I am not putting it on you to fix.

Yep! We're not in disagreement here 😄 An audit of those would be a good idea (some are even taken as MutableArrayRefs...), but not part of this.

itaiferber · 2017-03-16T16:52:55Z

@slavapestov Thanks for the review! As for the generic tests, absolutely — that's on the to-do list above.

slavapestov · 2017-03-16T22:11:01Z

I think you can merge this whenever you're ready -- we can sort out the remaining issues later. I hope you get some time to work on the various TODO comments too :)

itaiferber · 2017-03-16T22:17:18Z

@slavapestov It seems I don't have write access for this repo — do you mind running a CI test please? And yes, the TODO comments are a top priority to eliminate and are covered in the TODO list at the top of this PR.

slavapestov · 2017-03-16T22:17:40Z

@swift-ci Please smoke test

slavapestov · 2017-03-16T22:17:56Z

Wait, don't we have to merge the other PR first?

itaiferber · 2017-03-16T22:18:03Z

I don't want to merge this, though, until #8124 lands as well

itaiferber · 2017-03-16T22:19:21Z

Yeah, we definitely shouldn't merge this until the other one lands. Does the smoke test run the unit tests? Those will fail because the Codable and CodingKey protocols won't actually be in Foundation... If not, this should at least compile and pass.

slavapestov · 2017-03-16T22:19:51Z

Yes, the smoke tests run the tests, that's the whole point of presubmit testing :)

itaiferber · 2017-03-16T22:20:59Z

Duh, yeah. Okay, then this will almost certainly fail ¯\(ツ)/¯

gottesmm

@itaiferber I am looking through this for swift style violations...

I am seeing a couple already. Have you run this through clang-format? It will lower the amount of style violations I need to comment on.

gottesmm · 2017-03-17T18:27:14Z

lib/Sema/DerivedConformanceCodable.cpp

@@ -0,0 +1,950 @@
+#include "TypeChecker.h"


This needs a swift header.

What do you mean, like #include "swift/.../TypeChecker.h"? This is not a public header; it is in lib/Sema/TypeChecker.h, right alongside lib/Sema/DerivedConformanceCodable.cpp. This is how all other clients using TypeChecker import it.

Adding this

gottesmm · 2017-03-17T18:27:38Z

lib/Sema/DerivedConformanceCodingKey.cpp

@@ -0,0 +1,615 @@
+#include "TypeChecker.h"


This needs a swift header.

Adding this

gottesmm · 2017-03-17T18:38:50Z

*No I am talking about adding this to the top of the file: //===--- test.cpp ---------------------------------------------------------===// // // This source file is part of the Swift.org open source project // // Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors // Licensed under Apache License v2.0 with Runtime Library Exception // // See https://swift.org/LICENSE.txt for license information // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors // //===----------------------------------------------------------------------===//

…

On Mar 17, 2017, at 11:36 AM, Itai Ferber ***@***.***> wrote: commented. Reply to this email directly, view it o <#8125 (comment)>

itaiferber · 2017-03-17T18:41:03Z

Ah, got it — will do!

gottesmm

Please fix these issues and run this through git-clang-format. Once that is done, I will look again.

gottesmm · 2017-03-17T18:31:00Z

lib/AST/Decl.cpp

    default:
      return false;
    }
+  } else if (isa<StructDecl>(this) || isa<ClassDecl>(this)) {


This violates LLVM style:

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

All of the code paths before the else if return. The else is not necessary. In fact, you could even invert the if condition and do an early return false if you want to.

When you fix this, can you also add a * to the auto above on knownProtocol. (Or even change it to a reference). I know that this is something that is not in the code that you have touched yourself.

Fixing the else after return. knownProtocol is an Optional<KnownProtocolKind>, so leaving as auto

gottesmm · 2017-03-17T18:33:02Z

lib/Sema/DerivedConformanceCodable.cpp

+  ASTContext &C = tc.Context;
+  auto parentDC = type->getDeclContext();
+  auto superType = type->getSuperclass();
+  return (bool)tc.conformsToProtocol(superType,


Is this bool conversion necessary?

Shouldn't be. Removing

I stand corrected; the return type is Optional<swift::ProtocolConformanceRef>, so the explicit cast is required.

gottesmm · 2017-03-17T18:33:24Z

lib/Sema/DerivedConformanceCodable.cpp

+// Asserts that the Foundation module is loaded, that the given name does not
+// refer to more than one type, and that the entity with the given name refers
+// to a type instead of a different type of declaration.
+static TypeDecl* lookupFoundationTypeDecl(ASTContext &C, Identifier name) {


This violates Swift style. We use 'TypeDecl *'. This is a case that would be caught by clang-format.

I was following style I'd seen earlier, but fixed throughout the file

gottesmm · 2017-03-17T18:33:50Z

lib/Sema/DerivedConformanceCodable.cpp

+                                     ConformanceCheckFlags::Used);
+}
+
+// Looks up a type declaration with the given name in the Foundation module.


If this is a doxygen comment, it should use 3 slashes ala LLVM style.

http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

These weren't meant to be Doxygen comments, but no harm in documenting everything here. Added

gottesmm · 2017-03-17T18:34:18Z

lib/Sema/DerivedConformanceCodable.cpp

+// refer to more than one type, and that the entity with the given name refers
+// to a type instead of a different type of declaration.
+static TypeDecl* lookupFoundationTypeDecl(ASTContext &C, Identifier name) {
+  auto foundationModule = C.getLoadedModule(C.Id_Foundation);


If this is a pointer, please use 'auto *', rather than just auto.

gottesmm · 2017-03-17T18:46:34Z

lib/Sema/DerivedConformanceCodable.cpp

+                                            C.Id_KeyedEncodingContainer,
+                                            codingKeysType);
+
+  auto containerExpr = new (C) DeclRefExpr(ConcreteDeclRef(containerDecl),


More 'auto *'

gottesmm · 2017-03-17T18:46:53Z

lib/Sema/DerivedConformanceCodable.cpp

+                                           AccessSemantics::DirectToStorage);
+
+  auto enumElements = codingKeysEnum->getAllElements();
+  if (!enumElements.empty()) {


This can be extracted into its own function.

gottesmm · 2017-03-17T18:47:04Z

lib/Sema/DerivedConformanceCodable.cpp

+    // `let container` (containerExpr) is generated above.
+
+    // encoder
+    auto encoderParam = encodeDecl->getParameterList(1)->get(0);


More 'auto *' everywhere in the rest of this scope.

gottesmm · 2017-03-17T18:48:28Z

lib/Sema/DerivedConformanceCodable.cpp

+
+    // Now need to generate `try container.encode(x, forKey: .x)` for all
+    // existing properties.
+    for (auto elt : enumElements) {


Is this a pointer? Or a reference? In these cases it is good to put in the '*' or the '&' so people down the line are able to understand the ownership semantics of the for each. We have had several cases of people assuming an auto was a & when it was really a *& resulting in miscompiles.

Pointer; fixed

gottesmm · 2017-03-17T18:48:38Z

lib/Sema/DerivedConformanceCodable.cpp

+                                                  /*Implicit=*/true);
+
+      // container.encode(self.x, forKey: CodingKeys.x)
+      Expr* args[2] = { varExpr, keyExpr };


Please use a SmallVector here.

gottesmm · 2017-03-17T18:50:48Z

@itaiferber If you are unfamiliar with git-clang-format, it will take care of a ton of the issues I brought up, but it only does it on the git-diff of what you changed, so you don't have to worry about changing other pieces of code you didn't touch.

itaiferber · 2017-03-17T18:56:43Z

@gottesmm Thanks for all of these — I will address them and re-run through git-clang-format.

gottesmm · 2017-03-18T00:53:43Z

@itaiferber Btw, of the changes I mentioned, git-clang-format will handle the 'Type*' => 'Type *' transformations. So I would just let it handle those for you

* Add Swift headers * Avoid else-after-return * Convert documentation to Doxygen comments * Follow correct pointer style * Add missing pointer/reference annotations on auto variables * Use SmallDenseSet instead of DenseSet * Split up large functions (and factor out repeated code where possible) * Avoid casting TypeChecker

itaiferber · 2017-03-20T21:42:46Z

@gottesmm That last push should've address most if not all of your concerns, hopefully. Refactored a lot of code in DerivedConformanceCodingKeys.cpp that should shorten it up considerably. Unit tests pass as they did before, and clang-format has nothing to complain about but the indentation of function calls (most of which I find unreadable once it formats them)

gottesmm · 2017-03-20T21:51:59Z

On Mar 20, 2017, at 2:42 PM, Itai Ferber ***@***.***> wrote: @gottesmm <https://github.com/gottesmm> That last push should've address most if not all of your concerns, hopefully. Refactored a lot of code in DerivedConformanceCodingKeys.cpp that should shorten it up considerably. Unit tests pass as they did before, and clang-format has nothing to complain about but the indentation of function calls (most of which I find unreadable once it formats them)

I will look at this later today, I gotta finish debugging something first. Even so, I noticed while looking at this real quick that the change in lib/AST/Decl.h I don't think has been addressed. Michael

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8125 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAee37jSzFQ-GHuhZRDrzpWvir9cnX3Iks5rnvLcgaJpZM4Memyu>.

CodaFi · 2017-04-11T04:31:32Z

lib/AST/Decl.cpp

+      }
+
+      // Empty enums are allowed to conform as well.
+      return enumDecl->getAllElements().empty() ||


Just a heads up: I'm changing hasOnlyCasesWithoutAssociatedValues so you no longer need the first part of this clause.

@CodaFi Thanks for the heads up on this. (Sorry for the delay, got back from vacation yesterday.) Is this change in, or not yet? Any good way for me to track the change so I can update accordingly?

All referenced known types are now part of the Swift stdlib, rather than Foundation

itaiferber · 2017-04-25T18:14:20Z

BTW, I'm currently working on shuffling PRs (this and #8124) so that all the Swift stdlib/derived conformance stuff will be part of one PR and can land together, and the Foundation bits can land separately. This should NOT be merged.

itaiferber · 2017-04-25T20:20:24Z

The work put up here has been subsumed into #9004.

Explicitly derive Codable & CodingKey conformance

c41aabe

Integrate synthesis of Codable and CodingKey requirements along with preliminary unit tests

itaiferber changed the title ~~Explicitly Codable & CodingKey Derived Conformance~~ Explicit Codable & CodingKey Derived Conformance Mar 15, 2017

slavapestov reviewed Mar 16, 2017

View reviewed changes

classInheritsCodable → superclassIsCodable

d7651eb

gottesmm reviewed Mar 17, 2017

View reviewed changes

Itai Ferber added 5 commits March 21, 2017 11:03

Don't decode a let var with a default value

b98b68e

Split derived conformance for Codable into Encodable and Decodable

8ab7f87

Make CodingKey.stringValue non-optional

f4d7d43

Fix stringValue nil return for empty CodingKey enums

369a89b

Update CodingKey unit tests with non-optional stringValue

a411077

CodaFi reviewed Apr 11, 2017

View reviewed changes

Sink core protocols and types into the Swift stdlib

a1902cb

All referenced known types are now part of the Swift stdlib, rather than Foundation

itaiferber mentioned this pull request Apr 25, 2017

Swift Archival & Serialization API #8124

Closed

3 tasks

itaiferber mentioned this pull request Apr 25, 2017

Swift Archival & Serialization API #9004

Merged

itaiferber closed this Apr 25, 2017

itaiferber mentioned this pull request May 2, 2017

[SR-4773] Remove or fix validateDecl in Codable derived conformance #47350

Closed

swift-ci mentioned this pull request Dec 12, 2022

[SR-5092] Compiler crash when referencing a Codable object with self-defined CodingKeys enum #47668

Open

Explicit Codable & CodingKey Derived Conformance #8125

Explicit Codable & CodingKey Derived Conformance #8125

Conversation

itaiferber commented Mar 15, 2017 • edited Loading

slavapestov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiferber Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiferber Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiferber commented Mar 16, 2017

slavapestov commented Mar 16, 2017

itaiferber commented Mar 16, 2017 • edited Loading

slavapestov commented Mar 16, 2017

slavapestov commented Mar 16, 2017

itaiferber commented Mar 16, 2017

itaiferber commented Mar 16, 2017 • edited Loading

slavapestov commented Mar 16, 2017

itaiferber commented Mar 16, 2017 • edited Loading

gottesmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gottesmm commented Mar 17, 2017 via email • edited Loading

itaiferber commented Mar 17, 2017

gottesmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiferber commented Mar 15, 2017 •

edited

Loading

itaiferber Mar 17, 2017 •

edited

Loading

itaiferber Mar 17, 2017 •

edited

Loading

itaiferber commented Mar 16, 2017 •

edited

Loading

itaiferber commented Mar 16, 2017 •

edited

Loading

itaiferber commented Mar 16, 2017 •

edited

Loading

gottesmm commented Mar 17, 2017 via email •

edited

Loading