mwyman created this revision.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
mwyman added reviewers: stephanemoore, benhamilton, dmaclach.
mwyman added a project: clang-tools-extra.
mwyman edited the summary of this revision.
This check is similar to an ARC Migration check that warned about this
incorrect usage under ARC, but most projects are no longer undergoing migration
from pre-ARC code. The documentation for NSInvocation is not explicit about
these requirements and incorrect usage has been found in many of our projects.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77571
Files:
clang-tools-extra/clang-tidy/objc/CMakeLists.txt
clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.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-nsinvocation-argument-lifetime.rst
clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
Index: clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s objc-nsinvocation-argument-lifetime %t -- -fobjc-arc
+
+__attribute__((objc_root_class))
+@interface NSObject
+@end
+
+@interface NSInvocation : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+- (void)getReturnValue:(void *)ReturnValue;
+@end
+
+@interface OtherClass : NSObject
+- (void)getArgument:(void *)Arg atIndex:(int)Index;
+@end
+
+void foo(NSInvocation *Invocation) {
+ __unsafe_unretained id Arg2;
+ id Arg3;
+ // CHECK-FIXES: __unsafe_unretained id Arg3;
+ NSObject __strong *Arg4;
+ // CHECK-FIXES: NSObject __unsafe_unretained *Arg4;
+ __weak id Arg5;
+ // CHECK-FIXES: __unsafe_unretained id Arg5;
+ id ReturnValue;
+ // CHECK-FIXES: __unsafe_unretained id ReturnValue;
+
+ [Invocation getArgument:&Arg2 atIndex:2];
+
+ [Invocation getArgument:&Arg3 atIndex:3];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&Arg4 atIndex:4];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:&Arg5 atIndex:5];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getReturnValue:&ReturnValue];
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: NSInvocation's 'getReturnValue:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+
+ [Invocation getArgument:(void*)0 atIndex:6];
+}
+
+void bar(OtherClass *OC) {
+ id Arg;
+ [OC getArgument:&Arg atIndex:2];
+}
+
+@interface TestClass : NSObject {
+ id Argument;
+}
+@end
+
+@implementation TestClass
+
+- (void)processInvocation:(NSInvocation *)Invocation {
+ [Invocation getArgument:&Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&self->Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation's 'getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+}
+
+@end
Index: clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
@@ -0,0 +1,39 @@
+.. title:: clang-tidy - objc-nsinvocation-argument-lifetime
+
+objc-nsinvocation-argument-lifetime
+===================================
+
+Finds calls to NSInvocation methods under ARC that don't have proper
+argument object lifetimes. When passing Objective-C objects as parameters
+to the NSInvocation methods ``getArgument:atIndex:`` and ``getReturnValue:``,
+the values are copied by value into the argument pointer, which leads to
+to incorrect releasing behavior if the object pointers are not declared
+``__unsafe_unretained``.
+
+For code:
+
+.. code-block:: objc
+
+ id arg;
+ [invocation getArgument:&arg atIndex:2];
+
+ __strong id returnValue;
+ [invocation getReturnValue:&returnValue];
+
+The fix will be:
+
+.. code-block:: objc
+
+ __unsafe_unretained id arg;
+ [invocation getArgument:&arg atIndex:2];
+
+ __unsafe_unretained id returnValue;
+ [invocation getReturnValue:&returnValue];
+
+The check will warn on being passed instance variable references that have
+lifetimes other than ``__unsafe_unretained``, but does not propose a fix:
+
+.. code-block:: objc
+
+ // "id _returnValue" is declaration of instance variable of class.
+ [invocation getReturnValue:&self->_returnValue];
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
@@ -239,6 +239,7 @@
`objc-dealloc-in-category <objc-dealloc-in-category.html>`_,
`objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
`objc-missing-hash <objc-missing-hash.html>`_,
+ `objc-nsinvocation-argument-lifetime <objc-nsinvocation-argument-lifetime.html>`_, "Yes"
`objc-property-declaration <objc-property-declaration.html>`_, "Yes"
`objc-super-self <objc-super-self.html>`_, "Yes"
`openmp-exception-escape <openmp-exception-escape.html>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -129,6 +129,12 @@
Finds recursive functions and diagnoses them.
+- New :doc:`objc-nsinvocation-argument-lifetime
+ <clang-tidy/checks/objc-nsinvocation-argument-lifetime>` check.
+
+ Finds calls to NSInvocation methods under ARC that don't have proper
+ argument object lifetimes.
+
New check aliases
^^^^^^^^^^^^^^^^^
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
@@ -13,6 +13,7 @@
#include "DeallocInCategoryCheck.h"
#include "ForbiddenSubclassingCheck.h"
#include "MissingHashCheck.h"
+#include "NsinvocationArgumentLifetimeCheck.h"
#include "PropertyDeclarationCheck.h"
#include "SuperSelfCheck.h"
@@ -33,6 +34,8 @@
"objc-forbidden-subclassing");
CheckFactories.registerCheck<MissingHashCheck>(
"objc-missing-hash");
+ CheckFactories.registerCheck<NsinvocationArgumentLifetimeCheck>(
+ "objc-nsinvocation-argument-lifetime");
CheckFactories.registerCheck<PropertyDeclarationCheck>(
"objc-property-declaration");
CheckFactories.registerCheck<SuperSelfCheck>(
Index: clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.h
@@ -0,0 +1,39 @@
+//===--- NsinvocationArgumentLifetimeCheck.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_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_NSINVOCATIONARGUMENTLIFETIMECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Basic/LangStandard.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds calls to NSInvocation methods under ARC that don't have proper
+/// argument object lifetimes.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-nsinvocation-argument-lifetime.html
+class NsinvocationArgumentLifetimeCheck : public ClangTidyCheck {
+public:
+ NsinvocationArgumentLifetimeCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.ObjC && LangOpts.ObjCAutoRefCount;
+ }
+ 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_NSINVOCATIONARGUMENTLIFETIMECHECK_H
Index: clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
@@ -0,0 +1,133 @@
+//===--- NsinvocationArgumentLifetimeCheck.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 "NsinvocationArgumentLifetimeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attrs.inc"
+#include "clang/AST/ComputeDependence.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/ExprObjC.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+namespace {
+
+constexpr StringRef WeakText = "__weak";
+constexpr StringRef StrongText = "__strong";
+constexpr StringRef UnsafeUnretainedText = "__unsafe_unretained";
+
+/// Matches ObjCIvarRefExpr or DeclRefExpr that reference Objective-C object
+/// variables whose object lifetimes are not __unsafe_unretained.
+AST_POLYMORPHIC_MATCHER(isObjCManagedLifetime,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(ObjCIvarRefExpr,
+ DeclRefExpr)) {
+ QualType QT = Node.getType();
+ return QT->getScalarTypeKind() == Type::STK_ObjCObjectPointer &&
+ QT.getQualifiers().getObjCLifetime() > Qualifiers::OCL_ExplicitNone;
+}
+
+llvm::Optional<FixItHint> replacementForOwnershipString(StringRef Text,
+ CharSourceRange Range,
+ StringRef Ownership) {
+ size_t Index = Text.find(Ownership);
+ if (Index == StringRef::npos)
+ return llvm::None;
+
+ SourceLocation Begin = Range.getBegin().getLocWithOffset(Index);
+ SourceLocation End = Begin.getLocWithOffset(Ownership.size());
+ return FixItHint::CreateReplacement(SourceRange(Begin, End),
+ UnsafeUnretainedText);
+}
+
+llvm::Optional<FixItHint> fixItForVarDecl(const VarDecl *VD,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ assert(VD && "VarDecl parameter must not be null");
+ // Don't provide fix-its for any parameter variables at this time.
+ if (isa<ParmVarDecl>(VD))
+ return llvm::None;
+
+ // Currently there is no way to directly get the source range for the
+ // __weak/__strong ObjC lifetime qualifiers, so it's necessary to string
+ // search in the source code.
+ CharSourceRange Range = Lexer::makeFileCharRange(
+ CharSourceRange::getTokenRange(VD->getSourceRange()), SM, LangOpts);
+ if (Range.isInvalid()) {
+ // An invalid range likely means inside a macro, in which case don't supply
+ // a fix-it.
+ return llvm::None;
+ }
+
+ StringRef VarDeclText = Lexer::getSourceText(Range, SM, LangOpts);
+ if (auto Hint = replacementForOwnershipString(VarDeclText, Range, WeakText)) {
+ return Hint;
+ }
+
+ if (auto Hint =
+ replacementForOwnershipString(VarDeclText, Range, StrongText)) {
+ return Hint;
+ }
+
+ return FixItHint::CreateInsertion(Range.getBegin(), "__unsafe_unretained ");
+}
+
+} // namespace
+
+void NsinvocationArgumentLifetimeCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ objcMessageExpr(
+ hasReceiverType(asString("NSInvocation *")),
+ anyOf(hasSelector("getArgument:atIndex:"),
+ hasSelector("getReturnValue:")),
+ hasArgument(
+ 0, anyOf(hasDescendant(objcIvarRefExpr(isObjCManagedLifetime())),
+ hasDescendant(declRefExpr(to(varDecl().bind("var")),
+ isObjCManagedLifetime())))))
+ .bind("call"),
+ this);
+}
+
+void NsinvocationArgumentLifetimeCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *MatchedExpr = Result.Nodes.getNodeAs<ObjCMessageExpr>("call");
+
+ auto Diag = diag(MatchedExpr->getArg(0)->getBeginLoc(),
+ "NSInvocation's %0 should only pass pointers to "
+ "objects with ownership __unsafe_unretained")
+ << MatchedExpr->getSelector();
+
+ // Only provide fix-it hints for references to local variables; fixes for
+ // instance variable references don't have as clear an automated fix.
+ const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+ if (!VD)
+ return;
+
+ if (auto Hint = fixItForVarDecl(VD, *Result.SourceManager,
+ Result.Context->getLangOpts())) {
+ Diag << *Hint;
+ }
+}
+
+} // 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
@@ -5,6 +5,7 @@
DeallocInCategoryCheck.cpp
ForbiddenSubclassingCheck.cpp
MissingHashCheck.cpp
+ NsinvocationArgumentLifetimeCheck.cpp
ObjCTidyModule.cpp
PropertyDeclarationCheck.cpp
SuperSelfCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
