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 @@ -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 BTs[CK_NumCheckKinds]; const std::unique_ptr &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 &BT = getBugType(CK); auto R = std::make_unique(*BT, Msg, N); if (Region) { @@ -180,6 +198,10 @@ if (const auto *Ex = dyn_cast(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(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(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(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(); \ 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) { \ 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,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