Sema: Fix failure to emit a diagnostic when a protocol witness is being validated already

Validating a declaration can trigger conformance checking. If the conformance checker
comes across the same declaration as a candidate witness, it would fail to emit a
diagnostic. As a result we would then go onto SILGen, which would crash while emitting
a witness table with a missing entry.

Fixes <rdar://problem/45151902>.

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -1875,6 +1875,9 @@ NOTE(protocol_witness_nonconform_type,none,
      "possibly intended match %0 does not "
      "%select{inherit from|conform to}2 %1", (Type, Type, bool))
 
+NOTE(protocol_witness_circularity,none,
+     "candidate references itself", ())
+
 NOTE(protocol_conformance_here,none,
      "%select{|class }0%1 declares conformance to protocol %2 here",
      (bool, DeclName, DeclName))
@@ -1982,8 +1985,8 @@ ERROR(requires_generic_params_made_equal,none,
 ERROR(requires_generic_param_made_equal_to_concrete,none,
       "same-type requirement makes generic parameter %0 non-generic",
       (Type))
-ERROR(recursive_type_reference,none,
-      "%0 %1 references itself", (DescriptiveDeclKind, Identifier))
+ERROR(recursive_decl_reference,none,
+      "%0 %1 references itself", (DescriptiveDeclKind, DeclBaseName))
 ERROR(recursive_same_type_constraint,none,
       "same-type constraint %0 == %1 is recursive", (Type, Type))
 ERROR(recursive_superclass_constraint,none,

lib/Sema/TypeCheckDecl.cpp

@@ -3918,7 +3918,7 @@ void TypeChecker::validateDecl(ValueDecl *D) {
             });
 
         if (mentionsItself) {
-          diagnose(defaultDefinition.getLoc(), diag::recursive_type_reference,
+          diagnose(defaultDefinition.getLoc(), diag::recursive_decl_reference,
                    assocType->getDescriptiveKind(), assocType->getName());
           diagnose(assocType, diag::kind_declared_here, DescriptiveDeclKind::Type);
         }

lib/Sema/TypeCheckDeclObjC.cpp

@@ -760,7 +760,7 @@ bool swift::isRepresentableInObjC(const VarDecl *VD, ObjCReason Reason) {
     VD->getASTContext().getLazyResolver()->resolveDeclSignature(
                                               const_cast<VarDecl *>(VD));
     if (!VD->hasInterfaceType()) {
-      VD->diagnose(diag::recursive_type_reference, VD->getDescriptiveKind(),
+      VD->diagnose(diag::recursive_decl_reference, VD->getDescriptiveKind(),
                    VD->getName());
       return false;
     }

lib/Sema/TypeCheckProtocol.cpp

@@ -440,9 +440,12 @@ swift::matchWitness(
     return RequirementMatch(witness, MatchKind::KindConflict);
 
   // If the witness is invalid, record that and stop now.
-  if (witness->isInvalid() || !witness->hasValidSignature())
+  if (witness->isInvalid())
     return RequirementMatch(witness, MatchKind::WitnessInvalid);
 
+  if (!witness->hasValidSignature())
+    return RequirementMatch(witness, MatchKind::Circularity);
+
   // Get the requirement and witness attributes.
   const auto &reqAttrs = req->getAttrs();
   const auto &witnessAttrs = witness->getAttrs();
@@ -1146,15 +1149,9 @@ bool WitnessChecker::findBestWitness(
         continue;
       }
 
-      if (!witness->hasValidSignature()) {
+      if (!witness->hasValidSignature())
         TC.validateDecl(witness);
 
-        if (!witness->hasValidSignature()) {
-          doNotDiagnoseMatches = true;
-          continue;
-        }
-      }
-
       auto match = matchWitness(TC, ReqEnvironmentCache, Proto, conformance, DC,
                                 requirement, witness);
       if (match.isViable()) {
@@ -2130,6 +2127,10 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance,
     // about them.
     break;
 
+  case MatchKind::Circularity:
+    diags.diagnose(match.Witness, diag::protocol_witness_circularity);
+    break;
+
   case MatchKind::TypeConflict: {
     auto diag = diags.diagnose(match.Witness, 
                                diag::protocol_witness_type_conflict,

lib/Sema/TypeCheckProtocol.h

@@ -166,6 +166,11 @@ enum class MatchKind : uint8_t {
   /// The witness is invalid or has an invalid type.
   WitnessInvalid,
 
+  /// The witness is currently being type checked and this type checking in turn
+  /// triggered conformance checking, so the witness cannot be considered as a
+  /// candidate.
+  Circularity,
+
   /// The kind of the witness and requirement differ, e.g., one
   /// is a function and the other is a variable.
   KindConflict,
@@ -405,6 +410,7 @@ struct RequirementMatch {
       return true;
 
     case MatchKind::WitnessInvalid:
+    case MatchKind::Circularity:
     case MatchKind::KindConflict:
     case MatchKind::TypeConflict:
     case MatchKind::MissingRequirement:
@@ -435,6 +441,7 @@ struct RequirementMatch {
       return true;
 
     case MatchKind::WitnessInvalid:
+    case MatchKind::Circularity:
     case MatchKind::KindConflict:
     case MatchKind::StaticNonStaticConflict:
     case MatchKind::SettableConflict:

lib/Sema/TypeCheckType.cpp

@@ -700,7 +700,7 @@ Type TypeChecker::applyGenericArguments(Type type,
 
   // FIXME: More principled handling of circularity.
   if (!genericDecl->hasValidSignature()) {
-    diags.diagnose(loc, diag::recursive_type_reference,
+    diags.diagnose(loc, diag::recursive_decl_reference,
              genericDecl->getDescriptiveKind(), genericDecl->getName());
     genericDecl->diagnose(diag::kind_declared_here, DescriptiveDeclKind::Type);
     return ErrorType::get(ctx);
@@ -922,7 +922,7 @@ static Type resolveTypeDecl(TypeDecl *typeDecl, SourceLoc loc,
 
     // If we were not able to validate recursively, bail out.
     if (!typeDecl->hasInterfaceType()) {
-      diags.diagnose(loc, diag::recursive_type_reference,
+      diags.diagnose(loc, diag::recursive_decl_reference,
                   typeDecl->getDescriptiveKind(), typeDecl->getName());
       typeDecl->diagnose(diag::kind_declared_here,
                          DescriptiveDeclKind::Type);

test/decl/protocol/conforms/circular_validation.swift

@@ -0,0 +1,13 @@
+// RUN: %target-typecheck-verify-swift
+
+// With a bit of effort, we could make this work -- but for now, let's just
+// not crash.
+
+protocol P {
+  var x: Int { get set } // expected-note {{protocol requires property 'x' with type 'Int'; do you want to add a stub?}}
+}
+
+struct S : P { // expected-error {{type 'S' does not conform to protocol 'P'}}
+  static var x = 0 // expected-note {{candidate operates on a type, not an instance as required}}
+  var x = S.x // expected-note {{candidate references itself}}
+}