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-7395] CaseEnumerable and Codable cause crash #49938

Open
belkadan opened this issue Apr 9, 2018 · 11 comments
Open

[SR-7395] CaseEnumerable and Codable cause crash #49938

belkadan opened this issue Apr 9, 2018 · 11 comments

Comments

@belkadan
Copy link
Contributor

@belkadan belkadan commented Apr 9, 2018

Previous ID SR-7395
Radar rdar://problem/39237868
Original Reporter @belkadan
Type Bug
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, Codable, CompilerCrash
Assignee None
Priority Medium

md5: 5c187570ea65a580656b47caf24ec65e

Issue Description:

    struct MyOptions : Codable {
        var debug: Bool = false

        enum CodingKeys : String, CodingKey, CaseEnumerable {
            case debug = "d"
        }
    }

The above code causes a crash in serialization, with an ErrorType appearing somewhere in the CodingKeys enum. This may indicate an error that wasn't diagnosed, or it may indicate a type not being computed property.

(You can get this to happen with swiftc -g.)

@belkadan
Copy link
Contributor Author

@belkadan belkadan commented Apr 9, 2018

CodaFi (JIRA User), @itaiferber, any ideas?

@CodaFi
Copy link
Member

@CodaFi CodaFi commented Apr 11, 2018

This should not compile. The final name of the protocol is CaseIterable. In fact, you can play this game with any invalid protocol name and it crashes. For that reason, I'm out. @itaiferber?

@belkadan
Copy link
Contributor Author

@belkadan belkadan commented Apr 11, 2018

Oh, good catch.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Apr 12, 2018

Looks like this happens only for CodingKeys inside something Codable, i.e. this happens during Decodable/Codable synthesis. Looking into this, though on first attempt I wasn't able to get this to reproduce on master (with no change from me).

@belkadan
Copy link
Contributor Author

@belkadan belkadan commented Apr 12, 2018

Don't forget the -g, to make sure it's emitting a module. But it's possible your recent changes to check for invalid things better were sufficient.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Apr 12, 2018

Looks like this is still an issue in 4.2; I'm repeating the test with a clean build of master just to make sure those changes aren't making it in (since they haven't been merged yet)

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Apr 12, 2018

Looks like this happens on master too. Taking a look

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Apr 12, 2018

@belkadan Okay, looks like the fix here is relatively simple, but highlights a problem with how Codable synthesis produces diagnostics. Since synthesis is relatively linear, any problems during synthesis can be output as we go along. However, save for the very top level, all of the diagnostics produced are notes, not individual errors, which means that unless they are produced after a top-level error, they can get lost in Xcode. i.e., if we produce

note: some codable problem here
note: another codable problem here
error: type 'Foo' does not conform to 'Decodable'

those two notes get lost because Xcode has no error to associate them with. To combat this, PR-10253 introduced the collation of these errors by using a DiagnosticTransaction; synthesis always starts a transaction off with the top-level errors (so notes can be produced afterwards without further thought), and if it turns out that nothing went wrong after that, the transaction is aborted. This means that instead of having to pass around a list of error messages everywhere, we can just produce diagnostics if necessary, and throw them away if they weren't.

The problem is that if we don't consider everything that could go wrong, other diagnostics can get lost. What's happening here is that the CodingKeys enum is inspected in Decodable synthesis before having been fully validated. When we validateDecl(codingKeysTypeDecl) in hasValidCodingKeysEnum, the diagnostic produced for the unknown type CaseEnumerable is added to the same diagnostic transaction as everything else. Since the code assumes incorrectly that this would have been complained about earlier already, it doesn't further validate the enum (e.g. we don't iterate through, say, codingKeysTypeDecl->getInherited() to see if any of the types has an error.

Because validateDecl doesn't directly return whether validation fully succeeded or not, the code didn't check for any further issues, synthesized its end of things as necessary, and declared Decodable synthesis done. This meant the transaction was getting aborted, which meant that the real error was being thrown away too.

The easy fix here is to check for these errors in validateCodingKeysEnum, but I think the "right" solution is to rethink how these errors are collated. Instead of playing whack-a-mole with any errors we fail to realize have happened, it's important that we don't swallow others already produced by validateDecl. This means that we might need to actually pass errors around somehow.

@belkadan
Copy link
Contributor Author

@belkadan belkadan commented Apr 12, 2018

At worst we can collect a bunch of std::function instances that emit the notes, rather than eagerly emitting them.

@itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Apr 12, 2018

@belkadan Yeah, that's one reasonable way to do it. It just still requires us to thread those through other functions which already have their own return types too. Feasible (we could pass in a mutable vector reference to append to instead), just a bit of a pain potentially.

@belkadan
Copy link
Contributor Author

@belkadan belkadan commented Apr 12, 2018

Another option is to make a helper class to contain all of these synthesis functions, and then the class can have state.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants