Undo Bogus DiagnosticTransactions in Codable Synthesis

The order of diagnostic emission absolutely does not matter. What this transaction was actually doing was suppressing valid diagnostics. This is a deeply unsound thing to do since if errors are emitted but Codable synthesis succeeds then invalid code can make its way past Sema.

rdar://74392492

Author: CodaFi

lib/Sema/DerivedConformanceCodable.cpp

@@ -1058,36 +1058,19 @@ ValueDecl *DerivedConformance::deriveEncodable(ValueDecl *requirement) {
   if (checkAndDiagnoseDisallowedContext(requirement))
     return nullptr;
 
-  // We're about to try to synthesize Encodable. If something goes wrong,
-  // we'll have to output at least one error diagnostic because we returned
-  // true from NominalTypeDecl::derivesProtocolConformance; if we don't, we're
-  // expected to return a witness here later (and we crash on an assertion).
-  // Producing a diagnostic stops compilation before then.
-  //
-  // A synthesis attempt will produce NOTE diagnostics throughout, but we'll
-  // want to collect them before displaying -- we want NOTEs to display
-  // _after_ a main diagnostic so we don't get a NOTE before the error it
-  // relates to.
-  //
-  // We can do this with a diagnostic transaction -- first collect failure
-  // diagnostics, then potentially collect notes. If we succeed in
-  // synthesizing Encodable, we can cancel the transaction and get rid of the
-  // fake failures.
-  DiagnosticTransaction diagnosticTransaction(Context.Diags);
-  ConformanceDecl->diagnose(diag::type_does_not_conform,
-                            Nominal->getDeclaredType(), getProtocolType());
-  requirement->diagnose(diag::no_witnesses, diag::RequirementKind::Func,
-                        requirement->getName(), getProtocolType(),
-                        /*AddFixIt=*/false);
-
   // Check other preconditions for synthesized conformance.
   // This synthesizes a CodingKeys enum if possible.
-  if (canSynthesize(*this, requirement)) {
-    diagnosticTransaction.abort();
-    return deriveEncodable_encode(*this);
+  if (!canSynthesize(*this, requirement)) {
+    ConformanceDecl->diagnose(diag::type_does_not_conform,
+                              Nominal->getDeclaredType(), getProtocolType());
+    requirement->diagnose(diag::no_witnesses, diag::RequirementKind::Func,
+                          requirement->getName(), getProtocolType(),
+                          /*AddFixIt=*/false);
+
+    return nullptr;
   }
 
-  return nullptr;
+  return deriveEncodable_encode(*this);
 }
 
 ValueDecl *DerivedConformance::deriveDecodable(ValueDecl *requirement) {
@@ -1104,24 +1087,17 @@ ValueDecl *DerivedConformance::deriveDecodable(ValueDecl *requirement) {
   if (checkAndDiagnoseDisallowedContext(requirement))
     return nullptr;
 
-  // We're about to try to synthesize Decodable. If something goes wrong,
-  // we'll have to output at least one error diagnostic. We need to collate
-  // diagnostics produced by canSynthesize and deriveDecodable_init to produce
-  // them in the right order -- see the comment in deriveEncodable for
-  // background on this transaction.
-  DiagnosticTransaction diagnosticTransaction(Context.Diags);
-  ConformanceDecl->diagnose(diag::type_does_not_conform,
-                            Nominal->getDeclaredType(), getProtocolType());
-  requirement->diagnose(diag::no_witnesses, diag::RequirementKind::Constructor,
-                        requirement->getName(), getProtocolType(),
-                        /*AddFixIt=*/false);
-
   // Check other preconditions for synthesized conformance.
   // This synthesizes a CodingKeys enum if possible.
-  if (canSynthesize(*this, requirement)) {
-    diagnosticTransaction.abort();
-    return deriveDecodable_init(*this);
+  if (!canSynthesize(*this, requirement)) {
+    ConformanceDecl->diagnose(diag::type_does_not_conform,
+                              Nominal->getDeclaredType(), getProtocolType());
+    requirement->diagnose(diag::no_witnesses, diag::RequirementKind::Constructor,
+                          requirement->getName(), getProtocolType(),
+                          /*AddFixIt=*/false);
+
+    return nullptr;
   }
 
-  return nullptr;
+  return deriveDecodable_init(*this);
 }

test/decl/protocol/special/coding/class_codable_nonconforming_property.swift

@@ -165,11 +165,10 @@ class NonConformingClass : Codable { // expected-error {{type 'NonConformingClas
   // These lines have to be within the NonConformingClass type because
   // CodingKeys should be private.
   func foo() {
-    // They should not get a CodingKeys type.
-    let _ = NonConformingClass.CodingKeys.self // expected-error {{type 'NonConformingClass' has no member 'CodingKeys'}}
-    let _ = NonConformingClass.CodingKeys.x // expected-error {{type 'NonConformingClass' has no member 'CodingKeys'}}
-    let _ = NonConformingClass.CodingKeys.y // expected-error {{type 'NonConformingClass' has no member 'CodingKeys'}}
-    let _ = NonConformingClass.CodingKeys.z // expected-error {{type 'NonConformingClass' has no member 'CodingKeys'}}
+    let _ = NonConformingClass.CodingKeys.self
+    let _ = NonConformingClass.CodingKeys.x
+    let _ = NonConformingClass.CodingKeys.y
+    let _ = NonConformingClass.CodingKeys.z // expected-error {{type 'NonConformingClass.CodingKeys' has no member 'z'}}
   }
 }
 
@@ -178,4 +177,4 @@ let _ = NonConformingClass.init(from:) // expected-error {{type 'NonConformingCl
 let _ = NonConformingClass.encode(to:) // expected-error {{type 'NonConformingClass' has no member 'encode(to:)'}}
 
 // They should not get a CodingKeys type.
-let _ = NonConformingClass.CodingKeys.self // expected-error {{type 'NonConformingClass' has no member 'CodingKeys'}}
+let _ = NonConformingClass.CodingKeys.self // expected-error {{'CodingKeys' is inaccessible due to 'private' protection level}}

test/decl/protocol/special/coding/struct_codable_nonconforming_property.swift

@@ -165,11 +165,10 @@ struct NonConformingStruct : Codable { // expected-error {{type 'NonConformingSt
   // These lines have to be within the NonConformingStruct type because
   // CodingKeys should be private.
   func foo() {
-    // They should not get a CodingKeys type.
-    let _ = NonConformingStruct.CodingKeys.self // expected-error {{type 'NonConformingStruct' has no member 'CodingKeys'}}
-    let _ = NonConformingStruct.CodingKeys.x // expected-error {{type 'NonConformingStruct' has no member 'CodingKeys'}}
-    let _ = NonConformingStruct.CodingKeys.y // expected-error {{type 'NonConformingStruct' has no member 'CodingKeys'}}
-    let _ = NonConformingStruct.CodingKeys.z // expected-error {{type 'NonConformingStruct' has no member 'CodingKeys'}}
+    let _ = NonConformingStruct.CodingKeys.self
+    let _ = NonConformingStruct.CodingKeys.x
+    let _ = NonConformingStruct.CodingKeys.y
+    let _ = NonConformingStruct.CodingKeys.z // expected-error {{type 'NonConformingStruct.CodingKeys' has no member 'z'}}
   }
 }
 
@@ -178,4 +177,4 @@ let _ = NonConformingStruct.init(from:) // expected-error {{type 'NonConformingS
 let _ = NonConformingStruct.encode(to:) // expected-error {{type 'NonConformingStruct' has no member 'encode(to:)'}}
 
 // They should not get a CodingKeys type.
-let _ = NonConformingStruct.CodingKeys.self // expected-error {{type 'NonConformingStruct' has no member 'CodingKeys'}}
+let _ = NonConformingStruct.CodingKeys.self // expected-error {{'CodingKeys' is inaccessible due to 'private' protection level}}

test/decl/protocol/special/coding/struct_codable_simple.swift

@@ -31,19 +31,19 @@ let _ = SimpleStruct.encode(to:)
 let _ = SimpleStruct.CodingKeys.self // expected-error {{'CodingKeys' is inaccessible due to 'private' protection level}}
 
 // rdar://problem/59655704 
-struct SR_12248_1: Codable { // expected-error {{type 'SR_12248_1' does not conform to protocol 'Encodable'}}
+struct SR_12248_1: Codable { // expected-error {{type 'SR_12248_1' does not conform to protocol 'Encodable'}} expected-error {{type 'SR_12248_1' does not conform to protocol 'Decodable'}}
   var x: Int // expected-note {{'x' previously declared here}}
   var x: Int // expected-error {{invalid redeclaration of 'x'}}
   // expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'Int' does not conform to 'Encodable'}}
   // expected-note@-2 {{cannot automatically synthesize 'Encodable' because 'Int' does not conform to 'Encodable'}}
 }
 
-struct SR_12248_2: Decodable { 
+struct SR_12248_2: Decodable { // expected-error {{type 'SR_12248_2' does not conform to protocol 'Decodable'}}
   var x: Int // expected-note {{'x' previously declared here}}
   var x: Int // expected-error {{invalid redeclaration of 'x'}}
 }
 
-struct SR_12248_3: Encodable { 
+struct SR_12248_3: Encodable { // expected-error {{type 'SR_12248_3' does not conform to protocol 'Encodable'}}
   var x: Int // expected-note {{'x' previously declared here}}
   var x: Int // expected-error {{invalid redeclaration of 'x'}}
 }

test/diagnostics/pretty-printed-diagnostics.swift

@@ -136,8 +136,6 @@ foo(b:
 // CHECK: [[#LINE]]   | struct B: Decodable {
 // CHECK:             |        ^ error: type 'B' does not conform to protocol 'Decodable'
 // CHECK: [[#LINE+1]] |   let a: Foo
-// CHECK:             |       ^ note: cannot automatically synthesize 'Decodable' because 'Foo' does not conform to 'Decodable'
-// CHECK: [[#LINE+2]] | }
 // CHECK: Swift.Decodable:2:5
 // CHECK: 1 | public protocol Decodable {
 // CHECK: 2 |     init(from decoder: Decoder) throws