MosheBerman updated this revision to Diff 429065.
MosheBerman added a comment.
Incorporated feedback:
- Removed confusing comment about NS-prefixed classes
- Use StringRef instead of str
- Fixed case where `isa` was more appropriate
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/analyzer-config.c
clang/test/Analysis/nullability-fixits.mm
Index: clang/test/Analysis/nullability-fixits.mm
===================================================================
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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 \
+// 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 \
+// 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 \
+// 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
+
+// RUN: cp %s %t
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config apply-fixits=true %s
+
+// Use grep to skip lines with comments to avoid matching on checks or RUN lines:
+// RUN: cat %s | grep -v -E "\/\/|^\s*\$" | FileCheck %t -check-prefix=CHECK-FIXIT-VERIFY-OUT
+// RUN: cp %t %s
+
+// 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
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nullable)returnsNil
+- (TestObject *_Nonnull)returnsNil {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject<NSObject> *_Nullable)returnsNilProtocol
+- (TestObject<NSObject> *_Nonnull)returnsNilProtocol {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject<NSObject> *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject<NSObject> *)returnsNilProtocolSyntaxSugar {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (id)returnsNilId
+- (id)returnsNilId {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (instancetype)returnsNilInstancetype
+- (instancetype)returnsNilInstancetype {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilSyntaxSugar
+- (nonnull TestObject *)returnsNilSyntaxSugar {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nonnull)returnsNilSuppressed
+- (TestObject *_Nonnull)returnsNilSuppressed {
+ return (TestObject *_Nonnull)nil;
+}
+
+// Implicitly unspecified are not auto-fixed.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *)returnsNilUnannotated
+- (TestObject *)returnsNilUnannotated {
+ return nil;
+}
+
+// Implicitly unspecified are not auto-fixed.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Null_unspecified)returnsNilUnannotatedExplicitlyUnspecified
+- (TestObject *_Null_unspecified)returnsNilUnannotatedExplicitlyUnspecified {
+ return nil;
+}
+
+NS_ASSUME_NONNULL_BEGIN
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilAssumeInClass
+- (TestObject *)returnsNilAssumeInClass {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nullable)returnsNilAssumeInClass
+- (TestObject *_Nonnull)returnsNilAssumeInClassAnnotated {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilAssumeInClassAnnotatedSyntaxSugar
+- (nonnull TestObject *)returnsNilAssumeInClassAnnotatedSyntaxSugar {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Null_unspecified)returnsNilAssumeInClassExplicitlyUnspecified
+- (TestObject *_Null_unspecified)returnsNilAssumeInClassExplicitlyUnspecified {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (id)returnsNilIdAssume
+- (id)returnsNilIdAssume {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (instancetype)returnsNilInstancetypeAssume
+- (instancetype)returnsNilInstancetypeAssume {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject<NSObject> *)returnsNilProtocolAssume
+- (TestObject<NSObject> *)returnsNilProtocolAssume {
+ return nil;
+}
+NS_ASSUME_NONNULL_END
+
+// Self is not nil here.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+ TestObject *o = [self returnsNil];
+ return o;
+}
+@end
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilObjCInstanceIndirectly() {
+NSObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+ TestObject *local = nil;
+ return local;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: TestObject *_Nullable returnsNilObjCInstanceDirectly() {
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+ return nil;
+}
+
+NS_ASSUME_NONNULL_BEGIN
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilFuncAssume() {
+NSObject *returnsNilFuncAssume() {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: TestObject<NSObject> *_Nullable returnsNilFuncProtocolAssume() {
+TestObject<NSObject> *returnsNilFuncProtocolAssume() {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Null_unspecified returnsNilFuncAssumeNonnullExplicitlyUnspecified() {
+NSObject *_Null_unspecified returnsNilFuncAssumeNonnullExplicitlyUnspecified() {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilFuncAssumeNonnullExplicitlyAnnotated() {
+NSObject *_Nonnull returnsNilFuncAssumeNonnullExplicitlyAnnotated() {
+ return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: id returnsIdFunc() {
+id returnsIdFunc() {
+ return nil;
+}
+
+// We don't need to check instancetype because this is outside a class
+
+NS_ASSUME_NONNULL_END
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -96,6 +96,12 @@
// CHECK-NEXT: mode = deep
// CHECK-NEXT: model-path = ""
// CHECK-NEXT: notes-as-events = false
+// CHECK-NEXT: nullability.NullPassedToNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullReturnedFromNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullabilityBase:ShowFixIts = false
+// CHECK-NEXT: nullability.NullableDereferenced:ShowFixIts = false
+// CHECK-NEXT: nullability.NullablePassedToNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullableReturnedFromNonnull:ShowFixIts = false
// CHECK-NEXT: nullability:NoDiagnoseCallsToSystemHeaders = false
// CHECK-NEXT: objc-inlining = true
// CHECK-NEXT: optin.cplusplus.UninitializedObject:CheckPointeeInitialization = false
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -117,6 +117,7 @@
DefaultBool ChecksEnabled[CK_NumCheckKinds];
CheckerNameRef CheckNames[CK_NumCheckKinds];
+ DefaultBool ShowFixIts[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
@@ -161,11 +162,13 @@
ExplodedNode *N, const MemRegion *Region,
CheckerContext &C,
const Stmt *ValueExpr = nullptr,
- bool SuppressPath = false) const;
+ bool SuppressPath = false,
+ const llvm::Optional<FixItHint> Hint = None) 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 llvm::Optional<FixItHint> Hint = None) const {
const std::unique_ptr<BugType> &BT = getBugType(CK);
auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
if (Region) {
@@ -180,6 +183,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 +444,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 llvm::Optional<FixItHint> Hint) const {
ProgramStateRef OriginalState = N->getState();
if (checkInvariantViolation(OriginalState, N, C))
@@ -447,7 +454,7 @@
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.
@@ -564,6 +571,50 @@
return E->IgnoreImpCasts();
}
+llvm::Optional<FixItHint> getFixItHint(TypeLoc RetTypeLoc,
+ bool CallsiteSupportsSyntaxSugar) {
+ const SourceLocation NullabilityLoc = RetTypeLoc.findNullabilityLoc();
+ const auto CanonicalTypeStr =
+ RetTypeLoc.getType().getCanonicalType().getAsString();
+ if (StringRef(CanonicalTypeStr).equals("id") || StringRef(CanonicalTypeStr).equals("instancetype")) {
+ return None;
+ }
+ // If we're inside of an NS_ASSUME, then the sourceRange will end before the
+ // asterisk.
+ const auto CanonicalTypeSize = CanonicalTypeStr.size();
+ const bool IsInsideOfAssume =
+ NullabilityLoc == RetTypeLoc.getSourceRange().getBegin().getLocWithOffset(
+ CanonicalTypeSize - 1);
+
+ const bool UseSyntaxSugar = CallsiteSupportsSyntaxSugar &&
+ (!(NullabilityLoc.isValid() &&
+ RetTypeLoc.getBeginLoc() < NullabilityLoc) ||
+ IsInsideOfAssume);
+
+ const auto NewToken =
+ getNullabilitySpelling(NullabilityKind::Nullable, UseSyntaxSugar);
+ const auto ExistingToken =
+ getNullabilitySpelling(NullabilityKind::NonNull, UseSyntaxSugar);
+
+ FixItHint Hint;
+ if (NullabilityLoc.isValid() && !IsInsideOfAssume) {
+ const size_t ExistingTokenSize = ExistingToken.size();
+ Hint = FixItHint::CreateReplacement(
+ SourceRange(NullabilityLoc,
+ NullabilityLoc.getLocWithOffset(ExistingTokenSize - 1)),
+ NewToken);
+ } else {
+
+ Hint = FixItHint::CreateInsertion(
+ UseSyntaxSugar
+ ? RetTypeLoc.getBeginLoc()
+ : RetTypeLoc.getSourceRange().getBegin().getLocWithOffset(
+ CanonicalTypeSize),
+ llvm::Twine(NewToken + " ").str());
+ }
+ return Hint;
+}
+
/// This method check when nullable pointer or null value is returned from a
/// function that has nonnull return type.
void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
@@ -586,6 +637,7 @@
bool InSuppressedMethodFamily = false;
QualType RequiredRetType;
+ TypeLoc RetTypeLoc;
AnalysisDeclContext *DeclCtxt =
C.getLocationContext()->getAnalysisDeclContext();
const Decl *D = DeclCtxt->getDecl();
@@ -599,8 +651,12 @@
InSuppressedMethodFamily = true;
RequiredRetType = MD->getReturnType();
+ if (const auto TSI = MD->getReturnTypeSourceInfo()) {
+ RetTypeLoc = TSI->getTypeLoc();
+ }
} else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
RequiredRetType = FD->getReturnType();
+ RetTypeLoc = FD->getFunctionTypeLoc().getReturnLoc();
} else {
return;
}
@@ -619,6 +675,11 @@
bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
Nullness == NullConstraint::IsNull);
+
+ const bool ContextSupportsSyntaxSugar = isa<ObjCMethodDecl>(D);
+ llvm::Optional<FixItHint> Hint =
+ RetTypeLoc ? getFixItHint(RetTypeLoc, ContextSupportsSyntaxSugar) : None;
+
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
RetExprTypeLevelNullability != Nullability::Nonnull &&
!InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) {
@@ -634,7 +695,7 @@
" that is expected to return a non-null value";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
CK_NullReturnedFromNonnull, N, nullptr, C,
- RetExpr);
+ RetExpr, false, Hint);
return;
}
@@ -667,7 +728,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, Hint);
}
return;
}
@@ -1247,6 +1309,9 @@
checker->NoDiagnoseCallsToSystemHeaders || \
mgr.getAnalyzerOptions().getCheckerBooleanOption( \
checker, "NoDiagnoseCallsToSystemHeaders", true); \
+ checker->ShowFixIts[NullabilityChecker::CheckKind::CK_##name] = \
+ mgr.getAnalyzerOptions().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<HasDocumentation>;
-
+
def CStringUninitializedRead : Checker<"UninitializedRead">,
HelpText<"Checks if the string manipulation function would read uninitialized bytes">,
Dependencies<[CStringModeling]>,
Documentation<HasDocumentation>;
-
+
} // 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
