MosheBerman updated this revision to Diff 422902.
MosheBerman added a comment.
The tests now pass and actually verify the behavior.
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,70 @@
+// This test runs the nullability checkers with fixits enabled to verify that the
+// fixits are produced as expected. Note that we disable `-Wnonnull` since it
+// pollutes the output and doesn't provide fixits.
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=false \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=false \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-EXPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-ENABLED
+
+
+// CHECK-FIXIT-ENABLED: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+// CHECK-FIXIT-DISABLED-EXPLICIT-NOT: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+// CHECK-FIXIT-DISABLED-IMPLICIT-NOT: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+
+#include "Inputs/system-header-simulator-for-nullability.h"
+
+@interface TestObject : NSObject
+@end
+
+NSObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+ TestObject *local = nil;
+ return local;
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+ return nil;
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+ return nil;
+}
+
+- (TestObject *_Nullable)returnsNil {
+ return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+ TestObject *o = [self returnsNil];
+ return o;
+}
+@end
+
+NS_ASSUME_NONNULL_BEGIN
+
+NSObject * returnsNilAssumeNonnull() {
+ return nil;
+}
+
+NS_ASSUME_NONNULL_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;
}
@@ -616,9 +643,29 @@
// return (NSString * _Nonnull)0;
Nullability RetExprTypeLevelNullability =
getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
-
bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
+
+ llvm::outs() << "RetExprTypeLevelNullability: " << getNullabilityString(RetExprTypeLevelNullability) << "\n"
+ << "RequiredNullability: " << getNullabilityString(RequiredNullability) << "\n";
+ bool IsContextSensitive = dyn_cast<ObjCMethodDecl>(D) != nullptr;
+ const auto RequiredRetTypeStr = RequiredRetType.getAsString();
+ const SourceLocation FixItHintInsertionLoc = getInsertionLocForTypeInfo(
+ RetTypeSourceInfo, RequiredRetTypeStr.length(),
+ IsContextSensitive);
+ const auto SourceRangeToInspect = D.getSourceRange().printToString(C.getSourceManager());
+ // const SourceManager &SM = C.getSourceManager();
+
+ // range of RetTypeSourceInfo
+ // range ending in FixItHintInsertionLoc
+ // Create range from end of RetTypeSourceInfo to end of FixItHintInsertionLoc
+ // Look for _Nonnull in that range
+ // Look for nonnull in RetTypeSourceInfo
+ // If we find either of them, replace _Nonnull with _Nullable
+ // Otherwise just insert _Nullable or nullable
+
+ const FixItHint Hint =
+ FixItHint::CreateInsertion(FixItHintInsertionLoc, getNullabilitySpelling(NullabilityKind::Nullable, IsContextSensitive));
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
!InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) {
@@ -631,10 +678,10 @@
llvm::raw_svector_ostream OS(SBuf);
OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
OS << " returned from a " << C.getDeclDescription(D) <<
- " that is expected to return a non-null value";
+ " that is expected to return a non-null value foo";
+
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
- CK_NullReturnedFromNonnull, N, nullptr, C,
- RetExpr);
+ CK_NullReturnedFromNonnull, N, nullptr, C, RetExpr, false, &Hint);
return;
}
@@ -665,9 +712,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 +1284,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 +1292,10 @@
checker->NeedTracking = checker->NeedTracking || trackingRequired; \
checker->NoDiagnoseCallsToSystemHeaders = \
checker->NoDiagnoseCallsToSystemHeaders || \
- mgr.getAnalyzerOptions().getCheckerBooleanOption( \
+ AnOpts.getCheckerBooleanOption( \
checker, "NoDiagnoseCallsToSystemHeaders", true); \
+ checker->ShowFixIts[NullabilityChecker::CheckKind::CK_##name] = \
+ AnOpts.getCheckerBooleanOption(mgr.getCurrentCheckerName(), "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",
+ Released>;
+
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
