MosheBerman updated this revision to Diff 421587.
MosheBerman added a comment.
- Changed the ShowFixIts flag to be per-checker.
- Added support for syntax sugar (`nullable` vs `_Nullable`)
- Pass FixItHint through, so we can do that.
- Changed tests to use `-fdiagnostics-parseable-fixits`.
This doesn't yet account for removing existing `_Nonnull` local annotations,
but I want to make sure we can get tests working first and/or determine if a
tidy is a better approach.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123352/new/
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,52 @@
+// RUN: %clang_analyzer_cc1 %s -analyzer-checker=core,nullability.NullReturnedFromNonnull,nullability.NullableReturnedFromNonnull \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -fobjc-arc \
+// RUN: -fdiagnostics-parseable-fixits \
+// RUN: 2&>1 -o - | FileCheck %s -check-prefix=FIXIT
+
+// This test runs the nullability checkers with fixits enabled. We're piping
+// the diagnostic output to FileCheck, not output of the clang invocation.
+
+#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
+
+TestObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+ TestObject *local = nil; // FIXIT: fix-it:{{.*}}:27}:"nullable TestObject"
+ return local;
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+ return nil; // FIXIT: fix-it:{{.*}}:32}:"TestObject *_Nullable"
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+ return nil; // FIXIT: fix-it:{{.*}}:41}:"nullable TestObject *"
+}
+
+- (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
@@ -117,6 +117,9 @@
DefaultBool ChecksEnabled[CK_NumCheckKinds];
CheckerNameRef CheckNames[CK_NumCheckKinds];
+ // FIXME: Should we consider changing the invariant behavior for `nil`
+ // and/or NS collection classes if this is enabled?
+ DefaultBool ShowFixIts[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
@@ -152,6 +155,20 @@
const MemRegion *Region;
};
+ /// There are a few cases where we want to move the insertion point.
+ /// 1. If we are inserting after the pointer, we need to offset ourselves,
+ /// because `getReturnTypeSourceRange` does not include qualifiers so the
+ /// annotation would be inserted incorrectly before the pointer symbol.
+ /// 2. If we are annotating an Objective-C method, and not a function, we
+ /// want to use the `nullable` form instead of `_Nullable`.
+ /// When \p SyntaxSugar is true, we handle the second case.
+ SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *TypeSourceInfo,
+ const size_t Offset = 0,
+ bool SyntaxSugar = false) const {
+ return TypeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(
+ SyntaxSugar ? 0 : Offset);
+ }
+
/// When any of the nonnull arguments of the analyzed function is null, do not
/// report anything and turn off the check.
///
@@ -160,12 +177,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 FixItHint *Hint = 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 FixItHint *Hint = nullptr) const {
const std::unique_ptr<BugType> &BT = getBugType(CK);
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
if (Region) {
@@ -180,6 +198,10 @@
if (const auto *Ex = dyn_cast<Expr>(ValueExpr))
bugreporter::trackExpressionValue(N, Ex, *R);
}
+
+ if (ShowFixIts[CK] && Hint) {
+ 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 FixItHint *Hint) const {
ProgramStateRef OriginalState = N->getState();
if (checkInvariantViolation(OriginalState, N, C))
@@ -447,7 +469,8 @@
N = C.addTransition(OriginalState, N);
}
- reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
+ reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr,
+ Hint);
}
/// Cleaning up the program state.
@@ -586,9 +609,11 @@
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 +624,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;
}
@@ -617,8 +644,16 @@
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
+ bool IsObjCPointerType = RetExpr->getType()->isObjCObjectPointerType();
+ const SourceLocation FixItHintInsertionLoc = getInsertionLocForTypeInfo(
+ RetTypeSourceInfo, RequiredRetType.getAsString().length(),
+ IsObjCPointerType);
+ const auto AnnotationString = IsObjCPointerType ? "nullable" : "_Nullable";
+
bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
+ const FixItHint Hint =
+ FixItHint::CreateInsertion(FixItHintInsertionLoc, AnnotationString);
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
!InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) {
@@ -629,12 +664,12 @@
SmallString<256> SBuf;
llvm::raw_svector_ostream OS(SBuf);
- OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
+ OS << (IsObjCPointerType ? "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);
+ CK_NullReturnedFromNonnull, N, nullptr, C, RetExpr, false, &Hint);
return;
}
@@ -665,9 +700,8 @@
llvm::raw_svector_ostream OS(SBuf);
OS << "Nullable pointer is returned from a " << C.getDeclDescription(D) <<
" 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, &Hint);
}
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[NullabilityChecker::CK_##name] = \
+ 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
