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 @@ -311,39 +311,57 @@ // 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; 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,7 @@ bool ChecksEnabled[CK_NumCheckKinds] = {false}; CheckerNameRef CheckNames[CK_NumCheckKinds]; + DefaultBool ShowFixIts[CK_NumCheckKinds]; mutable std::unique_ptr BTs[CK_NumCheckKinds]; const std::unique_ptr &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 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 +183,9 @@ 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 +443,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 +453,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 +570,51 @@ return E->IgnoreImpCasts(); } +llvm::Optional 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(D)) { RequiredRetType = FD->getReturnType(); + RetTypeLoc = FD->getFunctionTypeLoc().getReturnLoc(); } else { return; } @@ -619,6 +675,9 @@ bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull && Nullness == NullConstraint::IsNull); + const bool ContextSupportsSyntaxSugar = isa(D); + llvm::Optional Hint = + RetTypeLoc ? getFixItHint(RetTypeLoc, ContextSupportsSyntaxSugar) : None; if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull && RetExprTypeLevelNullability != Nullability::Nonnull && !InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) { @@ -634,7 +693,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 +726,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; } @@ -1235,7 +1295,6 @@ bool ento::shouldRegisterNullabilityBase(const CheckerManager &mgr) { return true; } - #define REGISTER_CHECKER(name, trackingRequired) \ void ento::register##name##Checker(CheckerManager &mgr) { \ NullabilityChecker *checker = mgr.getChecker(); \ @@ -1247,6 +1306,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/analyzer-config.c b/clang/test/Analysis/analyzer-config.c --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -100,6 +100,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 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,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_analyze_cc1 \ +// 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_analyze_cc1 \ +// 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_analyze_cc1 \ +// 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_analyze_cc1 \ +// 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 *_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; +} + +// We don't need to check instancetype because this is outside a class + +NS_ASSUME_NONNULL_END