Merge pull request swiftlang#32993 from brentdax/override-sally-ride

beccadax · web-flow · commit ddc0494c8107 · 2020-07-24T12:05:18.000-07:00
Allow shadowing of unavailable members
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2415,6 +2415,9 @@ NOTE(multiple_override_prev,none,
 ERROR(override_unavailable, none,
       "cannot override %0 which has been marked unavailable%select{|: %1}1",
       (DeclBaseName, StringRef))
+NOTE(suggest_removing_override, none,
+     "remove 'override' modifier to declare a new %0",
+     (DeclBaseName))
 
 ERROR(override_less_available,none,
       "overriding %0 must be as available as declaration it overrides",
diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp
@@ -214,6 +214,19 @@ bool swift::isOverrideBasedOnType(const ValueDecl *decl, Type declTy,
   return canDeclTy == canParentDeclTy;
 }
 
+static bool isUnavailableInAllVersions(ValueDecl *decl) {
+  ASTContext &ctx = decl->getASTContext();
+  auto *attr = decl->getAttrs().getUnavailable(ctx);
+
+  if (!attr)
+    return false;
+  if (attr->isUnconditionallyUnavailable())
+    return true;
+
+  return attr->getVersionAvailability(ctx)
+             == AvailableVersionComparison::Unavailable;
+}
+
 /// Perform basic checking to determine whether a declaration can override a
 /// declaration in a superclass.
 static bool areOverrideCompatibleSimple(ValueDecl *decl,
@@ -246,6 +259,15 @@ static bool areOverrideCompatibleSimple(ValueDecl *decl,
   if (decl->getKind() != parentDecl->getKind())
     return false;
 
+  // If the parent decl is unavailable, the subclass decl can shadow it, but it
+  // can't override it. To avoid complex version logic, we don't apply this to
+  // `obsoleted` members, only `unavailable` ones.
+  // FIXME: Refactor to allow that when the minimum version is always satisfied.
+  if (isUnavailableInAllVersions(parentDecl))
+    // If the subclass decl is trying to override, we'll diagnose it later.
+    if (!decl->getAttrs().hasAttribute<OverrideAttr>())
+      return false;
+
   // Ignore invalid parent declarations.
   // FIXME: Do we really need this?
   if (parentDecl->isInvalid())
@@ -1864,6 +1886,15 @@ 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);
+
+    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());
+      }
+    }
   }
 
   if (!ctx.LangOpts.DisableAvailabilityChecking) {
diff --git a/localization/diagnostics/en.yaml b/localization/diagnostics/en.yaml
@@ -6081,6 +6081,10 @@
   msg: >-
     cannot override %0 which has been marked unavailable%select{|: %1}1
 
+- id: suggest_removing_override
+  msg: >-
+    remove 'override' modifier to declare a new %0
+
 - id: override_less_available
   msg: >-
     overriding %0 must be as available as declaration it overrides
diff --git a/test/SILGen/vtables.swift b/test/SILGen/vtables.swift
@@ -6,6 +6,7 @@ class C : B {
   override func bar() {}
   // bas inherited from A
   override func qux() {}
+  func flux() {}
 
   // zim inherited from B
   override func zang() {}
@@ -20,9 +21,11 @@ class C : B {
 // CHECK:   #A.bar: {{.*}} : @$s7vtables1CC3bar{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.bas: {{.*}} : @$s7vtables1AC3bas{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.qux: {{.*}} : @$s7vtables1CC3qux{{[_0-9a-zA-Z]*}}F
+// CHECK:   #A.flux: {{.*}} : @$s7vtables1BC4flux{{[_0-9a-zA-Z]*}}F
 // CHECK:   #B.init!allocator: {{.*}} : @$s7vtables1CC{{[_0-9a-zA-Z]*}}fC
 // CHECK:   #B.zim: {{.*}} : @$s7vtables1BC3zim{{[_0-9a-zA-Z]*}}F
 // CHECK:   #B.zang: {{.*}} : @$s7vtables1CC4zang{{[_0-9a-zA-Z]*}}F
+// CHECK:   #C.flux: {{.*}} : @$s7vtables1CC4flux{{[_0-9a-zA-Z]*}}F
 // CHECK:   #C.flopsy: {{.*}} : @$s7vtables1CC6flopsy{{[_0-9a-zA-Z]*}}F
 // CHECK:   #C.mopsy: {{.*}} : @$s7vtables1CC5mopsy{{[_0-9a-zA-Z]*}}F
 // CHECK: }
@@ -32,13 +35,15 @@ class A {
   func bar() {}
   func bas() {}
   func qux() {}
+  func flux() {}
 }
 
 // CHECK: sil_vtable A {
 // CHECK:   #A.foo: {{.*}} : @$s7vtables1AC3foo{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.bar: {{.*}} : @$s7vtables1AC3bar{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.bas: {{.*}} : @$s7vtables1AC3bas{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.qux: {{.*}} : @$s7vtables1AC3qux{{[_0-9a-zA-Z]*}}F
+// CHECK:   #A.flux: {{.*}} : @$s7vtables1AC4flux{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.init!allocator: {{.*}} : @$s7vtables1AC{{[_0-9a-zA-Z]*}}fC
 // CHECK: }
 
@@ -49,6 +54,7 @@ class B : A {
   // bar inherited from A
   // bas inherited from A
   override func qux() {}
+  @available(*, unavailable) override func flux() {}
 
   func zim() {}
   func zang() {}
@@ -59,6 +65,7 @@ class B : A {
 // CHECK:   #A.bar: {{.*}} : @$s7vtables1AC3bar{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.bas: {{.*}} : @$s7vtables1AC3bas{{[_0-9a-zA-Z]*}}F
 // CHECK:   #A.qux: {{.*}} : @$s7vtables1BC3qux{{[_0-9a-zA-Z]*}}F
+// CHECK:   #A.flux: {{.*}} : @$s7vtables1BC4flux{{[_0-9a-zA-Z]*}}F
 // CHECK:   #B.init!allocator: {{.*}} : @$s7vtables1BC{{[_0-9a-zA-Z]*}}fC
 // CHECK:   #B.zim: {{.*}} : @$s7vtables1BC3zim{{[_0-9a-zA-Z]*}}F
 // CHECK:   #B.zang: {{.*}} : @$s7vtables1BC4zang{{[_0-9a-zA-Z]*}}F
diff --git a/test/Sema/availability.swift b/test/Sema/availability.swift
@@ -27,35 +27,62 @@ func foo(x : NSUInteger) { // expected-error {{'NSUInteger' is unavailable: use
   let z3 : MyModule.Outer.NSUInteger = 42 // expected-error {{'NSUInteger' is unavailable: use 'UInt' instead}}  
 }
 
-// Test preventing overrides of unavailable methods.
+// 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 2 {{'init(int:)' has been explicitly marked unavailable here}}
+  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)
-  subscript (i: Int) -> Int { // expected-note{{'subscript(_:)' has been explicitly marked unavailable here}}
+  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 {{cannot override 'doNotOverride' which has been marked unavailable}}
+  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) {
+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
diff --git a/test/attr/attr_availability.swift b/test/attr/attr_availability.swift
