Merge pull request #60044 from slavapestov/fix-two-opaque-return-type-bugs

Fix two opaque return type bugs

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -4215,6 +4215,9 @@ NOTE(opaque_type_underlying_type_candidate_here,none,
 ERROR(opaque_type_self_referential_underlying_type,none,
       "function opaque return type was inferred as %0, which defines the "
       "opaque type in terms of itself", (Type))
+ERROR(opaque_type_cannot_contain_dynamic_self,none,
+      "function with opaque return type cannot return "
+      "the covariant 'Self' type of a class", ())
 ERROR(opaque_type_var_no_init,none,
       "property declares an opaque return type, but has no initializer "
       "expression from which to infer an underlying type", ())

lib/SILGen/SILGenExpr.cpp

@@ -2129,11 +2129,9 @@ RValue RValueEmitter::visitUnderlyingToOpaqueExpr(UnderlyingToOpaqueExpr *E,
   if (value.getType() == opaqueTL.getLoweredType())
     return RValue(SGF, E, value);
 
-  auto cast = SGF.B.createUncheckedReinterpretCast(E, value.forward(SGF),
-                                                   opaqueTL.getLoweredType());
-  value = SGF.emitManagedRValueWithCleanup(cast);
-
-  return RValue(SGF, E, value);
+  auto cast = SGF.B.createUncheckedBitCast(E, value,
+                                           opaqueTL.getLoweredType());
+  return RValue(SGF, E, cast);
 }
 
 VarargsInfo Lowering::emitBeginVarargs(SILGenFunction &SGF, SILLocation loc,

lib/Sema/CSApply.cpp

@@ -6875,6 +6875,60 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
     }
   }
 
+  // Use an opaque type to abstract a value of the underlying concrete type.
+  // The full check here would be that `toType` and `fromType` are structurally
+  // equal except in any position where `toType` has an opaque archetype. The
+  // below is just an approximate check since the above would be expensive to
+  // verify and still relies on the type checker ensuing `fromType` is
+  // compatible with any opaque archetypes.
+  if (toType->hasOpaqueArchetype() &&
+      cs.getConstraintLocator(locator)->isForContextualType()) {
+    // Find the opaque type declaration. We need its generic signature.
+    OpaqueTypeDecl *opaqueDecl = nullptr;
+    bool found = toType.findIf([&](Type type) {
+      if (auto opaqueType = type->getAs<OpaqueTypeArchetypeType>()) {
+        opaqueDecl = opaqueType->getDecl();
+        return true;
+      }
+
+      return false;
+    });
+    (void)found;
+    assert(found && "No opaque type archetype?");
+
+    // Compute the substitutions for the opaque type declaration.
+    auto opaqueLocator = solution.getConstraintSystem().getOpenOpaqueLocator(
+        locator, opaqueDecl);
+    SubstitutionMap substitutions = solution.computeSubstitutions(
+        opaqueDecl->getOpaqueInterfaceGenericSignature(), opaqueLocator);
+
+    // If we don't have substitutions, this is an opaque archetype from
+    // another declaration being manipulated, and not an erasure of a
+    // concrete type to an opaque type inside its defining declaration.
+    if (!substitutions.empty()) {
+      // Compute the underlying type by replacing all opaque archetypes with
+      // the fixed type of their opened type.
+      auto underlyingType = toType.subst(
+        [&](SubstitutableType *type) -> Type {
+          if (auto *opaqueType = type->getAs<OpaqueTypeArchetypeType>()) {
+            if (opaqueType->getDecl() == opaqueDecl) {
+              return opaqueType->getInterfaceType().subst(substitutions);
+            }
+          }
+          return type;
+        },
+        LookUpConformanceInModule(cs.DC->getParentModule()),
+        SubstFlags::SubstituteOpaqueArchetypes);
+
+      // Coerce the result expression to the underlying type.
+      // FIXME: Wrong locator?
+      auto *subExpr = coerceToType(expr, underlyingType, locator);
+
+      return cs.cacheType(
+          new (ctx) UnderlyingToOpaqueExpr(subExpr, toType, substitutions));
+    }
+  }
+
   // Handle "from specific" coercions before "catch all" coercions.
   auto desugaredFromType = fromType->getDesugaredType();
   switch (desugaredFromType->getKind()) {
@@ -7260,36 +7314,6 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
   if (fromType->hasUnresolvedType() || toType->hasUnresolvedType())
     return cs.cacheType(new (ctx) UnresolvedTypeConversionExpr(expr, toType));
 
-  // Use an opaque type to abstract a value of the underlying concrete type.
-  // The full check here would be that `toType` and `fromType` are structurally
-  // equal except in any position where `toType` has an opaque archetype. The
-  // below is just an approximate check since the above would be expensive to
-  // verify and still relies on the type checker ensuing `fromType` is
-  // compatible with any opaque archetypes.
-  if (toType->hasOpaqueArchetype()) {
-    // Find the opaque type declaration. We need its generic signature.
-    OpaqueTypeDecl *opaqueDecl = nullptr;
-    bool found = toType.findIf([&](Type type) {
-      if (auto opaqueType = type->getAs<OpaqueTypeArchetypeType>()) {
-        opaqueDecl = opaqueType->getDecl();
-        return true;
-      }
-
-      return false;
-    });
-    (void)found;
-    assert(found && "No opaque type archetype?");
-
-    // Compute the substitutions for the opaque type declaration.
-    auto opaqueLocator = solution.getConstraintSystem().getOpenOpaqueLocator(
-        locator, opaqueDecl);
-    SubstitutionMap substitutions = solution.computeSubstitutions(
-        opaqueDecl->getOpaqueInterfaceGenericSignature(), opaqueLocator);
-    assert(!substitutions.empty() && "Missing substitutions for opaque type");
-    return cs.cacheType(
-        new (ctx) UnderlyingToOpaqueExpr(expr, toType, substitutions));
-  }
-
   llvm_unreachable("Unhandled coercion");
 }
 

lib/Sema/MiscDiagnostics.cpp

@@ -2846,20 +2846,26 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
 
   std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
     if (auto underlyingToOpaque = dyn_cast<UnderlyingToOpaqueExpr>(E)) {
-      auto key =
-          underlyingToOpaque->substitutions.getCanonical().getOpaqueValue();
+      auto subMap = underlyingToOpaque->substitutions;
 
+      auto key = subMap.getCanonical().getOpaqueValue();
       auto isUnique = UniqueSignatures.insert(key).second;
 
       auto candidate =
-          std::make_tuple(underlyingToOpaque->getSubExpr(),
-                          underlyingToOpaque->substitutions, isUnique);
+          std::make_tuple(underlyingToOpaque->getSubExpr(), subMap, isUnique);
 
       if (isSelfReferencing(candidate)) {
         HasInvalidReturn = true;
         return {false, nullptr};
       }
 
+      if (subMap.hasDynamicSelf()) {
+        Ctx.Diags.diagnose(E->getLoc(),
+                           diag::opaque_type_cannot_contain_dynamic_self);
+        HasInvalidReturn = true;
+        return {false, nullptr};
+      }
+
       Candidates.push_back({CurrentAvailability, candidate});
       return {false, E};
     }

test/SILGen/opaque_result_type_structural.swift

@@ -0,0 +1,38 @@
+// RUN: %target-swift-emit-silgen -disable-availability-checking %s
+
+public protocol P {}
+
+extension P {
+  // This will only typecheck and SILGen if we emit the UnderlyingToOpaqueExpr
+  // instead of a MetatypeConversionExpr
+  public func f1() -> (some P).Type {
+    return Self.self
+  }
+
+  // This will only typecheck and SILGen if we emit the UnderlyingToOpaqueExpr
+  // instead of a FunctionConversionExpr
+  public func f2() -> () -> (some P) {
+    let fn = { self }
+    return fn
+  }
+
+  // FIXME: This still doesn't work because the '() -> (some P)' propagates
+  // backwards into the closure literal's return type
+  /* public func f3() -> () -> (some P) {
+    return { self }
+  } */
+
+  // Optionals already worked, but let's just be sure
+  public func f4() -> (some P)? {
+    return self
+  }
+
+  // Make sure we can handle a function conversion *and* opaque type erasure;
+  // here we first convert (Any) -> (Self) to (Int) -> (Self) before erasing
+  // to (Int) -> (some P)
+  public func f5() -> (Int) -> (some P) {
+    let fn: (Any) -> (Self) = { _ in self }
+    return fn
+  }
+}
+

test/type/opaque_return_dynamic_self.swift

@@ -0,0 +1,36 @@
+// RUN: %target-typecheck-verify-swift -disable-availability-checking
+
+protocol P {
+  init()
+}
+
+extension P {
+  static func opaqueProtocolExtensionFunc() -> some P { return Self() }
+}
+
+class C: P {
+  required init() {}
+
+  static func opaqueFunc1() -> some P {
+    return Self()
+    // expected-error@-1 {{function with opaque return type cannot return the covariant 'Self' type of a class}}
+  }
+
+  static func opaqueFunc2() -> some P {
+    return Self.opaqueProtocolExtensionFunc()
+    // expected-error@-1 {{function with opaque return type cannot return the covariant 'Self' type of a class}}
+  }
+
+  var opaqueProperty: some P {
+    return Self()
+    // expected-error@-1 {{function with opaque return type cannot return the covariant 'Self' type of a class}}
+  }
+
+  subscript() -> some P {
+    return Self()
+    // expected-error@-1 {{function with opaque return type cannot return the covariant 'Self' type of a class}}
+  }
+
+  var opaqueStoredProperty: some P = Self()
+  // expected-error@-1 {{covariant 'Self' type cannot be referenced from a stored property initializer}}
+}