MosheBerman created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp,
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware,
xazax.hun.
Herald added a project: All.
MosheBerman requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This diff adds:
- A `CmdLineOption` called `ShowFixIts` to the all of nullability checks. (We
add to all of them because of the way `NullabilityChecker.cpp` registers all of
the checkers.)
- For each of the two `*ReturnedFromNonnull` methods, attaches a `FixItHint` to
the output.
Use Case:
This enables us to automate the process of annotating headers with
`NS_ASSUME_NONNULL_BEGIN/END` because the checker can fix callsites where we
would otherwise break the nullability contract.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D123352
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
clang/test/Analysis/nullability-fixits.mm
Index: clang/test/Analysis/nullability-fixits.mm
===================================================================
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,56 @@
+// RUN: %clang_analyze_cc1 %s -analyzer-checker=core,nullability.NullReturnedFromNonnull,nullability.NullableReturnedFromNonnull \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config apply-fixits=true \
+// RUN: -analyzer-output=plist \
+// RUN: -fobjc-arc \
+// RUN: -o - | FileCheck %s
+
+#define nil 0
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+- (instancetype)autorelease;
+- (void)release;
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObject<NSObject>
+@end
+
+
+@interface TestObject : NSObject
+@end
+
+// CHECK: TestObject *_Nullable
+TestObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+ TestObject *local = nil;
+ return local; // expected-warning {{nil returned from a function that is expected to return a non-null value}}
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+ return nil; // expected-warning {{nil returned from a function that is expected to return a non-null value}}
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+// FIXME: We want this check for when we add support for syntactic sugar, and
+// put the annotation before the type name. `nullable` vs `_Nonnull`.
+// CHECK: - (TestObject *_Nonnull) returnsNilUnsuppressed {
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+ return nil; // expected-warning {{nil returned from a method that is expected to return a non-null value}}
+}
+
+- (TestObject *_Nullable)returnsNil {
+ return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+ TestObject *o = [self returnsNil];
+ return o;
+}
+@end
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -92,6 +92,11 @@
// libraries.
DefaultBool NoDiagnoseCallsToSystemHeaders;
+ // If true, the checker will generate Fix-it-hints for *ReturnedToNonnull
+ // warnings. Should we consider changing the invariant behavior for `nil`
+ // and/or NS collection classes if this is enabled?
+ DefaultBool ShowFixIts;
+
void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext &C) const;
void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
@@ -152,6 +157,14 @@
const MemRegion *Region;
};
+ // `getReturnTypeSourceRange` does not include qualifiers. As a result, the
+ // nullability annotation will precede the asterisk, instead of following it.
+ // This helper function accounts for that.
+ SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *typeSourceInfo,
+ const size_t offset = 0) const {
+ return typeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(offset);
+ }
+
/// When any of the nonnull arguments of the analyzed function is null, do not
/// report anything and turn off the check.
///
@@ -160,12 +173,13 @@
void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
ExplodedNode *N, const MemRegion *Region,
CheckerContext &C,
- const Stmt *ValueExpr = nullptr,
- bool SuppressPath = false) const;
+ const Stmt *ValueExpr = nullptr, bool SuppressPath = false,
+ const SourceLocation *FixItHintInsertionLoc = nullptr) const;
void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
const MemRegion *Region, BugReporter &BR,
- const Stmt *ValueExpr = nullptr) const {
+ const Stmt *ValueExpr = nullptr,
+ const SourceLocation *FixItHintInsertionLoc = nullptr) const {
const std::unique_ptr<BugType> &BT = getBugType(CK);
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
if (Region) {
@@ -180,6 +194,14 @@
if (const auto *Ex = dyn_cast<Expr>(ValueExpr))
bugreporter::trackExpressionValue(N, Ex, *R);
}
+
+ // If CheckFixIts is false, we will have received a nullptr from above.
+ if (this->ShowFixIts && FixItHintInsertionLoc) {
+ // FIXME: consider prefix style `nullable` vs `_Nullable`.
+ const clang::FixItHint hint =
+ FixItHint::CreateInsertion(*FixItHintInsertionLoc, "_Nullable");
+ R->addFixItHint(hint);
+ }
BR.emitReport(std::move(R));
}
@@ -437,7 +459,7 @@
void NullabilityChecker::reportBugIfInvariantHolds(
StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr,
- bool SuppressPath) const {
+ bool SuppressPath, const SourceLocation *FixItHintInsertionLoc) const {
ProgramStateRef OriginalState = N->getState();
if (checkInvariantViolation(OriginalState, N, C))
@@ -447,7 +469,9 @@
N = C.addTransition(OriginalState, N);
}
- reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
+ reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr,
+ FixItHintInsertionLoc =
+ this->ShowFixIts ? FixItHintInsertionLoc : nullptr);
}
/// Cleaning up the program state.
@@ -586,9 +610,12 @@
bool InSuppressedMethodFamily = false;
QualType RequiredRetType;
+ TypeSourceInfo *RetTypeSourceInfo;
+
AnalysisDeclContext *DeclCtxt =
C.getLocationContext()->getAnalysisDeclContext();
const Decl *D = DeclCtxt->getDecl();
+
if (auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
// HACK: This is a big hammer to avoid warning when there are defensive
// nil checks in -init and -copy methods. We should add more sophisticated
@@ -599,8 +626,10 @@
InSuppressedMethodFamily = true;
RequiredRetType = MD->getReturnType();
+ RetTypeSourceInfo = MD->getReturnTypeSourceInfo();
} else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
RequiredRetType = FD->getReturnType();
+ RetTypeSourceInfo = FD->getTypeSourceInfo();
} else {
return;
}
@@ -619,6 +648,9 @@
bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
+ const SourceLocation FixItHintInsertionLoc = getInsertionLocForTypeInfo(
+ RetTypeSourceInfo, RequiredRetType.getAsString().length());
+
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
!InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) {
@@ -632,9 +664,10 @@
OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
OS << " returned from a " << C.getDeclDescription(D) <<
" that is expected to return a non-null value";
+
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
CK_NullReturnedFromNonnull, N, nullptr, C,
- RetExpr);
+ RetExpr, false, &FixItHintInsertionLoc);
return;
}
@@ -667,7 +700,8 @@
" that is expected to return a non-null value";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NullableReturnedToNonnull,
- CK_NullableReturnedFromNonnull, N, Region, C);
+ CK_NullableReturnedFromNonnull, N, Region, C,
+ nullptr, false, &FixItHintInsertionLoc);
}
return;
}
@@ -1238,6 +1272,7 @@
#define REGISTER_CHECKER(name, trackingRequired) \
void ento::register##name##Checker(CheckerManager &mgr) { \
+ const AnalyzerOptions &AnOpts = mgr.getAnalyzerOptions(); \
NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>(); \
checker->ChecksEnabled[NullabilityChecker::CK_##name] = true; \
checker->CheckNames[NullabilityChecker::CK_##name] = \
@@ -1245,8 +1280,10 @@
checker->NeedTracking = checker->NeedTracking || trackingRequired; \
checker->NoDiagnoseCallsToSystemHeaders = \
checker->NoDiagnoseCallsToSystemHeaders || \
- mgr.getAnalyzerOptions().getCheckerBooleanOption( \
+ AnOpts.getCheckerBooleanOption( \
checker, "NoDiagnoseCallsToSystemHeaders", true); \
+ checker->ShowFixIts = \
+ AnOpts.getCheckerBooleanOption(checker, "ShowFixIts"); \
} \
\
bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -304,39 +304,58 @@
// Nullability checkers.
//===----------------------------------------------------------------------===//
+
+// Although only some checkers support fix-its, all of the nullability checkers
+// are registered by the same macro. So let's keep it simple by re-using the
+// same option definition, and document where it is actually used in the helptext.
+// Added bonus: if we _do_ add more nullability hints, the flag is already in place.
+def NullabilityFixIts: CmdLineOption<Boolean,
+ "ShowFixIts",
+ "Enable fix-it hints for this nullability checkers. "
+ "Fix-it hints are only supported by the checkers "
+ "NullReturnedFromNonnull and NullableReturnedFromNonnull",
+ "false",
+ InAlpha>;
+
let ParentPackage = Nullability in {
def NullabilityBase : Checker<"NullabilityBase">,
HelpText<"Stores information during the analysis about nullability.">,
+ CheckerOptions<[NullabilityFixIts]>,
Documentation<NotDocumented>,
Hidden;
def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
HelpText<"Warns when a null pointer is passed to a pointer which has a "
"_Nonnull type.">,
+ CheckerOptions<[NullabilityFixIts]>,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">,
HelpText<"Warns when a null pointer is returned from a function that has "
"_Nonnull return type.">,
+ CheckerOptions<[NullabilityFixIts]>,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullableDereferencedChecker : Checker<"NullableDereferenced">,
HelpText<"Warns when a nullable pointer is dereferenced.">,
+ CheckerOptions<[NullabilityFixIts]>,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">,
HelpText<"Warns when a nullable pointer is passed to a pointer which has a "
"_Nonnull type.">,
+ CheckerOptions<[NullabilityFixIts]>,
Dependencies<[NullabilityBase]>,
Documentation<HasDocumentation>;
def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">,
HelpText<"Warns when a nullable pointer is returned from a function that has "
"_Nonnull return type.">,
+ CheckerOptions<[NullabilityFixIts]>,
Dependencies<[NullabilityBase]>,
Documentation<NotDocumented>;
@@ -475,12 +494,12 @@
HelpText<"Check for arguments which are not null-terminating strings">,
Dependencies<[CStringModeling]>,
Documentation<HasAlphaDocumentation>;
-
+
def CStringUninitializedRead : Checker<"UninitializedRead">,
HelpText<"Checks if the string manipulation function would read uninitialized bytes">,
Dependencies<[CStringModeling]>,
Documentation<HasAlphaDocumentation>;
-
+
} // end "alpha.unix.cstring"
let ParentPackage = Unix in {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
