diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/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; + let ParentPackage = Nullability in { def NullabilityBase : Checker<"NullabilityBase">, HelpText<"Stores information during the analysis about nullability.">, + CheckerOptions<[NullabilityFixIts]>, Documentation, 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; 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; def NullableDereferencedChecker : Checker<"NullableDereferenced">, HelpText<"Warns when a nullable pointer is dereferenced.">, + CheckerOptions<[NullabilityFixIts]>, Dependencies<[NullabilityBase]>, Documentation; 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; 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; @@ -475,12 +494,12 @@ HelpText<"Check for arguments which are not null-terminating strings">, Dependencies<[CStringModeling]>, Documentation; - + def CStringUninitializedRead : Checker<"UninitializedRead">, HelpText<"Checks if the string manipulation function would read uninitialized bytes">, Dependencies<[CStringModeling]>, Documentation; - + } // end "alpha.unix.cstring" let ParentPackage = Unix in { diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/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 &BT = getBugType(CK); auto R = std::make_unique(*BT, Msg, N); if (Region) { @@ -180,6 +194,14 @@ if (const auto *Ex = dyn_cast(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(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(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(); \ 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) { \ diff --git a/clang/test/Analysis/nullability-fixits.mm b/clang/test/Analysis/nullability-fixits.mm new file mode 100644 --- /dev/null +++ b/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 +@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