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; @@ -480,7 +499,7 @@ 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,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. /// @@ -161,11 +172,13 @@ ExplodedNode *N, const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr = nullptr, - bool SuppressPath = false) const; + bool SuppressPath = false, + const llvm::Optional 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 Hint = None) const { const std::unique_ptr &BT = getBugType(CK); auto R = std::make_unique(*BT, Msg, N); if (Region) { @@ -180,6 +193,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 +454,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 Hint) const { ProgramStateRef OriginalState = N->getState(); if (checkInvariantViolation(OriginalState, N, C)) @@ -447,7 +464,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 +581,50 @@ return E->IgnoreImpCasts(); } +llvm::Optional getFixItHint(TypeLoc RetTypeLoc, + bool CallsiteSupportsSyntaxSugar) { + const SourceLocation NullabilityLoc = RetTypeLoc.findNullabilityLoc(); + const auto CanonicalTypeStr = + RetTypeLoc.getType().getCanonicalType().getAsString(); + if (CanonicalTypeStr == "id" || CanonicalTypeStr == "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 +647,7 @@ bool InSuppressedMethodFamily = false; QualType RequiredRetType; + TypeLoc RetTypeLoc; AnalysisDeclContext *DeclCtxt = C.getLocationContext()->getAnalysisDeclContext(); const Decl *D = DeclCtxt->getDecl(); @@ -599,8 +661,10 @@ InSuppressedMethodFamily = true; RequiredRetType = MD->getReturnType(); + RetTypeLoc = MD->getReturnTypeSourceInfo()->getTypeLoc(); } else if (auto *FD = dyn_cast(D)) { RequiredRetType = FD->getReturnType(); + RetTypeLoc = FD->getFunctionTypeLoc().getReturnLoc(); } else { return; } @@ -619,6 +683,12 @@ bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull && Nullness == NullConstraint::IsNull); + + const bool ContextSupportsSyntaxSugar = + dyn_cast(D) != nullptr; + llvm::Optional Hint = + getFixItHint(RetTypeLoc, ContextSupportsSyntaxSugar); + if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull && RetExprTypeLevelNullability != Nullability::Nonnull && !InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) { @@ -634,7 +704,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 +737,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 +1318,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) { \ 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,184 @@ +// 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 + +// RUN: cp %s %t +// 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: -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 *_Nullable)returnsNilProtocol +- (TestObject *_Nonnull)returnsNilProtocol { + return nil; +} + +// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar +- (nonnull TestObject *)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 *)returnsNilProtocolAssume +- (TestObject *)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 *_Nullable returnsNilFuncProtocolAssume() { +TestObject *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; +} + +NS_ASSUME_NONNULL_END