Sema: New opaque return type circularity check that doesn't trigger lazy type checking of bodies

Commit b70f8a8 introduced a usage of
ReplaceOpaqueTypesWithUnderlyingTypes in Sema. Previously this was only
called from SILGen.

The problem was that ReplaceOpaqueTypesWithUnderlyingTypes would call
getUniqueUnderlyingTypeSubstitutions(), which triggers a request to
type check the body of the referenced function.

While this didn't result in unnecessary type checking work, because
UniqueUnderlyingTypeSubstitutionsRequest::evaluate() would skip bodies
in secondary files, it did change declaration checking order.

The specific issue we saw was a bad interaction with associated type
inference and unqualified lookup in a WMO build, and a complete test
case is hard to reduce here.

However, no behavior change is intended with this change, modulo bugs
elsewhere related to declaration checking order, so I feel OK not adding
a test case.

I'll hopefully address the unqualified lookup issue exposed in the
radar soon; it has a reproducer independent of opaque return types.

Fixes rdar://157329046.

Author: slavapestov

lib/Sema/MiscDiagnostics.cpp

@@ -27,7 +27,6 @@
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/Expr.h"
-#include "swift/AST/InFlightSubstitution.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/NameLookupRequests.h"
 #include "swift/AST/Pattern.h"
@@ -3666,50 +3665,6 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
     }
   }
 
-  bool isSelfReferencing(const Candidate &candidate) {
-    auto substitutions = std::get<1>(candidate);
-
-    // The underlying type can't be defined recursively
-    // in terms of the opaque type itself.
-    for (auto genericParam : OpaqueDecl->getOpaqueGenericParams()) {
-      auto underlyingType = Type(genericParam).subst(substitutions);
-
-      // Look through underlying types of other opaque archetypes known to
-      // us. This is not something the type checker is allowed to do in
-      // general, since the intent is that the underlying type is completely
-      // hidden from view at the type system level. However, here we're
-      // trying to catch recursive underlying types before we proceed to
-      // SIL, so we specifically want to erase opaque archetypes just
-      // for the purpose of this check.
-      ReplaceOpaqueTypesWithUnderlyingTypes replacer(
-          OpaqueDecl->getDeclContext(),
-          ResilienceExpansion::Maximal,
-          /*isWholeModuleContext=*/false);
-      InFlightSubstitution IFS(replacer, replacer,
-                               SubstFlags::SubstituteOpaqueArchetypes |
-                               SubstFlags::PreservePackExpansionLevel);
-      auto simplifiedUnderlyingType = underlyingType.subst(IFS);
-
-      auto isSelfReferencing =
-          (IFS.wasLimitReached() ||
-           simplifiedUnderlyingType.findIf([&](Type t) -> bool {
-             if (auto *other = t->getAs<OpaqueTypeArchetypeType>()) {
-               return other->getDecl() == OpaqueDecl;
-             }
-             return false;
-           }));
-
-      if (isSelfReferencing) {
-        Ctx.Diags.diagnose(std::get<0>(candidate)->getLoc(),
-                           diag::opaque_type_self_referential_underlying_type,
-                           underlyingType);
-        return true;
-      }
-    }
-
-    return false;
-  }
-
   // A single unique underlying substitution.
   void finalizeUnique(const Candidate &candidate) {
     // If we have one successful candidate, then save it as the underlying
@@ -3808,11 +3763,6 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
       auto candidate =
           std::make_tuple(underlyingToOpaque->getSubExpr(), subMap, isUnique);
 
-      if (isSelfReferencing(candidate)) {
-        HasInvalidReturn = true;
-        return Action::Stop();
-      }
-
       if (subMap.getRecursiveProperties().hasDynamicSelf()) {
         Ctx.Diags.diagnose(E->getLoc(),
                            diag::opaque_type_cannot_contain_dynamic_self);

lib/Sema/TypeCheckGeneric.cpp

@@ -21,6 +21,7 @@
 #include "swift/AST/ASTWalker.h"
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
+#include "swift/AST/InFlightSubstitution.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/ProtocolConformance.h"
@@ -267,6 +268,53 @@ OpaqueResultTypeRequest::evaluate(Evaluator &evaluator,
   return opaqueDecl;
 }
 
+void TypeChecker::checkCircularOpaqueReturnTypeDecl(OpaqueTypeDecl *opaqueDecl) {
+  auto optSubs = opaqueDecl->getUniqueUnderlyingTypeSubstitutions(
+      /*typeCheckFunctionBodies=*/false);
+  if (!optSubs)
+    return;
+
+  auto substitutions = *optSubs;
+
+  // The underlying type can't be defined recursively
+  // in terms of the opaque type itself.
+  for (auto genericParam : opaqueDecl->getOpaqueGenericParams()) {
+    auto underlyingType = Type(genericParam).subst(substitutions);
+
+    // Look through underlying types of other opaque archetypes known to
+    // us. This is not something the type checker is allowed to do in
+    // general, since the intent is that the underlying type is completely
+    // hidden from view at the type system level. However, here we're
+    // trying to catch recursive underlying types before we proceed to
+    // SIL, so we specifically want to erase opaque archetypes just
+    // for the purpose of this check.
+    ReplaceOpaqueTypesWithUnderlyingTypes replacer(
+        opaqueDecl->getDeclContext(),
+        ResilienceExpansion::Maximal,
+        /*isWholeModuleContext=*/false,
+        /*typeCheckFunctionBodies=*/false);
+    InFlightSubstitution IFS(replacer, replacer,
+                             SubstFlags::SubstituteOpaqueArchetypes |
+                             SubstFlags::PreservePackExpansionLevel);
+    auto simplifiedUnderlyingType = underlyingType.subst(IFS);
+
+    auto isSelfReferencing =
+        (IFS.wasLimitReached() ||
+         simplifiedUnderlyingType.findIf([&](Type t) -> bool {
+           if (auto *other = t->getAs<OpaqueTypeArchetypeType>()) {
+             return other->getDecl() == opaqueDecl;
+           }
+           return false;
+         }));
+
+    if (isSelfReferencing) {
+      opaqueDecl->getNamingDecl()->diagnose(
+          diag::opaque_type_self_referential_underlying_type,
+          underlyingType);
+    }
+  }
+}
+
 static bool checkProtocolSelfRequirementsImpl(
     ASTContext &ctx, ProtocolDecl *proto, ValueDecl *decl,
     GenericSignature originalSig,

lib/Sema/TypeChecker.cpp

@@ -326,6 +326,10 @@ TypeCheckPrimaryFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
       }
     }
     SF->typeCheckDelayedFunctions();
+
+    for (auto *opaqueDecl : SF->getOpaqueReturnTypeDecls()) {
+      TypeChecker::checkCircularOpaqueReturnTypeDecl(opaqueDecl);
+    }
   }
 
   // If region-based isolation is enabled, we diagnose unnecessary

lib/Sema/TypeChecker.h

@@ -506,6 +506,8 @@ void typeCheckTopLevelCodeDecl(TopLevelCodeDecl *TLCD);
 
 void typeCheckDecl(Decl *D);
 
+void checkCircularOpaqueReturnTypeDecl(OpaqueTypeDecl *opaqueDecl);
+
 void addImplicitDynamicAttribute(Decl *D);
 void checkDeclAttributes(Decl *D);
 void checkDeclABIAttribute(Decl *apiDecl, ABIAttr *abiAttr);

test/Generics/infinite_opaque_result_type.swift

@@ -5,25 +5,25 @@ func concrete1() -> some Any {
   return concrete1()
 }
 
-func concrete2() -> some Any {
-  return [concrete2()]  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+func concrete2() -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+  return [concrete2()]
 }
 
 
-func concrete1a() -> some Any {
-  return concrete1b()  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
+func concrete1a() -> some Any {  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
+  return concrete1b()
 }
 
-func concrete1b() -> some Any {
+func concrete1b() -> some Any {  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
   return concrete1a()
 }
 
 
-func concrete2a() -> some Any {
-  return [concrete2b()]  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+func concrete2a() -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+  return [concrete2b()]
 }
 
-func concrete2b() -> some Any {
+func concrete2b() -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
   return [concrete2a()]
 }
 
@@ -33,41 +33,41 @@ func generic1<T>(_ t: T) -> some Any {
   return generic1(t)
 }
 
-func generic2<T>(_ t: T) -> some Any {
-  return [generic2(t)]  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+func generic2<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+  return [generic2(t)]
 }
 
 
-func generic1a<T>(_ t: T) -> some Any {
-  return generic1b(t)  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
+func generic1a<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
+  return generic1b(t)
 }
 
-func generic1b<T>(_ t: T) -> some Any {
+func generic1b<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as 'some Any', which defines the opaque type in terms of itself}}
   return generic1a(t)
 }
 
 
-func generic2a<T>(_ t: T) -> some Any {
-  return [generic2b(t)]  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+func generic2a<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+  return [generic2b(t)]
 }
 
-func generic2b<T>(_ t: T) -> some Any {
+func generic2b<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
   return [generic2a(t)]
 }
 
 
-func generic3a<T>(_ t: T) -> some Any {
-  return [generic3b(t)]  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+func generic3a<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
+  return [generic3b(t)]
 }
 
-func generic3b<T>(_ t: T) -> some Any {
+func generic3b<T>(_ t: T) -> some Any {  // expected-error {{function opaque return type was inferred as '[some Any]', which defines the opaque type in terms of itself}}
   return [generic3a([t])]
 }
 
-func very_wide1() -> some Any {
-  return (very_wide2(), very_wide2())  // expected-error {{function opaque return type was inferred as '(some Any, some Any)', which defines the opaque type in terms of itself}}
+func very_wide1() -> some Any {  // expected-error {{function opaque return type was inferred as '(some Any, some Any)', which defines the opaque type in terms of itself}}
+  return (very_wide2(), very_wide2())
 }
 
-func very_wide2() -> some Any {
+func very_wide2() -> some Any {  // expected-error {{function opaque return type was inferred as '(some Any, some Any)', which defines the opaque type in terms of itself}}
   return (very_wide1(), very_wide1())
 }