Allow Codable synthesis in same-file extensions #11735

itaiferber · 2017-09-01T21:29:27Z

What's in this pull request?
Resolves SR-4920 by lifting the restriction that prevented Codable synthesis in same-file extensions.

Arbitrary synthesis of Codable in extensions is still disallowed, though.

Lifts the restriction that prevented Codable synthesis in extensions declared in the same source file as the original type declaration.

itaiferber · 2017-09-01T21:29:33Z

@swift-ci Please test

itaiferber · 2017-09-01T21:29:59Z

lib/Sema/DerivedConformanceCodable.cpp

-  // type itself or an extension, in which case we will emit the declaration
-  // normally.
-  if (target->hasClangNode())
-    tc.Context.addExternalDecl(encodeDecl);


We're never going to synthesize conformance for imported types; might as well remove this.

itaiferber · 2017-09-01T21:30:06Z

lib/Sema/DerivedConformanceCodable.cpp

-  // type itself or an extension, in which case we will emit the declaration
-  // normally.
-  if (target->hasClangNode())
-    tc.Context.addExternalDecl(initDecl);


itaiferber · 2017-09-01T21:33:37Z

lib/Sema/TypeCheckDecl.cpp

+      // If we just extended a nominal type with a protocol conformance (which
+      // may have just gotten derived), members which are synthesized might need
+      // to be visited for validation (since the extended type might have
+      // otherwise finished being validated).


This addition is necessary to perform additional passes on types getting synthesized members in extensions after they've already been validated. For instance, the following code:

struct X { let value: Int } extension X : Codable {}

performs four (very simplified) passes:

A first pass on X to validate access and function signatures

A first pass on extension X to validate some access and inherited types

A second pass on X to do conformance checking

A second pass on extension X to do conformance checking

It's pass 4 here which actually synthesizes Codable methods on X. The method bodies, however, are lazily synthesized, so if the function decls are never visited, the bodies are never filled in. This leads to compiling code which references methods which are never compiled in, leading to linker errors.

This forces a pass on synthesized decls in case they haven't been visited before.

extension X : Codable {} struct X { let value: Int }

would otherwise still work because pass 4 above would happen before pass 3, allowing pass 3 to synthesize the method bodies.

This doesn't feel right, but I can't quite put my finger on what the right answer is. @DougGregor would probably know.

@jrose-apple I agree; if there's a better way to do this, I'm more than happy to rip this out.

What is different about Codable that necessitates this logic? Other derived conformances don't need this it seems.

@slavapestov Codable is so far the only protocol for which we synthesize conformance in a non-constrained manner — Equatable, Hashable, and RawRepresentable are currently only synthesized for enums, and at that, they are automatically synthesized even if you don't request conformance. This case never comes up because we don't support arbitrary Equatable/Hashable derived conformance, so extending a struct/class doesn't work. (Yet — when Equatable/Hashable synthesis lands, it will have the same problem.)

jrose-apple

I'd like Doug to take a look at this, but I do have a few comments about testing:

Can you make sure this is rejected across module boundaries, and that we get a reasonable error message in that case?
Can you add an execution test for the successful case?

jrose-apple · 2017-09-01T22:23:47Z

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

@@ -0,0 +1,17 @@
+// RUN: %target-typecheck-verify-swift


Files under Inputs/ don't get tested, so no need for this RUN line.

jrose-apple · 2017-09-01T22:24:09Z

test/decl/protocol/special/coding/class_codable_simple_extension.swift

@@ -1,29 +1,33 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -verify-ignore-unknown


Where is there an unknown location? (as opposed to a cross-file location)

I'm currently re-building Swift now so I can't at the moment get at the exact unknown locations, but when Codable derived conformance fails, the unit tests pick up a few <unknown>:0:0: messages that don't actually appear during compilation. This has been an issue across all of these tests (hence why other files have -verify-ignore-unknown); I've only added it where necessary to get tests to run correctly.

We should figure out where those locations are coming from, because they're bad UX.

Sure. These never appear to anyone who's doing real compilation (they only appear if you do just type checking, which is really only relevant to unit tests), but I'll investigate a bit further.

jrose-apple · 2017-09-01T22:25:47Z

This is all suspicious for classes too, which are supposed to have all of their designated initializers in the original class declaration. I think you should just restrict it to structs and enums for now.

swift-ci · 2017-09-01T22:26:09Z

Build failed
Swift Test OS X Platform
Git Sha - f74d33c

itaiferber · 2017-09-01T22:35:50Z

@jrose-apple I can add the tests you asked for. Looks like some other tests have failed (either because of this or unrelated) so I'll look into that as well.

As for not doing this for classes — how much does that restriction matter? Since init(from:) is being synthesized, it doesn't really have a source location anyway...

jrose-apple · 2017-09-01T23:51:07Z

It's about the AST structure. We walk the ClassDecl to find all the initializers when we check if a subclass has overridden them all.

slavapestov · 2017-09-08T07:59:57Z

lib/Sema/TypeCheckDecl.cpp

+              auto layout = canType->getExistentialLayout();
+              for (auto protocolType : layout.getProtocols())
+                inheritedProtocols.push_back(protocolType->getDecl());
+            } else if (auto *inheritedNominal = canType->getAnyNominal()) {


I don't think this 'else if' is reachable. isExistentialType() will already return true for ProtocolType.

itaiferber · 2017-09-08T20:26:47Z

@jrose-apple @slavapestov Thanks for the comments. I'm going to close this PR for now to rethink this a bit. If you can think of a better approach, let me know. For now, I don't think this is a big deal, but as we allow synthesis of Equatable and Hashable, more people are going to hit this limitation.

allevato · 2017-10-25T06:05:43Z

Is there a chance that this could be revisited soon? 😀 Now that synthesized Equatable/Hashable has landed, and it sounds like conditional conformances are on the horizon, same-file extension synthesis is an important part of the story that allows container types like Optional, Array, and Dictionary to be usable as fields in types with synthesized operations.

jrose-apple · 2017-10-25T16:28:59Z

Why is that? Why wouldn't the existing code just work?

itaiferber · 2017-10-25T16:30:52Z

@allevato I agree that this should be revisited, but this isn't preventing anything from happening. You just can't get implicit implementation of Codable in an extension at the moment...

jrose-apple · 2017-10-25T16:39:28Z

Ah, I see. If your type conditionally conforms to Codable, the conformance won't be autosynthesized. But the most common case will be using types that conditionally conform to Codable, and that will work fine as it does today.

allevato · 2017-10-25T17:05:30Z

Yeah, that's true—I was thinking a little too myopically about what needed to be synthesized. Array and Dictionary would likely still need to implement manual operators because they're complex containers.

I was mainly thinking of being able to write this:

public enum Optional<Wrapped> {
  // same as before
}

// These get synthesized
extension Optional: Equatable where Wrapped: Equatable {}
extension Optional: Hashable where Wrapped: Hashable {}

But those are simple enough to implement that it may not be compelling on its own, and maybe it doesn't affect user code that much after all.

itaiferber · 2017-10-25T17:12:37Z

@allevato Does that currently work with Equatable and Hashable synthesis, or is that also explicitly disabled? I haven't had the time to look at the implementation, unfortunately.

I agree, though, this would be nice to have for sure.

allevato · 2017-10-25T17:18:11Z

No, Equatable/Hashable also forbids synthesis in an extension right now. When we were first discussing that long ago there were resilience concerns, but now same-file extensions should be safe because they have access to everything private.

I started working on a PR to add it to Equatable/Hashable, but then I pulled up this PR and saw that it wasn't as simple as I thought it might be at the outset.

itaiferber · 2017-10-25T17:24:05Z

Yeah, this will likely require some deeper investigation and potential rearchitecting; it's on my list of things to get done for Swift 4.1 though, so this isn't getting lost in the meantime.

Allow Codable synthesis in same-file extensions

f74d33c

Lifts the restriction that prevented Codable synthesis in extensions declared in the same source file as the original type declaration.

itaiferber requested a review from jrose-apple September 1, 2017 21:29

itaiferber commented Sep 1, 2017

View reviewed changes

jrose-apple reviewed Sep 1, 2017

View reviewed changes

benrimmington mentioned this pull request Sep 4, 2017

Synthesize Equatable/Hashable for complex enums, structs #9619

Merged

slavapestov reviewed Sep 8, 2017

View reviewed changes

itaiferber closed this Sep 8, 2017

This was referenced Sep 26, 2017

[SR-5965] Unexpected type-mismatch errors when parsing JSON in Release configuration #48524

Closed

[SR-4920] Compiler crash when extending type with Codable #47497

Closed

		@@ -1,29 +1,33 @@
		// RUN: %target-typecheck-verify-swift
		// RUN: %target-typecheck-verify-swift -verify-ignore-unknown

Allow Codable synthesis in same-file extensions #11735

Allow Codable synthesis in same-file extensions #11735

Conversation

itaiferber commented Sep 1, 2017

Uh oh!

itaiferber commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber Sep 1, 2017 • 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!

jrose-apple left a comment

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!

jrose-apple commented Sep 1, 2017

Uh oh!

swift-ci commented Sep 1, 2017

Uh oh!

itaiferber commented Sep 1, 2017

Uh oh!

jrose-apple commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 8, 2017

Uh oh!

allevato commented Oct 25, 2017

Uh oh!

jrose-apple commented Oct 25, 2017

Uh oh!

itaiferber commented Oct 25, 2017

Uh oh!

jrose-apple commented Oct 25, 2017

Uh oh!

allevato commented Oct 25, 2017

Uh oh!

itaiferber commented Oct 25, 2017

Uh oh!

allevato commented Oct 25, 2017

Uh oh!

itaiferber commented Oct 25, 2017

Uh oh!

itaiferber Sep 1, 2017 •

edited

Loading