[clang-tidy] New check for methods marked __attribute__((unavailable)) that do not override a method from a superclass.

[mwyman]

Summary

Such method declarations don't provide any benefit, as even without the declaration the compiler would complain about calling the method as it doesn't exist.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

104

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst

7

Please enclose attribute((unavailable)) in double back-ticks.

[mwyman]

Updated to explicitly check for attribute((unavailable)), to avoid flagging methods marked based on platform availability. Updated test file to validate this.

[mwyman]

Updated documentation per review comments.

[njames93]

Fix the test case.

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

30

Should probably be in an anonymous namespace this as you don't intend it to be visible in another translation unit.

35

See https://reviews.llvm.org/D75340

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m

35

Generally we are cautious about modifying MACRO usages in clang_tidy as we don't know if its definition can change based on configuration, perhaps its safer to just warn instead of providing a fix it

52

This is not a satisfactory check, it will ensure at least one method has been deleted but it wont ensure all methods have been deleted. Would probably be safer putting a unique comment on the line and having a check fix that checks for an empty line and the comment for each case.

[mwyman]

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m

35

Sounds reasonable; I made this the default behavior.

However for Objective-C, it's quite common for developers to use a macro from the Apple SDKs like NS_UNAVAILABLE that are unconditional in any situations I know of. I added a config option to allow whitelisting macros that should have fix-its provided.

52

Thanks for the suggestion! Done.

[mwyman]

I goofed on updating with Arcanist—the changes I marked done will be incoming shortly!

[mwyman]

Don't provide fix-it hints when the unavailable attribute is inside a macro, unless within a config-whitelisted macro.

[njames93]

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

48–50

return llvm::is_contained(Macros, Tok.getRawIdentifier().str())

53

Should probably return an llvm::Optional<FixItHint>

81

This check is surplus to requirements now that there is the isLanguageVersionSupported check

85

objcMethodDecl is implicitly an allOf matcher, so there is no need to specify allOf

[mwyman]

Updated per review feedback.

[mwyman]

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

81

Oops, forgot to delete it here.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst

21

You should also document the user-facing option for the check.

[mwyman]

Update documentation to include description of the FixMacroNames config option.

[aaron.ballman]

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

64

I'm not an ObjC expert, so I apologize if this is a dumb question, but why is the fix-it removing the entire method as opposed to just the attribute?

Also, are you sure that the source range for the method declaration is sufficient to remove without breaking code? e.g., what about the definition of the method? Or can there be any trailing bits after the method that are not accounted for in the source range (such as trailing attributes, if those are possible in ObjC)?

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst

30

This looks wrong to me -- it's a semicolon-delimited list, isn't it?

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m

35

If the common practice is to use macros like NS_UNAVAILABLE, should that be on the default list of options (along with any other common-use macro names that do the same thing)?

[njames93]

Can objc instance methods be redeclared, the AST suggests so, if so you should probably only warm on the first occurrence but create fix its for each.

[njames93]

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

35

Not a point for this patch, but maybe this should be made into an actual matcher, though not complimented, or better yet the CXXMethodDecl isOverride matcher be extended for ObjCMethodDecl

[mwyman]

After some discussion, have decided to remove the fix-it entirely and update the diagnostic message; removing the method altogether may not be the correct behavior, as previously deprecated methods that have since been removed may want to have an unavailable attribute attached with a message explaining what to use instead, even though they don't override a superclass method.

But in general still feel like non-overriding, non-messaged attributes are probably reasonable to flag.

[mwyman]

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

35

I agree, and also that it's probably best left to a later patch. Not sure how to deal with the making one that works for both CXXMethodDecl and ObjCMethodDecl.

64

Removing the attribute is undesirable, as that would make the compiler allow calling the method, when it has explicitly been marked unavailable. In my experience the majority of the cases in Objective-C for using this attribute are to mark a superclass method unavailable (e.g. a designated initializer that shouldn't be called on the subclass, even just -init, favoring its own declared designated initializer instead), but it can also be used to mark a previously-deprecated-now-removed method with a message to point updaters what method to migrate to.

Which brings up an interesting point that if a message is provided, it could be that an API has finally finished allowing a deprecated method to be called and removed it entirely, and so if a message is provided it may be indicating that and telling what new API to use, rather than overriding the availability of a superclass method. But a method being marked unavailable without any message doesn't really provide much help on that versus the method just not existing. Perhaps if a message is provided this check shouldn't complain at all.

I've removed the fix-it entirely, and replaced it with a different warning message.

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst

30

Obsolete, deleted.

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m

35

Obsolete, fix-it deleted.

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
   DeallocInCategoryCheck.cpp
   ForbiddenSubclassingCheck.cpp
+  MethodUnavailableNotOverrideCheck.cpp
   MissingHashCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
   SuperSelfCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.h

+//===--- MethodUnavailableNotOverrideCheck.h - clang-tidy -------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_METHODUNAVAILABLENOTOVERRIDECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_METHODUNAVAILABLENOTOVERRIDECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds methods marked with __attribute__((unavailable)) that do not override
+/// any declaration from a superclass and do also do not have a message.
+class MethodUnavailableNotOverrideCheck : public ClangTidyCheck {
+public:
+  MethodUnavailableNotOverrideCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.ObjC;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_METHODUNAVAILABLENOTOVERRIDECHECK_H

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp

+//===--- MethodUnavailableNotOverrideCheck.cpp - clang-tidy ---------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MethodUnavailableNotOverrideCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/Attrs.inc"
+#include "clang/AST/DeclObjC.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/AttrKinds.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+namespace {
+
+// Matches Objective-C methods that are not overriding a superclass method.
+AST_MATCHER(ObjCMethodDecl, isNotOverriding) { return !Node.isOverriding(); }
+
+} // anonymous namespace
+
+MethodUnavailableNotOverrideCheck::MethodUnavailableNotOverrideCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+void MethodUnavailableNotOverrideCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(objcMethodDecl(hasAttr(clang::attr::Unavailable),
+                                    isNotOverriding(),
+                                    hasDeclContext(objcInterfaceDecl()))
+                         .bind("decl"),
+                     this);
+}
+
+void MethodUnavailableNotOverrideCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MD = Result.Nodes.getNodeAs<ObjCMethodDecl>("decl");
+  const auto &UA = MD->getAttr<UnavailableAttr>();
+
+  // If a message has been provided,
+  if (UA->getMessageLength() > 0)
+    return;
+
+  SourceLocation AttrLoc = MD->getAttr<UnavailableAttr>()->getLocation();
+  diag(AttrLoc,
+       "methods marked unavailable should either override a superclass method "
+       "or explain why they are unavailable; consider adding a message to the "
+       "unavailable attribute, or simply deleting %0.")
+      << MD << MD->getSourceRange();
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp

 //===--- ObjCTidyModule.cpp - clang-tidy --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "DeallocInCategoryCheck.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "MethodUnavailableNotOverrideCheck.h"
 #include "MissingHashCheck.h"
 #include "PropertyDeclarationCheck.h"
 #include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
     CheckFactories.registerCheck<DeallocInCategoryCheck>(
         "objc-dealloc-in-category");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
+    CheckFactories.registerCheck<MethodUnavailableNotOverrideCheck>(
+        "objc-method-unavailable-not-override");
     CheckFactories.registerCheck<MissingHashCheck>(
         "objc-missing-hash");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
     CheckFactories.registerCheck<SuperSelfCheck>(
         "objc-super-self");
   }
 };

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`objc-dealloc-in-category
 
   Finds implementations of -dealloc in Objective-C categories.
 
 - New :doc:`misc-no-recursion
   <clang-tidy/checks/misc-no-recursion>` check.
 
   Finds recursive functions and diagnoses them.
 
+- New :doc:`objc-method-unavailable-not-override
+  <clang-tidy/checks/objc-method-unavailable-not-override>` check.
+
+  Checks that a method marked with ``__attribute__((unavailable))`` is
+  overriding a method declaration from a superclass, or has a message
+  explaining why it's unavailable.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-dcl37-c
   <clang-tidy/checks/cert-dcl37-c>` to
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 - New alias :doc:`cert-dcl51-cpp
   <clang-tidy/checks/cert-dcl51-cpp>` to
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`readability-qualified-auto
-  <clang-tidy/checks/readability-qualified-auto>` check now supports a 
+  <clang-tidy/checks/readability-qualified-auto>` check now supports a
   `AddConstToQualified` to enable adding ``const`` qualifiers to variables
   typed with ``auto *`` and ``auto &``.
 
 - Improved :doc:`readability-redundant-string-init
   <clang-tidy/checks/readability-redundant-string-init>` check now supports a
   `StringNames` option enabling its application to custom string classes. The
   check now detects in class initializers and constructor initializers which
   are deemed to be redundant.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `modernize-use-transparent-functors <modernize-use-transparent-functors.html>`_, "Yes"
    `modernize-use-uncaught-exceptions <modernize-use-uncaught-exceptions.html>`_, "Yes"
    `modernize-use-using <modernize-use-using.html>`_, "Yes"
    `mpi-buffer-deref <mpi-buffer-deref.html>`_, "Yes"
    `mpi-type-mismatch <mpi-type-mismatch.html>`_, "Yes"
    `objc-avoid-nserror-init <objc-avoid-nserror-init.html>`_,
    `objc-dealloc-in-category <objc-dealloc-in-category.html>`_,
    `objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
+   `objc-method-unavailable-not-override <objc-method-unavailable-not-override.html>`_, "Yes"
    `objc-missing-hash <objc-missing-hash.html>`_,
    `objc-property-declaration <objc-property-declaration.html>`_, "Yes"
    `objc-super-self <objc-super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp-exception-escape.html>`_,
    `openmp-use-default-none <openmp-use-default-none.html>`_,
    `performance-faster-string-find <performance-faster-string-find.html>`_, "Yes"
    `performance-for-range-copy <performance-for-range-copy.html>`_, "Yes"
    `performance-implicit-conversion-in-loop <performance-implicit-conversion-in-loop.html>`_,
.. csv-table::
    `readability-qualified-auto <readability-qualified-auto.html>`_, "Yes"
    `readability-redundant-access-specifiers <readability-redundant-access-specifiers.html>`_, "Yes"
    `readability-redundant-control-flow <readability-redundant-control-flow.html>`_, "Yes"
    `readability-redundant-declaration <readability-redundant-declaration.html>`_, "Yes"
    `readability-redundant-function-ptr-dereference <readability-redundant-function-ptr-dereference.html>`_, "Yes"
    `readability-redundant-member-init <readability-redundant-member-init.html>`_, "Yes"
    `readability-redundant-preprocessor <readability-redundant-preprocessor.html>`_,
    `readability-redundant-smartptr-get <readability-redundant-smartptr-get.html>`_, "Yes"
-   `readability-redundant-string-cstr <readability-redundant-string-cstr.html>`_,
+   `readability-redundant-string-cstr <readability-redundant-string-cstr.html>`_, "Yes"
    `readability-redundant-string-init <readability-redundant-string-init.html>`_, "Yes"
    `readability-simplify-boolean-expr <readability-simplify-boolean-expr.html>`_, "Yes"
    `readability-simplify-subscript-expr <readability-simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability-static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability-static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability-string-compare.html>`_, "Yes"
    `readability-uniqueptr-delete-release <readability-uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst

+.. title:: clang-tidy - objc-method-unavailable-not-override
+
+objc-method-unavailable-not-override
+====================================
+
+Checks that a method marked with ``__attribute__((unavailable))`` is overriding
+a method declaration from a superclass, or has a message explaining why it's
+unavailable.
+
+.. code-block:: objc
+
+   @interface ClassA : NSObject
+   // Neither ClassA nor any superclasses define method -foo.
+   @end
+
+   @interface ClassB : ClassA
+   - (void)foo __attribute__((unavailable));
+   @end

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m

+// RUN: %check_clang_tidy %s objc-method-unavailable-not-override %t -- \
+// RUN:     -config='{CheckOptions: [ \
+// RUN:         {key: objc-method-unavailable-not-override.FixMacroNames, \
+// RUN:          value: "UNAVAILABLE_FIXIT"} \
+// RUN:     ]}' --
+
+__attribute__((objc_root_class))
+@interface Object
+- (instancetype)init;
+@end
+
+@interface MyObject : Object
+- (instancetype)init __attribute__((unavailable));
+
+// A new method that is not overriding, and is available, should not trigger.
+- (void)notOverridingMethod;
+
+- (void)methodA __attribute__((unavailable)); // methodA
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: methods marked unavailable should either override a superclass method or explain why they are unavailable; consider adding a message to the unavailable attribute, or simply deleting 'methodA'. [objc-method-unavailable-not-override]
+
+// Verify check does NOT show when the unavailable attribute has a message.
+- (void)methodB __attribute__((unavailable("use methodE"))); // methodB
+
+#define UNAVAILABLE_ATTRIBUTE __attribute__((unavailable))
+#define UNAVAILABLE UNAVAILABLE_ATTRIBUTE
+
+// Verify check flags a macro that expands to the unavailable attribute.
+- (void)methodC UNAVAILABLE; // methodC
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: methods marked unavailable should either override a superclass method or explain why they are unavailable; consider adding a message to the unavailable attribute, or simply deleting 'methodC'. [objc-method-unavailable-not-override]
+
+@end
+
+
+@implementation MyObject
+
+- (void)notOverridingMethod {}
+
+// Should not flag implementations for methods declared unavailable; sometimes
+// implemementations are provided that assert, to catch such codepaths that
+// lead to those calls during development.
+- (void)methodA {}
+
+@end