Merge pull request #65456 from tshortli/improve-override-of-unavailable-diagnostics

Sema: Improve diagnostics for overrides of unavailable decls

Author: tshortli

lib/Sema/TypeCheckAvailability.cpp

@@ -2764,9 +2764,9 @@ bool TypeChecker::diagnoseIfDeprecated(SourceLoc loc,
   return true;
 }
 
-void swift::diagnoseUnavailableOverride(ValueDecl *override,
-                                        const ValueDecl *base,
-                                        const AvailableAttr *attr) {
+void swift::diagnoseOverrideOfUnavailableDecl(ValueDecl *override,
+                                              const ValueDecl *base,
+                                              const AvailableAttr *attr) {
   ASTContext &ctx = override->getASTContext();
   auto &diags = ctx.Diags;
   if (attr->Rename.empty()) {

lib/Sema/TypeCheckAvailability.h

@@ -245,9 +245,11 @@ bool diagnoseDeclAvailability(const ValueDecl *D, SourceRange R,
                               const Expr *call, const ExportContext &where,
                               DeclAvailabilityFlags flags = None);
 
-void diagnoseUnavailableOverride(ValueDecl *override,
-                                 const ValueDecl *base,
-                                 const AvailableAttr *attr);
+/// Emit a diagnostic for an available declaration that overrides an
+/// unavailable declaration.
+void diagnoseOverrideOfUnavailableDecl(ValueDecl *override,
+                                       const ValueDecl *base,
+                                       const AvailableAttr *attr);
 
 /// Emit a diagnostic for references to declarations that have been
 /// marked as unavailable, either through "unavailable" or "obsoleted:".

lib/Sema/TypeCheckDeclOverride.cpp

@@ -1835,6 +1835,52 @@ static bool diagnoseOverrideForAvailability(ValueDecl *override,
   return true;
 }
 
+enum class OverrideUnavailabilityStatus {
+  /// The unavailability of the base decl and override decl are compatible.
+  Compatible,
+  /// The base decl is unavailable but the override decl is not.
+  BaseUnavailable,
+  /// Do not diagnose the unavailability of these decls.
+  Ignored,
+};
+
+static std::pair<OverrideUnavailabilityStatus, const AvailableAttr *>
+checkOverrideUnavailability(ValueDecl *override, ValueDecl *base) {
+  if (auto *overrideParent = override->getDeclContext()->getAsDecl()) {
+    // If the parent of the override is unavailable, then the unavailability of
+    // the override decl is irrelevant.
+    if (overrideParent->getSemanticUnavailableAttr())
+      return {OverrideUnavailabilityStatus::Ignored, nullptr};
+  }
+
+  if (auto *baseAccessor = dyn_cast<AccessorDecl>(base)) {
+    // Ignore implicit accessors since the diagnostics are likely to duplicate
+    // the diagnostics for the explicit accessors that availability was inferred
+    // from.
+    if (baseAccessor->isImplicit())
+      return {OverrideUnavailabilityStatus::Ignored, nullptr};
+
+    if (auto *overrideAccessor = dyn_cast<AccessorDecl>(override)) {
+      // If base and override are accessors, check whether the unavailability of
+      // their storage matches. Diagnosing accessors with invalid storage
+      // produces redundant diagnostics.
+      if (checkOverrideUnavailability(overrideAccessor->getStorage(),
+                                      baseAccessor->getStorage())
+              .first != OverrideUnavailabilityStatus::Compatible)
+        return {OverrideUnavailabilityStatus::Ignored, nullptr};
+    }
+  }
+
+  auto &ctx = override->getASTContext();
+  auto *baseUnavailableAttr = base->getAttrs().getUnavailable(ctx);
+  auto *overrideUnavailableAttr = override->getAttrs().getUnavailable(ctx);
+
+  if (baseUnavailableAttr && !overrideUnavailableAttr)
+    return {OverrideUnavailabilityStatus::BaseUnavailable, baseUnavailableAttr};
+
+  return {OverrideUnavailabilityStatus::Compatible, nullptr};
+}
+
 static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
   // This can happen with circular inheritance.
   // FIXME: This shouldn't be possible once name lookup goes through the
@@ -2066,17 +2112,28 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
   }
 
   // FIXME: Possibly should extend to more availability checking.
-  if (auto *attr = base->getAttrs().getUnavailable(ctx)) {
-    diagnoseUnavailableOverride(override, base, attr);
+  auto unavailabilityStatusAndAttr =
+      checkOverrideUnavailability(override, base);
+  auto *unavailableAttr = unavailabilityStatusAndAttr.second;
+
+  switch (unavailabilityStatusAndAttr.first) {
+  case OverrideUnavailabilityStatus::BaseUnavailable: {
+    diagnoseOverrideOfUnavailableDecl(override, base, unavailableAttr);
 
     if (isUnavailableInAllVersions(base)) {
       auto modifier = override->getAttrs().getAttribute<OverrideAttr>();
       if (modifier && modifier->isValid()) {
-        diags.diagnose(override, diag::suggest_removing_override,
-                       override->getBaseName())
-          .fixItRemove(modifier->getRange());
+        diags
+            .diagnose(override, diag::suggest_removing_override,
+                      override->getBaseName())
+            .fixItRemove(modifier->getRange());
       }
     }
+    break;
+  }
+  case OverrideUnavailabilityStatus::Compatible:
+  case OverrideUnavailabilityStatus::Ignored:
+    break;
   }
 
   if (!ctx.LangOpts.DisableAvailabilityChecking) {

lib/Sema/TypeCheckProtocol.cpp

@@ -4384,7 +4384,7 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) {
 
     case CheckKind::Unavailable: {
       auto *attr = requirement->getAttrs().getUnavailable(getASTContext());
-      diagnoseUnavailableOverride(witness, requirement, attr);
+      diagnoseOverrideOfUnavailableDecl(witness, requirement, attr);
       break;
     }
 

test/Sema/availability.swift

@@ -31,64 +31,6 @@ func foo(x : NSUInteger) { // expected-error {{'NSUInteger' is unavailable: use
   // expected-error@-1 {{cannot convert value of type 'Int' to specified type 'Outer.NSUInteger'}}
 }
 
-// Test preventing overrides (but allowing shadowing) of unavailable methods.
-class ClassWithUnavailable {
-  @available(*, unavailable)
-  func doNotOverride() {} // expected-note {{'doNotOverride()' has been explicitly marked unavailable here}}
-
-  // FIXME: extraneous diagnostic here
-  @available(*, unavailable)
-  init(int _: Int) {} // expected-note 3 {{'init(int:)' has been explicitly marked unavailable here}}
-
-  @available(*, unavailable)
-  convenience init(otherInt: Int) {
-    self.init(int: otherInt) // expected-error {{'init(int:)' is unavailable}}
-  }
-
-  @available(*, unavailable)
-  required init(necessaryInt: Int) {}
-
-  @available(*, unavailable)
-  subscript (i: Int) -> Int { // expected-note 2 {{'subscript(_:)' has been explicitly marked unavailable here}}
-    return i
-  }
-}
-
-class ClassWithOverride : ClassWithUnavailable {
-  override func doNotOverride() {}
-  // expected-error@-1 {{cannot override 'doNotOverride' which has been marked unavailable}}
-  // expected-note@-2 {{remove 'override' modifier to declare a new 'doNotOverride'}} {{3-12=}}
-
-  override init(int: Int) {}
-  // expected-error@-1 {{cannot override 'init' which has been marked unavailable}}
-  // expected-note@-2 {{remove 'override' modifier to declare a new 'init'}} {{3-12=}}
-
-  // can't override convenience inits
-  // can't override required inits
-
-  override subscript (i: Int) -> Int { return i }
-  // expected-error@-1 {{cannot override 'subscript' which has been marked unavailable}}
-  // expected-note@-2 {{remove 'override' modifier to declare a new 'subscript'}} {{3-12=}}
-}
-
-class ClassWithShadowing : ClassWithUnavailable {
-  func doNotOverride() {} // no-error
-  init(int: Int) {} // no-error
-  convenience init(otherInt: Int) { self.init(int: otherInt) } // no-error
-  required init(necessaryInt: Int) {} // no-error
-  subscript (i: Int) -> Int { return i } // no-error
-}
-
-func testInit() {
-  ClassWithUnavailable(int: 0) // expected-error {{'init(int:)' is unavailable}} // expected-warning{{unused}}
-  ClassWithShadowing(int: 0) // expected-warning {{unused}}
-}
-
-func testSubscript(cwu: ClassWithUnavailable, cws: ClassWithShadowing) {
-  _ = cwu[5] // expected-error{{'subscript(_:)' is unavailable}}
-  _ = cws[5] // no-error
-}
-
 /* FIXME 'nil == a' fails to type-check with a bogus error message
  * <rdar://problem/17540796>
 func markUsed<T>(t: T) {}