Merge pull request #15102 from jrose-apple/deprecation-and-depreciation-are-two-different-words

jrose-apple · web-flow · commit 3354249f37bf · 2018-03-16T12:47:16.000-07:00
Check deprecation for getters and setters
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -3590,22 +3590,23 @@ NOTE(availability_obsoleted, none,
     (DeclName, StringRef, clang::VersionTuple))
 
 WARNING(availability_deprecated, none,
-        "%0 %select{is|%select{is|was}3}1 deprecated"
-        "%select{| %select{on|in}3 %2%select{| %4}3}1",
-        (DeclName, bool, StringRef, bool, clang::VersionTuple))
+        "%select{getter for |setter for |}0%1 %select{is|%select{is|was}4}2 "
+        "deprecated%select{| %select{on|in}4 %3%select{| %5}4}2",
+        (unsigned, DeclName, bool, StringRef, bool, clang::VersionTuple))
 
 WARNING(availability_deprecated_msg, none,
-        "%0 %select{is|%select{is|was}3}1 deprecated"
-        "%select{| %select{on|in}3 %2%select{| %4}3}1: %5",
-        (DeclName, bool, StringRef, bool, clang::VersionTuple, StringRef))
+        "%select{getter for |setter for |}0%1 %select{is|%select{is|was}4}2 "
+        "deprecated%select{| %select{on|in}4 %3%select{| %5}4}2: %6",
+        (unsigned, DeclName, bool, StringRef, bool, clang::VersionTuple,
+         StringRef))
 
 WARNING(availability_deprecated_rename, none,
-        "%0 %select{is|%select{is|was}3}1 deprecated"
-        "%select{| %select{on|in}3 %2%select{| %4}3}1: "
-        "%select{renamed to|replaced by}5%" REPLACEMENT_DECL_KIND_SELECT "6 "
-        "'%7'",
-        (DeclName, bool, StringRef, bool, clang::VersionTuple, bool, unsigned,
-         StringRef))
+        "%select{getter for |setter for |}0%1 %select{is|%select{is|was}4}2 "
+        "deprecated%select{| %select{on|in}4 %3%select{| %5}4}2: "
+        "%select{renamed to|replaced by}6%" REPLACEMENT_DECL_KIND_SELECT "7 "
+        "'%8'",
+        (unsigned, DeclName, bool, StringRef, bool, clang::VersionTuple, bool,
+         unsigned, StringRef))
 #undef REPLACEMENT_DECL_KIND_SELECT
 
 NOTE(note_deprecated_rename, none,
diff --git a/lib/Sema/TypeCheckAvailability.cpp b/lib/Sema/TypeCheckAvailability.cpp
@@ -1404,10 +1404,7 @@ const AvailableAttr *TypeChecker::getDeprecated(const Decl *D) {
 static bool
 someEnclosingDeclMatches(SourceRange ReferenceRange,
                          const DeclContext *ReferenceDC,
-                         TypeChecker &TC,
                          llvm::function_ref<bool(const Decl *)> Pred) {
-  ASTContext &Ctx = TC.Context;
-
   // Climb the DeclContext hierarchy to see if any of the containing
   // declarations matches the predicate.
   const DeclContext *DC = ReferenceDC;
@@ -1446,6 +1443,7 @@ someEnclosingDeclMatches(SourceRange ReferenceRange,
   if (ReferenceRange.isInvalid())
     return false;
 
+  ASTContext &Ctx = ReferenceDC->getASTContext();
   const Decl *DeclToSearch =
       findContainingDeclaration(ReferenceRange, ReferenceDC, Ctx.SourceMgr);
 
@@ -1474,28 +1472,32 @@ someEnclosingDeclMatches(SourceRange ReferenceRange,
   return false;
 }
 
-bool TypeChecker::isInsideImplicitFunction(SourceRange ReferenceRange,
-                                           const DeclContext *DC) {
+/// Returns true if the reference or any of its parents is an
+/// implicit function.
+static bool isInsideImplicitFunction(SourceRange ReferenceRange,
+                                     const DeclContext *DC) {
   auto IsInsideImplicitFunc = [](const Decl *D) {
     auto *AFD = dyn_cast<AbstractFunctionDecl>(D);
     return AFD && AFD->isImplicit();
   };
 
-  return someEnclosingDeclMatches(ReferenceRange, DC, *this,
-                                  IsInsideImplicitFunc);
+  return someEnclosingDeclMatches(ReferenceRange, DC, IsInsideImplicitFunc);
 }
 
-bool TypeChecker::isInsideUnavailableDeclaration(
-    SourceRange ReferenceRange, const DeclContext *ReferenceDC) {
+/// Returns true if the reference or any of its parents is an
+/// unavailable (or obsoleted) declaration.
+static bool isInsideUnavailableDeclaration(SourceRange ReferenceRange,
+                                           const DeclContext *ReferenceDC) {
   auto IsUnavailable = [](const Decl *D) {
     return D->getAttrs().getUnavailable(D->getASTContext());
   };
 
-  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, *this,
-                                  IsUnavailable);
+  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, IsUnavailable);
 }
 
-bool TypeChecker::isInsideCompatibleUnavailableDeclaration(
+/// Returns true if the reference or any of its parents is an
+/// unconditional unavailable declaration for the same platform.
+static bool isInsideCompatibleUnavailableDeclaration(
     SourceRange ReferenceRange, const DeclContext *ReferenceDC,
     const AvailableAttr *attr) {
   if (!attr->isUnconditionallyUnavailable()) {
@@ -1512,18 +1514,18 @@ bool TypeChecker::isInsideCompatibleUnavailableDeclaration(
     return EnclosingUnavailable && EnclosingUnavailable->Platform == platform;
   };
 
-  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, *this,
-                                  IsUnavailable);
+  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, IsUnavailable);
 }
 
-bool TypeChecker::isInsideDeprecatedDeclaration(SourceRange ReferenceRange,
-                                                const DeclContext *ReferenceDC){
+/// Returns true if the reference is lexically contained in a declaration
+/// that is deprecated on all deployment targets.
+static bool isInsideDeprecatedDeclaration(SourceRange ReferenceRange,
+                                          const DeclContext *ReferenceDC){
   auto IsDeprecated = [](const Decl *D) {
     return D->getAttrs().getDeprecated(D->getASTContext());
   };
 
-  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, *this,
-                                  IsDeprecated);
+  return someEnclosingDeclMatches(ReferenceRange, ReferenceDC, IsDeprecated);
 }
 
 static void fixItAvailableAttrRename(TypeChecker &TC,
@@ -1930,14 +1932,25 @@ void TypeChecker::diagnoseIfDeprecated(SourceRange ReferenceRange,
     }
   }
 
-  DeclName Name = DeprecatedDecl->getFullName();
+  DeclName Name;
+  Optional<unsigned> rawAccessorKind;
+  if (auto *accessor = dyn_cast<AccessorDecl>(DeprecatedDecl)) {
+    Name = accessor->getStorage()->getFullName();
+    assert(accessor->isGetterOrSetter());
+    rawAccessorKind = static_cast<unsigned>(accessor->getAccessorKind());
+  } else {
+    Name = DeprecatedDecl->getFullName();
+  }
+
   StringRef Platform = Attr->prettyPlatformString();
   clang::VersionTuple DeprecatedVersion;
   if (Attr->Deprecated)
     DeprecatedVersion = Attr->Deprecated.getValue();
 
+  static const unsigned NOT_ACCESSOR_INDEX = 2;
   if (Attr->Message.empty() && Attr->Rename.empty()) {
-    diagnose(ReferenceRange.Start, diag::availability_deprecated, Name,
+    diagnose(ReferenceRange.Start, diag::availability_deprecated,
+             rawAccessorKind.getValueOr(NOT_ACCESSOR_INDEX), Name,
              Attr->hasPlatform(), Platform, Attr->Deprecated.hasValue(),
              DeprecatedVersion)
       .highlight(Attr->getRange());
@@ -1951,21 +1964,23 @@ void TypeChecker::diagnoseIfDeprecated(SourceRange ReferenceRange,
 
   if (!Attr->Message.empty()) {
     EncodedDiagnosticMessage EncodedMessage(Attr->Message);
-    diagnose(ReferenceRange.Start, diag::availability_deprecated_msg, Name,
+    diagnose(ReferenceRange.Start, diag::availability_deprecated_msg,
+             rawAccessorKind.getValueOr(NOT_ACCESSOR_INDEX), Name,
              Attr->hasPlatform(), Platform, Attr->Deprecated.hasValue(),
              DeprecatedVersion, EncodedMessage.Message)
       .highlight(Attr->getRange());
   } else {
     unsigned rawReplaceKind = static_cast<unsigned>(
         replacementDeclKind.getValueOr(ReplacementDeclKind::None));
-    diagnose(ReferenceRange.Start, diag::availability_deprecated_rename, Name,
+    diagnose(ReferenceRange.Start, diag::availability_deprecated_rename,
+             rawAccessorKind.getValueOr(NOT_ACCESSOR_INDEX), Name,
              Attr->hasPlatform(), Platform, Attr->Deprecated.hasValue(),
              DeprecatedVersion, replacementDeclKind.hasValue(), rawReplaceKind,
              newName)
       .highlight(Attr->getRange());
   }
 
-  if (!Attr->Rename.empty()) {
+  if (!Attr->Rename.empty() && !rawAccessorKind.hasValue()) {
     auto renameDiag = diagnose(ReferenceRange.Start,
                                diag::note_deprecated_rename,
                                newName);
@@ -2232,9 +2247,11 @@ class AvailabilityWalker : public ASTWalker {
       return std::make_pair(false, E);
     };
 
-    if (auto DR = dyn_cast<DeclRefExpr>(E))
+    if (auto DR = dyn_cast<DeclRefExpr>(E)) {
       diagAvailability(DR->getDecl(), DR->getSourceRange(),
                        getEnclosingApplyExpr());
+      maybeDiagStorageAccess(DR->getDecl(), DR->getSourceRange(), DC);
+    }
     if (auto MR = dyn_cast<MemberRefExpr>(E)) {
       walkMemberRef(MR);
       return skipChildren();
@@ -2250,8 +2267,10 @@ class AvailabilityWalker : public ASTWalker {
     if (auto DS = dyn_cast<DynamicSubscriptExpr>(E))
       diagAvailability(DS->getMember().getDecl(), DS->getSourceRange());
     if (auto S = dyn_cast<SubscriptExpr>(E)) {
-      if (S->hasDecl())
+      if (S->hasDecl()) {
         diagAvailability(S->getDecl().getDecl(), S->getSourceRange());
+        maybeDiagStorageAccess(S->getDecl().getDecl(), S->getSourceRange(), DC);
+      }
     }
     if (auto A = dyn_cast<AssignExpr>(E)) {
       walkAssignExpr(A);
@@ -2334,20 +2353,17 @@ class AvailabilityWalker : public ASTWalker {
   /// Walk a member reference expression, checking for availability.
   void walkMemberRef(MemberRefExpr *E) {
     // Walk the base in a getter context.
+    // FIXME: We may need to look at the setter too, if we're going to do
+    // writeback. The AST should have this information.
     walkInContext(E, E->getBase(), MemberAccessContext::Getter);
 
     ValueDecl *D = E->getMember().getDecl();
     // Diagnose for the member declaration itself.
     if (diagAvailability(D, E->getNameLoc().getSourceRange()))
       return;
 
-    if (TC.getLangOpts().DisableAvailabilityChecking)
-      return;
-
-    if (auto *ASD = dyn_cast<AbstractStorageDecl>(D)) {
-      // Diagnose for appropriate accessors, given the access context.
-      diagStorageAccess(ASD, E->getSourceRange(), DC);
-    }
+    // Diagnose for appropriate accessors, given the access context.
+    maybeDiagStorageAccess(D, E->getSourceRange(), DC);
   }
   
   /// Walk an inout expression, checking for availability.
@@ -2365,9 +2381,16 @@ class AvailabilityWalker : public ASTWalker {
 
   /// Emit diagnostics, if necessary, for accesses to storage where
   /// the accessor for the AccessContext is not available.
-  void diagStorageAccess(AbstractStorageDecl *D,
-                         SourceRange ReferenceRange,
-                         const DeclContext *ReferenceDC) const {
+  void maybeDiagStorageAccess(const ValueDecl *VD,
+                              SourceRange ReferenceRange,
+                              const DeclContext *ReferenceDC) const {
+    if (TC.getLangOpts().DisableAvailabilityChecking)
+      return;
+
+    auto *D = dyn_cast<AbstractStorageDecl>(VD);
+    if (!D)
+      return;
+
     if (!D->hasAccessorFunctions()) {
       return;
     }
@@ -2395,13 +2418,18 @@ class AvailabilityWalker : public ASTWalker {
   }
 
   /// Emit a diagnostic, if necessary for a potentially unavailable accessor.
-  /// Returns true if a diagnostic was emitted.
   void diagAccessorAvailability(AccessorDecl *D, SourceRange ReferenceRange,
                                 const DeclContext *ReferenceDC,
                                 bool ForInout) const {
     if (!D) {
       return;
     }
+
+    // Make sure not to diagnose an accessor if we already complained about
+    // the property/subscript.
+    if (!TypeChecker::getDeprecated(D->getStorage()))
+      TC.diagnoseIfDeprecated(ReferenceRange, ReferenceDC, D, /*call*/nullptr);
+
     auto MaybeUnavail = TC.checkDeclarationAvailability(D, ReferenceRange.Start,
                                                         DC);
     if (MaybeUnavail.hasValue()) {
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -2404,27 +2404,6 @@ class TypeChecker final : public LazyResolver {
       const DeclContext *ReferenceDC, const UnavailabilityReason &Reason,
       bool ForInout);
 
-  /// Returns true if the reference or any of its parents is an
-  /// implicit function.
-  bool isInsideImplicitFunction(SourceRange ReferenceRange,
-                                const DeclContext *DC);
-
-  /// Returns true if the reference or any of its parents is an
-  /// unavailable (or obsoleted) declaration.
-  bool isInsideUnavailableDeclaration(SourceRange ReferenceRange,
-                                      const DeclContext *DC);
-
-  /// Returns true if the reference or any of its parents is an
-  /// unconditional unavailable declaration for the same platform.
-  bool isInsideCompatibleUnavailableDeclaration(SourceRange ReferenceRange,
-                                                const DeclContext *DC,
-                                                const AvailableAttr *attr);
-
-  /// Returns true if the reference is lexically contained in a declaration
-  /// that is deprecated on all deployment targets.
-  bool isInsideDeprecatedDeclaration(SourceRange ReferenceRange,
-                                     const DeclContext *DC);
-
   /// Returns the availability attribute indicating deprecation if the
   /// declaration is deprecated or null otherwise.
   static const AvailableAttr *getDeprecated(const Decl *D);
diff --git a/test/ClangImporter/Inputs/custom-modules/AvailabilityExtras.h b/test/ClangImporter/Inputs/custom-modules/AvailabilityExtras.h
@@ -93,3 +93,17 @@ typedef NS_ENUM(NSInteger, NSEnumAddedCasesIn2017) {
     NSEnumAddedCasesIn2017ExistingCaseThree,
     NSEnumAddedCasesIn2017NewCaseOne __attribute__((availability(macosx,introduced=10.13))) __attribute__((availability(ios,introduced=11.0))) __attribute__((availability(tvos,introduced=11.0))) __attribute__((availability(watchos,introduced=4.0)))
 };
+
+@interface AccessorDeprecations: NSObject
+@property int fullyDeprecated __attribute__((deprecated));
+
+@property int getterDeprecated;
+- (int)getterDeprecated __attribute__((deprecated));
+@property (class) int getterDeprecatedClass;
++ (int)getterDeprecatedClass __attribute__((deprecated));
+
+@property int setterDeprecated;
+- (void)setSetterDeprecated:(int)setterDeprecated __attribute__((deprecated));
+@property (class) int setterDeprecatedClass;
++ (void)setSetterDeprecatedClass:(int)setterDeprecated __attribute__((deprecated));
+@end
diff --git a/test/ClangImporter/availability.swift b/test/ClangImporter/availability.swift
@@ -25,8 +25,28 @@ func test_unavailable_func(_ x : NSObject) {
   NSDeallocateObject(x) // expected-error {{'NSDeallocateObject' is unavailable}}
 }
 
-func test_deprecated_imported_as_unavailable(_ s:UnsafeMutablePointer<CChar>) {
+func test_deprecated(_ s:UnsafeMutablePointer<CChar>, _ obj: AccessorDeprecations) {
   _ = tmpnam(s) // expected-warning {{'tmpnam' is deprecated: Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead.}}
+
+  _ = obj.fullyDeprecated // expected-warning {{'fullyDeprecated' is deprecated}}
+  obj.fullyDeprecated = 0 // expected-warning {{'fullyDeprecated' is deprecated}}
+  obj.fullyDeprecated += 1 // expected-warning {{'fullyDeprecated' is deprecated}}
+
+  _ = obj.getterDeprecated // expected-warning {{getter for 'getterDeprecated' is deprecated}}
+  obj.getterDeprecated = 0
+  obj.getterDeprecated += 1 // expected-warning {{getter for 'getterDeprecated' is deprecated}}
+
+  _ = AccessorDeprecations.getterDeprecatedClass // expected-warning {{getter for 'getterDeprecatedClass' is deprecated}}
+  AccessorDeprecations.getterDeprecatedClass = 0
+  AccessorDeprecations.getterDeprecatedClass += 1 // expected-warning {{getter for 'getterDeprecatedClass' is deprecated}}
+
+  _ = obj.setterDeprecated
+  obj.setterDeprecated = 0 // expected-warning {{setter for 'setterDeprecated' is deprecated}}
+  obj.setterDeprecated += 1 // expected-warning {{setter for 'setterDeprecated' is deprecated}}
+
+  _ = AccessorDeprecations.setterDeprecatedClass
+  AccessorDeprecations.setterDeprecatedClass = 0 // expected-warning {{setter for 'setterDeprecatedClass' is deprecated}}
+  AccessorDeprecations.setterDeprecatedClass += 1 // expected-warning {{setter for 'setterDeprecatedClass' is deprecated}}
 }
 
 func test_NSInvocation(_ x: NSInvocation,         // expected-error {{'NSInvocation' is unavailable}}
diff --git a/test/attr/attr_availability.swift b/test/attr/attr_availability.swift
