-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[SR-4920] Compiler crash when extending type with Codable #47497
Comments
Comment by David Owens (JIRA) It should be noted, that `public struct SaveOptions: Codable` works as expected. |
Interesting — this is failing in SILGen. @belkadan Any idea who would best be able to investigate this? |
@jckarter, maybe? Or still you, since you wrote the code generator. :-) |
Comment by David Owens (JIRA) Interestingly enough, there is a test case that would have caught this had there been a non-compiler error version. 😉 swift/test/decl/protocol/special/coding/struct_codable_simple_extension.swift |
@belkadan Of course I'll investigate, but I don't know much about SILGen, which is why I ask... @jckarter Any idea why this wouldn't affect anything in the Foundation overlay (since I use this exact pattern for every one of the Foundation value types; see #9715) but would be present in user code like this? I can repro this with a toolchain, but looking to do so with my own built version of Swift. |
Comment by David Owens (JIRA) Just an FYI: this crashes with the HEAD from yesterday in master too. |
owensd (JIRA User) Yeah, noticed here too. Thanks for the heads-up, though. Going to look at this today to see what's going on here. |
SILGen crashing during the AST walk usually indicates something inconsistent in the AST. If you're pulling contextual types from the original struct declaration into a definition inside an extension, that could cause problems for generic types, since the struct and extension are different generic contexts, but everything here is public and nongeneric, so without looking at the code I can't give more specific guidance. On a related note, @airspeedswift recently pointed out that we should not attempt to auto-synthesize Codable when the conformance is declared on an extension outside of the original declaration's file, since that extension may not have visibility into the full layout of the type. If that isn't already enforced, it'd be good to add that enforcement too while you're looking into this. |
Comment by David Owens (JIRA)
This would be such a huge disappointment and yet another special-case rule in Swift. The special magic that's going on in the background is already off-putting. |
This might be the first place it surfaces, but it's a fundamental limitation on any feature that derives information from the layout of a type. For instance, if we added autoderivation of Equatable/Hashable/Comparable, or memberwise init, they would have the same limitation. It so happens that the existing features in Swift that rely on layout, such as the implicit internal memberwise init on structs and the derivation of RawRepresentable for enums, are tied to the original declaration. It's as if there were a protocol that gives access to the layout details of a type, conformance to the protocol were private to its home file or module, and the standard library declared default implementations in extensions constrained by that protocol. |
@jckarter To be clear, by "layout" do you mean the actual layout of the type in memory, or information about the properties it contains? How is generating the statements in the AST any different than what you'd get if the user typed them in manually? (I'm looking to better understand the limitation here because this is a pretty important part of the feature...) |
> This would be such a huge disappointment and yet another special-case rule in Swift. How is it a special case to only allow access to private properties from code that has access to those properties? Just because the code is being synthesized shouldn't matter. It would also be consistent with how |
@itaiferber The stored properties the type contains. Outside of the home file that's supposed to be abstracted away. |
@jckarter Supposed to, or has to? Because I can see this being a good candidate for special-casing (along with derived conformance for Equatable/Hashable). FWIW, I'll take a look at suppressing derived conformance, but I'm not sure we have access to where the protocol requirements are coming from at that stage in synthesis (i.e. I don't know if it's possible for me to tell whether the conformance is coming from an extension or not, and at that, whether it's part of the home file or not). But I'll investigate. |
Comment by David Owens (JIRA) We are talking about a feature that generates code based on a type but leveraging an existing mechanism (protocol conformance) to do so that is, unless I'm misunderstanding something, completely unavailable outside adding code to the compiler. Applying conformance to `Codable` is unlike any other protocol. The `init` behavior is another special case rule system that I need to understand. That's kind of the meta point here. Hopefully the longer term solution is to actually promote this code up using a public reflection/mirror surface or some other mechanism that is actually available to code authors. Regardless, related to the actual implementation of the bug, if I can create a type using a public init(), then I should be able to persist those values and recreate the type from those values. |
Sure, we're using code generation now, but to me the ideal endpoint for this and similar features would be that they were protocol extensions defined on top of some lower-level Reflectable protocol or something like that. I think we'd still keep the implicit conformance to that protocol private by default, unless you explicitly made the type fragile or overrode its layout with your own conformance. |
Comment by David Owens (JIRA) I'd find that super frustrating as I already have to make up for the `init` internal generation only with a lot of this type of code: public struct TextDocumentClientCapabilities {
public init(
synchronization: SynchronizationCapability? = nil,
completion: CompletionCapability? = nil,
hover: DynamicRegistrationCapability? = nil,
signatureHelp: DynamicRegistrationCapability? = nil,
references: DynamicRegistrationCapability? = nil,
documentHighlight: DynamicRegistrationCapability? = nil,
documentSymbol: DynamicRegistrationCapability? = nil,
formatting: DynamicRegistrationCapability? = nil,
rangeFormatting: DynamicRegistrationCapability? = nil,
onTypeFormatting: DynamicRegistrationCapability? = nil,
definition: DynamicRegistrationCapability? = nil,
codeAction: DynamicRegistrationCapability? = nil,
codeLens: DynamicRegistrationCapability? = nil,
documentLink: DynamicRegistrationCapability? = nil,
rename: DynamicRegistrationCapability? = nil) {
self.synchronization = synchronization
self.completion = completion
self.hover = hover
self.signatureHelp = signatureHelp
self.references = references
self.documentHighlight = documentHighlight
self.documentSymbol = documentSymbol
self.formatting = formatting
self.rangeFormatting = rangeFormatting
self.onTypeFormatting = onTypeFormatting
self.definition = definition
self.codeAction = codeAction
self.codeLens = codeLens
self.documentLink = documentLink
self.rename = rename
} |
I wasn't saying that you couldn't generate public API from internal layout details, but you'd need to declare the public API from a context that has visibility to those details. |
Comment by admin (JIRA) In theory, you can't ever know if you have access to all of a struct's properties from a given context. In practice, within the same module it would be acceptable to say "there are properties you don't have access to" if that were the case, and silently allow the synthesis if it's not. I'm with Ben that this isn't a special case at all. Synthesized methods have to have bodies you could have written yourself. (I am also strongly against using reflection for any of this. That is a worse answer for both secrecy and performance.) |
Oops, that was me, forgetting to log out of the admin account after doing some housekeeping. |
With some further thought, I agree. Given all this, I think it's fair to prevent synthesis in extensions in general and provide a good diagnostic for it. I think it would be confusing to silently allow the synthesis in some contexts, but not in others, based on whether the type being extended has properties which you don't have access to (or not). |
Comment by David Owens (JIRA) If I understand the argument correctly, I consume a library that has a type like: struct SomeGoods {
public init(...) {}
public var valueA: String
public var valueB: String
private var valueC: String
} I cannot retroactively make Also, reflection doesn't have to be a runtime thing only. |
owensd (JIRA User) That's correct. If |
Comment by David Owens (JIRA) Why is there this assumption that I want, need, or must serialize the private members? If a public
So all protocols can be done via extensions except |
The public members might be computed properties based on the private members. They might not all have public setters. They might include things that do not make sense to serialize, like a counter. They might be incredibly redundant, like serializing You can always add Codable via an extension. You just won't get autosynthesis of its requirements. |
This should be addressed by #9758 |
Comment by David Owens (JIRA) @jordan Rose, I know that I can, I can also write the |
I think for the time being, it's appropriate to prevent this with a diagnostic saying it's not possible yet. We can relax the restriction in the near future (we should allow extensions within the same file, for instance). |
David, this isn't an arguable part of Swift's design. The difference between public and non-public APIs is something we consider important, and simultaneously the difference between computed and stored properties is not (from outside the type). We can talk about improvements to ergonomics, but the baseline for anything that deals with the public/non-public distinction is always going to be something that makes sense for library authors who care about source compatibility. (I'm not even against having a mode that turns all that off for someone who's just using libraries organizationally. What I don't want is a language feature that just falls down in that context.) |
Comment by David Owens (JIRA) Maybe I'm not being clear because I think you're misunderstanding me. When the public members match a public init() signature, I'd fully expect to be able to re-create a type from that. public struct SaveOptions {
public init(includeText: Bool?) {
self.includeText = includeText
self.internalDoohookie = true
}
public var includeText: Bool?
private var internalDoohookie: Bool // As a consumer, I don't know about this.
} I'm consuming this type and it doesn't support extension SaveOptions: Codable {
private enum CodingKeys: Int, CodingKey {
case includeText
}
public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)
if let it = self.includeText { try container.encode(it, forKey: .includeText) }
}
public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
let includeText = try container.decodeIfPresent(Bool.self, forKey: .includeText)
self.init(includeText: includeText)
}
} Ignoring the optional limitation for now, I'd expect to be able to add a Are there cases where you couldn't? Sure. But when you could, it would be really nice to not have to write all that boilerplate code. |
That sounds like a reasonable extension to the model, with rules about what to do when there are multiple initializers. That's what swift-evolution's for. |
The current proposal is defined in terms of properties only, ignoring any other initializers. |
Same-file extension derived conformances are up against master in PR-11735. |
The problems with extensions have been resolved, and synthesis in same-file extensions has been enabled in #16376 |
Attachment: Download
Environment
swift-4.0-DEVELOPMENT-SNAPSHOT-2017-05-15-a.xctoolchain
Additional Detail from JIRA
md5: cc9a88ad02818f50033692dc2df80356
relates to:
Issue Description:
Simple program results in a compiler crash:
The text was updated successfully, but these errors were encountered: