Fix crash in typechecking invalid CodingKeys alias #15859
Conversation
|
@swift-ci Please test |
| @@ -300,9 +300,17 @@ static CodingKeysValidity hasValidCodingKeysEnum(TypeChecker &tc, | |||
| if (!tc.conformsToProtocol(codingKeysType, codingKeyProto, | |||
| target->getDeclContext(), | |||
| ConformanceCheckFlags::Used)) { | |||
| tc.diagnose(codingKeysTypeDecl->getLoc(), | |||
| diag::codable_codingkeys_type_does_not_conform_here, | |||
| // If CodingKeys is a typealias which doesn't point to a valid nominal type, | |||
There was a problem hiding this comment.
The codable_codingkeys_type_is_not_an_enum_here diagnostic seems misleading, because we never actually check that codingKeysDecl is an enum.
There was a problem hiding this comment.
We do that below on line 318
| codingKeysTypeDecl->getLoc() : | ||
| cast<TypeDecl>(result)->getLoc(); | ||
|
|
||
| tc.diagnose(loc, diag::codable_codingkeys_type_does_not_conform_here, |
There was a problem hiding this comment.
Perhaps you can suppress this diagnostic if codingKeysTypeDecl->isInvalid(). In your test case you're emitting it twice which is not ideal
There was a problem hiding this comment.
If you conform to Codable, we go through synthesis twice — once for Decodable, and once for Encodable. There isn't a great way to signal that Decodable synthesis has gone through so Encodable synthesis shouldn't warn, and vice versa.
We can also suppress the diagnostic here if codingKeysTypeDecl is nullptr and just let the error message through. I do think it can be helpful to have the note as well to indicate that we at least looked at this decl, but it's not the end of the world either way.
| @@ -300,9 +300,17 @@ static CodingKeysValidity hasValidCodingKeysEnum(TypeChecker &tc, | |||
| if (!tc.conformsToProtocol(codingKeysType, codingKeyProto, | |||
There was a problem hiding this comment.
Do you need this part at all?
if (isa<TypeAliasDecl>(codingKeysTypeDecl))
codingKeysTypeDecl = codingKeysType->getAnyNominal();
You're not using codingKeysDecl below for anything, except to get its location. So why not always use the location of the original type to diagnose, even if it was a type alias?
There was a problem hiding this comment.
See also the check on line 318; we confirm that the decl itself is indeed an enum
|
@slavapestov @jrose-apple Any further feedback here? Happy to address any concerns. |
|
I'm good if Slava is. |
|
LGTM |
|
@swift-ci Please test and merge |
What's in this pull request?
Resolves a crash which would happen if your
Codabletype declared itsCodingKeysenum as a typealias to a nonexistent type. This allows us to diagnose on the usage of the undeclared identifier instead of crashing.Resolves rdar://problem/39021733.