Skip to content
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

Closed
swift-ci opened this issue May 18, 2017 · 34 comments
Closed

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

swift-ci opened this issue May 18, 2017 · 34 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself crash Bug: A crash, i.e., an abnormal termination of software

Comments

@swift-ci
Copy link
Collaborator

Previous ID SR-4920
Radar rdar://problem/33877132
Original Reporter owensd (JIRA User)
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment

swift-4.0-DEVELOPMENT-SNAPSHOT-2017-05-15-a.xctoolchain

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, CompilerCrash
Assignee @huonw
Priority Medium

md5: cc9a88ad02818f50033692dc2df80356

relates to:

  • SR-6803 Accept same-file extensions for automatic synthesis of Equatable/Hashable

Issue Description:

Simple program results in a compiler crash:

import Foundation

public struct SaveOptions  {
    public init(includeText: Bool) {
        self.includeText = includeText
    }
    public var includeText: Bool
}

extension SaveOptions: Codable {}

let encoder = JSONEncoder()
let decoder = JSONDecoder()
let saveOptions = SaveOptions(includeText: true)
let data = try encoder.encode(saveOptions)
let json = String(data: data, encoding: .utf8)!
let type = try decoder.decode(SaveOptions.self, from: data)

print("encoded: \(data)")
print("json: \(json)")
print("type: \(type)")
@swift-ci
Copy link
Collaborator Author

Comment by David Owens (JIRA)

It should be noted, that `public struct SaveOptions: Codable` works as expected.

@itaiferber
Copy link
Contributor

Interesting — this is failing in SILGen. @belkadan Any idea who would best be able to investigate this?

@belkadan
Copy link
Contributor

@jckarter, maybe? Or still you, since you wrote the code generator. :-)

@swift-ci
Copy link
Collaborator Author

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

@itaiferber
Copy link
Contributor

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

@swift-ci
Copy link
Collaborator Author

Comment by David Owens (JIRA)

Just an FYI: this crashes with the HEAD from yesterday in master too.

@itaiferber
Copy link
Contributor

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.

@jckarter
Copy link
Member

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.

@swift-ci
Copy link
Collaborator Author

Comment by David Owens (JIRA)

On a related note, Ben Cohen 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.

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.

@jckarter
Copy link
Member

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.

@itaiferber
Copy link
Contributor

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

@airspeedswift
Copy link
Member

> 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 init works. If you declare your own init inside the type, you don't get a synthesized one. But someone outside your module can't t kill the synthesized one by declaring their own init.

@jckarter
Copy link
Member

@itaiferber The stored properties the type contains. Outside of the home file that's supposed to be abstracted away.

@itaiferber
Copy link
Contributor

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

@swift-ci
Copy link
Collaborator Author

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.

@jckarter
Copy link
Member

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.

@swift-ci
Copy link
Collaborator Author

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
    }

@jckarter
Copy link
Member

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.

@swift-ci
Copy link
Collaborator Author

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

@belkadan
Copy link
Contributor

Oops, that was me, forgetting to log out of the admin account after doing some housekeeping.

@itaiferber
Copy link
Contributor

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

@swift-ci
Copy link
Collaborator Author

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 SomeGoods conform to Codable without writing the conformance code myself, even though I wouldn't be able to access that private variable?

Also, reflection doesn't have to be a runtime thing only.

@itaiferber
Copy link
Contributor

owensd (JIRA User) That's correct. If SomeGoods didn't define its own Codable conformance, you shouldn't be able to somehow get to its private contents if they would be encoded, despite SomeGoods' wishes.

@swift-ci
Copy link
Collaborator Author

Comment by David Owens (JIRA)

Why is there this assumption that I want, need, or must serialize the private members? If a public init() exists for SomeGoods that takes in the public members, that should be sufficient for recreating SomeGoods.

Given all this, I think it's fair to prevent synthesis in extensions in general and provide a good diagnostic for it.

So all protocols can be done via extensions except Codable then? And potentially Equatable and Hashable too?

@belkadan
Copy link
Contributor

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 x, y, r, and theta for a point.

You can always add Codable via an extension. You just won't get autosynthesis of its requirements.

@itaiferber
Copy link
Contributor

This should be addressed by #9758

@swift-ci
Copy link
Collaborator Author

Comment by David Owens (JIRA)

@jordan Rose, I know that I can, I can also write the public init(...) for my 10 members that need to be initialized. It's just tedious and error prone.

@itaiferber
Copy link
Contributor

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

@belkadan
Copy link
Contributor

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

@swift-ci
Copy link
Collaborator Author

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 Codable, I want to add it. Today, I have to write this:

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 Codeable conformance to that type via an extension and be able to re-create that type. Whatever the internal privates are, the init(includeText🙂 must set them to something. That code is all able to be generated by you at compile time.

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.

@belkadan
Copy link
Contributor

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.

@belkadan
Copy link
Contributor

The current proposal is defined in terms of properties only, ignoring any other initializers.

@itaiferber
Copy link
Contributor

Same-file extension derived conformances are up against master in PR-11735.

@huonw
Copy link
Mannequin

huonw mannequin commented May 9, 2018

The problems with extensions have been resolved, and synthesis in same-file extensions has been enabled in #16376

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added the crash Bug: A crash, i.e., an abnormal termination of software label Dec 12, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself crash Bug: A crash, i.e., an abnormal termination of software
Projects
None yet
Development

No branches or pull requests

6 participants