[ObjC] warn about availability attributes missing from a method's declaration when they're specified for a method's definition

[arphaman]

Summary

This patch extends the -Wavailability warning to warn about cases where a method declaration is missing an availability attribute clause that's present in the method's definition. We also warn about missing deprecated attributes as well.

rdar://15540962

[erik.pilkington]

Any thoughts on having this for regular functions and C++ member functions? Seems just as useful there.

include/clang/Sema/Sema.h

2406

Looks longer than 80 chars!

lib/Sema/SemaDeclAttr.cpp

2300

I hate to rewrite your function, but have you considered just doing the second loop over the attrs inline instead of hidden in the lambda? I think it would make the function more simple to avoid all the lambdas and callbacks:

for (auto *A : Implementation->specific_attrs<AvailabilityAttr>()) {
  for (auto *B : Original->specific_attrs<AvailabilityAttr>()) {
    if (A->getPlatform() != B->getPlatform()) continue;
    if (A->getIntroduced() != B->getIntroduced() || ...) {
      Diag();
    }
  }
}

You can do the deprecated check outside of the loop like:

if (Impl->hasAttr<Dep>() && !Original->hasAttr<Dep>())

[arphaman]

lib/Sema/SemaDeclAttr.cpp

2300

Nice, that's much better! Updated.

[arphaman]

• formatting
• simplify checks
• support c++

[ahatanak]

Is it not necessary to print a diagnostic when a non-member function declaration is missing an availability attribute?

void foo1();

__attribute__((availability(macos, introduced=10.1)))
void foo1() {
}

[arphaman]

In D39913#924131, @ahatanak wrote:

Is it not necessary to print a diagnostic when a non-member function declaration is missing an availability attribute?

void foo1();

__attribute__((availability(macos, introduced=10.1)))
void foo1() {
}

It's harder as we don't know the distinction between declaration/definition at merge time. Right now the C++ implementation of this warning actually checks the attributes too late, after they're merged. I'll take out the C++/C support completely and keep to ObjC in this patch for simplicity. The rest can be supported later.

[arphaman]

Remove C++ support for now (it will be in a followup patch along with non-member function support).

[erik.pilkington]

It's harder as we don't know the distinction between declaration/definition at merge time. Right now the C++ implementation of this warning actually checks the attributes too late, after they're merged. I'll take out the C++/C support completely and keep to ObjC in this patch for simplicity. The rest can be supported later.

That sounds fine for now! LGTM

[ahatanak]

LGTM

lib/Sema/SemaDeclAttr.cpp

2295

I feel like using "if" is easier to understand than a conditional operator, but it's up to you:

if (MissingIntroduced)
  MissingIntroduced = Decl->getIntroduced().empty();

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2921

Please quote 'availability' in the diagnostic wording. Same for the note.

2925

Please quote 'deprecated' in the diagnostic wording. Same for the note.

One concern I have about this diagnostic is that it suggests this will happen for *all* deprecated attributes when it does not.

include/clang/Sema/Sema.h

2406–2407

These should be const pointers.

Does this *only* warn on missing clauses, or does it also warn on conflicting clauses as the first sentence suggests?

lib/Sema/SemaDecl.cpp

3581–3582

Please do not use isa<> followed by cast<> (same below). Rather than use this complex if statement, it might be better to split things out into local variables.

lib/Sema/SemaDeclAttr.cpp

2286

const auto *

2291

const auto *

2295

The pattern we usually see is: MissingIntroduced = MissingIntroduced && Decl->getIntroduced().empty();

include/clang/Basic/DiagnosticSemaKinds.td

def warn_mismatched_availability_override : Warning<
   "deprecated before|obsoleted before}0 "
   "%select{the protocol method it implements|overridden method}4 "
   "on %1 (%2 vs. %3)">, InGroup<Availability>;
 def warn_mismatched_availability_override_unavail : Warning<
   "%select{|overriding }1method cannot be unavailable on %0 when "
   "%select{the protocol method it implements|its overridden method}1 is "
   "available">,
   InGroup<Availability>;
+def warn_availability_on_implementation_not_interface : Warning<
+  "method declaration is missing an availability attribute for "
+  "%0 that is specified in the definition">,
+  InGroup<Availability>;
+def warn_deprecated_on_implementation_not_interface : Warning<
+  "method declaration is missing a deprecated attribute that is specified in "
+  "the definition">,
+  InGroup<Availability>;
+def note_definition_with_availability_here : Note<
+  "definition with %0 availability attribute is here">;
+def note_definition_with_deprecated_here : Note<
+  "definition with deprecated attribute is here">;
 def note_overridden_method : Note<
   "overridden method is here">;
 def note_protocol_method : Note<
   "protocol method is here">;
 
 def warn_unguarded_availability :
   Warning<"%0 is only available on %1 %2 or newer">,
   InGroup<UnguardedAvailability>, DefaultIgnore;

include/clang/Sema/Sema.h

enum AvailabilityMergeKind {
     /// \brief Merge availability attributes for an override, which requires
     /// an exact match or a weakening of constraints.
     AMK_Override,
     /// \brief Merge availability attributes for an implementation of
     /// a protocol requirement.
     AMK_ProtocolImplementation,
   };
 
+  /// Verifies that availability attribute clauses that are specified on an
+  /// method definition match the clauses from the method's declaration.
+  ///
+  /// This will warn on any missing clauses in the declaration.
+  void checkMissingAvailabilityClausesInDeclaration(NamedDecl *Original,
+                                                    NamedDecl *Implementation);
+
   /// Attribute merging methods. Return true if a new attribute was added.
   AvailabilityAttr *mergeAvailabilityAttr(NamedDecl *D, SourceRange Range,
                                           IdentifierInfo *Platform,
                                           bool Implicit,
                                           VersionTuple Introduced,
                                           VersionTuple Deprecated,
                                           VersionTuple Obsoleted,
                                           bool IsUnavailable,

lib/Sema/SemaDecl.cpp

void Sema::mergeObjCMethodDecls(ObjCMethodDecl *newMethod,
                                 ObjCMethodDecl *oldMethod) {
   // Merge the attributes, including deprecated/unavailable
   AvailabilityMergeKind MergeKind =
     isa<ObjCProtocolDecl>(oldMethod->getDeclContext())
       ? AMK_ProtocolImplementation
       : isa<ObjCImplDecl>(newMethod->getDeclContext()) ? AMK_Redeclaration
                                                        : AMK_Override;
 
+  // Warn on any availability clauses that are missing in the method's
+  // declaration but specified in the definition.
+  if (const ObjCImplDecl *Imp =
+          dyn_cast<ObjCImplDecl>(newMethod->getDeclContext())) {
+    const Decl *DC = cast<Decl>(oldMethod->getDeclContext());
+    if (Imp->getClassInterface() == DC ||
+        (isa<ObjCCategoryDecl>(DC) &&
+         cast<ObjCCategoryDecl>(DC)->IsClassExtension() &&
+         Imp->getClassInterface() ==
+             cast<ObjCCategoryDecl>(DC)->getClassInterface()) ||
+        (isa<ObjCCategoryImplDecl>(Imp) &&
+         cast<ObjCCategoryImplDecl>(Imp)->getCategoryDecl() == DC))
+      checkMissingAvailabilityClausesInDeclaration(oldMethod, newMethod);
+  }
   mergeDeclAttributes(newMethod, oldMethod, MergeKind);
 
   // Merge attributes from the parameters.
   ObjCMethodDecl::param_const_iterator oi = oldMethod->param_begin(),
                                        oe = oldMethod->param_end();
   for (ObjCMethodDecl::param_iterator
          ni = newMethod->param_begin(), ne = newMethod->param_end();
        ni != ne && oi != oe; ++ni, ++oi)

lib/Sema/SemaDeclAttr.cpp

if (X == Y)
     return true;
 
   if (BeforeIsOkay && X < Y)
     return true;
 
   return false;
 }
 
+void Sema::checkMissingAvailabilityClausesInDeclaration(
+    NamedDecl *Original, NamedDecl *Implementation) {
+  if (!Implementation->hasAttrs())
+    return;
+
+  llvm::SmallPtrSet<const IdentifierInfo *, 4> MissingPlatformAttributes;
+  for (auto *Def : Implementation->specific_attrs<AvailabilityAttr>()) {
+    bool MissingIntroduced = !Def->getIntroduced().empty();
+    bool MissingDeprecated = !Def->getDeprecated().empty();
+    bool MissingObsoleted = !Def->getObsoleted().empty();
+    bool MissingUnavailable = Def->getUnavailable();
+    for (auto *Decl : Original->specific_attrs<AvailabilityAttr>()) {
+      if (Def->getPlatform() != Decl->getPlatform())
+        continue;
+      MissingIntroduced =
+          MissingIntroduced ? Decl->getIntroduced().empty() : false;
+      MissingDeprecated =
+          MissingDeprecated ? Decl->getDeprecated().empty() : false;
+      MissingObsoleted =
+          MissingObsoleted ? Decl->getObsoleted().empty() : false;
+      MissingUnavailable = MissingUnavailable ? !Decl->getUnavailable() : false;
+    }
+    if (MissingIntroduced || MissingDeprecated || MissingObsoleted ||
+        MissingUnavailable) {
+      if (!MissingPlatformAttributes.insert(Def->getPlatform()).second)
+        continue;
+      StringRef PlatformSpelling = AvailabilityAttr::getPrettyPlatformName(
+          Def->getPlatform()->getName());
+      Diag(Original->getLocation(),
+           diag::warn_availability_on_implementation_not_interface)
+          << PlatformSpelling;
+      Diag(Def->getLocation(), diag::note_definition_with_availability_here)
+          << PlatformSpelling;
+    }
+  }
+  if (const auto *Dep = Implementation->getAttr<DeprecatedAttr>()) {
+    if (!Original->hasAttr<DeprecatedAttr>()) {
+      Diag(Original->getLocation(),
+           diag::warn_deprecated_on_implementation_not_interface);
+      Diag(Dep->getLocation(), diag::note_definition_with_deprecated_here);
+    }
+  }
+}
+
 AvailabilityAttr *Sema::mergeAvailabilityAttr(NamedDecl *D, SourceRange Range,
                                               IdentifierInfo *Platform,
                                               bool Implicit,
                                               VersionTuple Introduced,
                                               VersionTuple Deprecated,
                                               VersionTuple Obsoleted,
                                               bool IsUnavailable,
                                               StringRef Message,

test/SemaObjC/method-attributes.m

 -(void) m2 __attribute__((stdcall));
 -(void) m3 __attribute__((optnone));
 @end
 
 
 @interface INTF
 - (int) foo1: (int)arg1 __attribute__((deprecated));
 
-- (int) foo: (int)arg1;
+- (int) foo: (int)arg1;  // expected-warning {{method declaration is missing a deprecated attribute that is specified in the definition}}
 
 - (int) foo2: (int)arg1 __attribute__((deprecated)) __attribute__((unavailable));
 - (int) foo3: (int)arg1 __attribute__((deprecated)) __attribute__((unavailable)) __attribute__((ns_consumes_self));
 @end
 
 @implementation INTF
-- (int) foo: (int)arg1  __attribute__((deprecated)){
+- (int) foo: (int)arg1  __attribute__((deprecated)){ // expected-note {{definition with deprecated attribute is here}}
         return 10;
 }
 - (int) foo1: (int)arg1 {
         return 10;
 }
 - (int) foo2: (int)arg1 __attribute__((deprecated)) {
         return 10;
 }

test/SemaObjC/unguarded-availability.m

 }
 
 __attribute__((objc_root_class))
 AVAILABLE_10_11 @interface Class_10_11 { // expected-note{{annotate 'Class_10_11' with an availability attribute to silence}}
   int_10_11 foo;
   int_10_12 bar; // expected-warning {{'int_10_12' is only available on macOS 10.12 or newer}}
 }
 - (void)method1;
-- (void)method2;
+- (void)method2 AVAILABLE_10_12;
 @end
 
 @implementation Class_10_11
 - (void) method1 {
   func_10_11();
   func_10_12(); // expected-warning{{'func_10_12' is only available on macOS 10.12 or newer}} expected-note{{enclose 'func_10_12' in an @available check to silence this warning}}
 }
 

test/SemaObjC/warn-missing-method-decl-availability.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9.0.0 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Warn about availability attribute when they're specified in the definition
+// of the method instead of the declaration.
+// rdar://15540962
+
+@interface MissingAvailabilityThingsInInterface
+
+- (void)missingIDO; // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+
+- (void)missingDO __attribute__((availability(macos, introduced=10.1))); // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+
+- (void)missingD __attribute__((availability(macos, introduced=10.1))); // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+
+- (void)missingIx2; // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+// expected-warning@-1 {{method declaration is missing an availability attribute for iOS that is specified in the definition}}
+
+- (void)missingIDOiOS __attribute__((availability(macos, introduced=10.1))); // expected-warning {{method declaration is missing an availability attribute for iOS that is specified in the definition}}
+
+- (void)differentIMissingD __attribute__((availability(macos, introduced=10.1))); // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+// expected-note@-1 {{previous attribute is here}}
+
+- (void)missingUnavailable; // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+
+- (void)same
+__attribute__((availability(macos, introduced=10.1)))
+__attribute__((availability(ios, unavailable)));
+
+- (void)missingDeprecatedAttr; // expected-warning {{method declaration is missing a deprecated attribute that is specified in the definition}}
+
+- (void)sameDeprecatedAttr __attribute__((deprecated("y")));
+
+@end
+
+@interface MissingAvailabilityThingsInInterface()
+
+- (void)missingInClassExtension; // expected-warning {{method declaration is missing an availability attribute for macOS that is specified in the definition}}
+
+@end
+
+@implementation MissingAvailabilityThingsInInterface
+
+- (void)missingIDO
+__attribute__((availability(macos, introduced=10.1, deprecated=10.2, obsoleted=10.3))) // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)missingDO
+__attribute__((availability(macos, introduced=10.1)))
+__attribute__((availability(macos, deprecated=10.2, obsoleted=10.3))) // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)missingD
+__attribute__((availability(macos, introduced=10.1, deprecated=10.2))) // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)missingIx2
+__attribute__((availability(ios, introduced=10))) // expected-note {{definition with iOS availability attribute is here}}
+__attribute__((availability(macos, introduced=10.1))) // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)missingIDOiOS
+__attribute__((availability(ios, introduced=10, deprecated=11, obsoleted=11.1))) // expected-note {{definition with iOS availability attribute is here}}
+__attribute__((availability(macOS, introduced=10.1)))
+{ }
+
+- (void)differentIMissingD __attribute__((availability(macos, introduced=10.2, deprecated=10.3))) // expected-note {{definition with macOS availability attribute is here}}
+{ } // expected-warning@-1{{availability does not match previous declaration}}
+
+- (void)missingInClassExtension
+__attribute__((availability(macos, introduced=10.1, deprecated=10.2))) // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)missingUnavailable
+__attribute__((availability(macos, unavailable))); // expected-note {{definition with macOS availability attribute is here}}
+{ }
+
+- (void)same
+__attribute__((availability(macos, introduced=10.1)))
+__attribute__((availability(ios, unavailable)))
+{ }
+
+- (void)missingDeprecatedAttr
+__attribute__((deprecated("x")))  // expected-note {{definition with deprecated attribute is here}}
+{ }
+
+- (void)sameDeprecatedAttr __attribute__((deprecated("y")))
+{ }
+
+@end
+
+@interface MissingAvailabilityThingsInInterface (Category)
+
+- (void)missingInCategory; // expected-warning {{method declaration is missing an availability attribute for tvOS that is specified in the definition}}
+
+@end
+
+@implementation MissingAvailabilityThingsInInterface (Category)
+
+- (void)missingInCategory
+__attribute__((availability(tvOS, introduced=10))) // expected-note {{definition with tvOS availability attribute is here}}
+{ }
+
+@end
+
+@interface DontWarnOnOverrideImpl
+
+- (void)missingIDO
+__attribute__((availability(macos, introduced=10.1, deprecated=10.2, obsoleted=10.3))); // ok
+
+@end
+
+@implementation DontWarnOnOverrideImpl
+
+- (void)missingIDO { }
+
+// ok
+- (void)missingDO
+__attribute__((availability(macos, introduced=10.1)))
+__attribute__((availability(macos, deprecated=10.2, obsoleted=10.3)))
+{ }
+
+@end
+
+@protocol DontWarnOnProtocol
+
+- (void)missingIDO;
+
+@end
+
+@interface DontWarnOnProtocolImpl<DontWarnOnProtocol>
+@end
+
+@implementation DontWarnOnProtocolImpl
+
+- (void)missingIDO
+__attribute__((availability(macos, introduced=10.1, deprecated=10.2, obsoleted=10.3)))
+{}
+
+@end