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

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?

Oh, good catch.

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

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.

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)

Looks like this happens on master too. Taking a look

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

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

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

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