mwyman updated this revision to Diff 249709.
mwyman marked 5 inline comments as done.
mwyman added a comment.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75569/new/
https://reviews.llvm.org/D75569
Files:
clang-tools-extra/clang-tidy/objc/CMakeLists.txt
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.h
clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
Index: clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
@@ -0,0 +1,43 @@
+// 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
Index: clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
@@ -0,0 +1,18 @@
+.. 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
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -236,6 +236,7 @@
`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"
@@ -283,7 +284,7 @@
`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"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -98,6 +98,13 @@
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
^^^^^^^^^^^^^^^^^
@@ -115,7 +122,7 @@
^^^^^^^^^^^^^^^^^^^^^^^^^^
- 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 &``.
Index: clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
+++ clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp
@@ -12,6 +12,7 @@
#include "AvoidNSErrorInitCheck.h"
#include "DeallocInCategoryCheck.h"
#include "ForbiddenSubclassingCheck.h"
+#include "MethodUnavailableNotOverrideCheck.h"
#include "MissingHashCheck.h"
#include "PropertyDeclarationCheck.h"
#include "SuperSelfCheck.h"
@@ -31,6 +32,8 @@
"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>(
Index: clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.h
@@ -0,0 +1,34 @@
+//===--- 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
Index: clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
@@ -0,0 +1,69 @@
+//===--- 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
Index: clang-tools-extra/clang-tidy/objc/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/objc/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/objc/CMakeLists.txt
@@ -4,6 +4,7 @@
AvoidNSErrorInitCheck.cpp
DeallocInCategoryCheck.cpp
ForbiddenSubclassingCheck.cpp
+ MethodUnavailableNotOverrideCheck.cpp
MissingHashCheck.cpp
ObjCTidyModule.cpp
PropertyDeclarationCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
