Merge pull request #24393 from xedin/rdar-49582531-5.1-04-24

[5.1 04/24][Diagnostics] Improve missing conformance diagnostics for opaque return

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -51,6 +51,8 @@ NOTE(implicit_member_declared_here,none,
      "%1 '%0' is implicitly declared", (StringRef, StringRef))
 NOTE(extended_type_declared_here,none,
      "extended type declared here", ())
+NOTE(opaque_return_type_declared_here,none,
+     "opaque return type declared here", ())
 
 //------------------------------------------------------------------------------
 // MARK: Constraint solver diagnostics
@@ -1585,6 +1587,9 @@ ERROR(type_does_not_conform_in_decl_ref,none,
 ERROR(type_does_not_conform_decl_owner,none,
       "%0 %1 requires that %2 conform to %3",
       (DescriptiveDeclKind, DeclName, Type, Type))
+ERROR(type_does_not_conform_in_opaque_return,none,
+      "return type of %0 %1 requires that %2 conform to %3",
+      (DescriptiveDeclKind, DeclName, Type, Type))
 ERROR(types_not_equal_decl,none,
       "%0 %1 requires the types %2 and %3 be equivalent",
       (DescriptiveDeclKind, DeclName, Type, Type))

lib/Sema/CSDiagnostics.cpp

@@ -155,12 +155,25 @@ ValueDecl *RequirementFailure::getDeclRef() const {
   auto *anchor = getRawAnchor();
   auto *locator = cs.getConstraintLocator(anchor);
 
-  if (isFromContextualType()) {
-    auto type = cs.getContextualType();
+  // Get a declaration associated with given type (if any).
+  // This is used to retrieve affected declaration when
+  // failure is in any way contextual, and declaration can't
+  // be fetched directly from constraint system.
+  auto getAffectedDeclFromType = [](Type type) -> ValueDecl * {
     assert(type);
-    auto *alias = dyn_cast<TypeAliasType>(type.getPointer());
-    return alias ? alias->getDecl() : type->getAnyGeneric();
-  }
+    // If problem is related to a typealias, let's point this
+    // diagnostic directly to its declaration without desugaring.
+    if (auto *alias = dyn_cast<TypeAliasType>(type.getPointer()))
+      return alias->getDecl();
+
+    if (auto *opaque = type->getAs<OpaqueTypeArchetypeType>())
+      return opaque->getDecl();
+
+    return type->getAnyGeneric();
+  };
+
+  if (isFromContextualType())
+    return getAffectedDeclFromType(cs.getContextualType());
 
   if (auto *AE = dyn_cast<CallExpr>(anchor)) {
     // NOTE: In valid code, the function can only be a TypeExpr
@@ -196,11 +209,7 @@ ValueDecl *RequirementFailure::getDeclRef() const {
   if (overload)
     return overload->choice.getDecl();
 
-  auto ownerType = getOwnerType();
-  if (auto *NA = dyn_cast<TypeAliasType>(ownerType.getPointer()))
-    return NA->getDecl();
-
-  return ownerType->getAnyGeneric();
+  return getAffectedDeclFromType(getOwnerType());
 }
 
 GenericSignature *RequirementFailure::getSignature(ConstraintLocator *locator) {
@@ -263,6 +272,28 @@ bool RequirementFailure::diagnoseAsError() {
   auto lhs = resolveType(getLHS());
   auto rhs = resolveType(getRHS());
 
+  if (auto *OTD = dyn_cast<OpaqueTypeDecl>(AffectedDecl)) {
+    auto *namingDecl = OTD->getNamingDecl();
+    emitDiagnostic(
+        anchor->getLoc(), diag::type_does_not_conform_in_opaque_return,
+        namingDecl->getDescriptiveKind(), namingDecl->getFullName(), lhs, rhs);
+
+    TypeLoc returnLoc;
+    if (auto *VD = dyn_cast<VarDecl>(namingDecl)) {
+      returnLoc = VD->getTypeLoc();
+    } else if (auto *FD = dyn_cast<FuncDecl>(namingDecl)) {
+      returnLoc = FD->getBodyResultTypeLoc();
+    } else if (auto *SD = dyn_cast<SubscriptDecl>(namingDecl)) {
+      returnLoc = SD->getElementTypeLoc();
+    }
+
+    if (returnLoc.hasLocation()) {
+      emitDiagnostic(returnLoc.getLoc(), diag::opaque_return_type_declared_here)
+          .highlight(returnLoc.getSourceRange());
+    }
+    return true;
+  }
+
   if (genericCtx != reqDC && (genericCtx->isChildContextOf(reqDC) ||
                               isStaticOrInstanceMember(AffectedDecl))) {
     auto *NTD = reqDC->getSelfNominalTypeDecl();

lib/Sema/MiscDiagnostics.cpp

@@ -2257,6 +2257,9 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
   AbstractFunctionDecl *Implementation;
   OpaqueTypeDecl *OpaqueDecl;
   SmallVector<std::pair<Expr*, Type>, 4> Candidates;
+
+  bool HasInvalidReturn = false;
+
 public:
   OpaqueUnderlyingTypeChecker(TypeChecker &TC,
                               AbstractFunctionDecl *Implementation,
@@ -2270,7 +2273,13 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
 
   void check() {
     Implementation->getBody()->walk(*this);
-    
+
+    // If given function has any invalid returns in the body
+    // let's not try to validate the types, since it wouldn't
+    // be accurate.
+    if (HasInvalidReturn)
+      return;
+
     // If there are no candidates, then the body has no return statements, and
     // we have nothing to infer the underlying type from.
     if (Candidates.empty()) {
@@ -2349,6 +2358,20 @@ class OpaqueUnderlyingTypeChecker : public ASTWalker {
     }
     return std::make_pair(false, E);
   }
+
+  std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
+    if (auto *RS = dyn_cast<ReturnStmt>(S)) {
+      if (RS->hasResult()) {
+        auto resultTy = RS->getResult()->getType();
+        // If expression associated with return statement doesn't have
+        // a type or type has an error, checking opaque types is going
+        // to produce incorrect diagnostics.
+        HasInvalidReturn |= resultTy.isNull() || resultTy->hasError();
+      }
+    }
+
+    return {true, S};
+  }
 };
 
 } // end anonymous namespace

test/type/opaque.swift

@@ -313,3 +313,45 @@ struct RedeclarationTest {
   subscript(redeclared _: Int) -> some Q { return 0 }
   subscript(redeclared _: Int) -> P { return 0 }
 }
+
+func diagnose_requirement_failures() {
+  struct S {
+    var foo: some P { return S() } // expected-note {{declared here}}
+    // expected-error@-1 {{return type of property 'foo' requires that 'S' conform to 'P'}}
+
+    subscript(_: Int) -> some P { // expected-note {{declared here}}
+      return S()
+      // expected-error@-1 {{return type of subscript 'subscript(_:)' requires that 'S' conform to 'P'}}
+    }
+
+    func bar() -> some P { // expected-note {{declared here}}
+      return S()
+      // expected-error@-1 {{return type of instance method 'bar()' requires that 'S' conform to 'P'}}
+    }
+
+    static func baz(x: String) -> some P { // expected-note {{declared here}}
+      return S()
+      // expected-error@-1 {{return type of static method 'baz(x:)' requires that 'S' conform to 'P'}}
+    }
+  }
+
+  func fn() -> some P { // expected-note {{declared here}}
+    return S()
+    // expected-error@-1 {{return type of local function 'fn()' requires that 'S' conform to 'P'}}
+  }
+}
+
+func global_function_with_requirement_failure() -> some P { // expected-note {{declared here}}
+  return 42 as Double
+  // expected-error@-1 {{return type of global function 'global_function_with_requirement_failure()' requires that 'Double' conform to 'P'}}
+}
+
+func recursive_func_is_invalid_opaque() {
+  func rec(x: Int) -> some P {
+    // expected-error@-1 {{function declares an opaque return type, but has no return statements in its body from which to infer an underlying type}}
+    if x == 0 {
+      return rec(x: 0)
+    }
+    return rec(x: x - 1)
+  }
+}