Infer stronger intent from non-literal nullable default member initializers. NULLABLE_DEFAULT_MEMBER_INITIALIZER evidence is considered in a soft no-conflict rule, subordinate to other evidence of Nonnull-ness, as literal nullptrs are often used simply to turn uninitialized-memory bugs into louder nullptr dereference crashes in cases where the pointer is still expected to be never null after construction of the containing object. ASSIGNED_TO_NULLABLE is more aggressive evidence, which will cause a conflict if we also see evidence requiring the pointer to be marked Nonnull, and we will now use this for cases where the default member initializer is a more complex nullable form than literal nullptr, which likely indicates more explicit Nullable intent. PiperOrigin-RevId: 642991495 Change-Id: I8f0d7cc232748cbb82c13ee5f500ba1e26fd0774
diff --git a/bazel/llvm.bzl b/bazel/llvm.bzl index f74a130..72a5668 100644 --- a/bazel/llvm.bzl +++ b/bazel/llvm.bzl
@@ -53,7 +53,7 @@ executable = False, ) -LLVM_COMMIT_SHA = "c012e487b7246239c31bd378ab074fb110631186" +LLVM_COMMIT_SHA = "98174fb6ec9784d9eb7be313ee1317ca6e703d90" def llvm_loader_repository_dependencies(): # This *declares* the dependency, but it won't actually be *downloaded* unless it's used.
diff --git a/nullability/inference/collect_evidence.cc b/nullability/inference/collect_evidence.cc index 9710d96..61e9a28 100644 --- a/nullability/inference/collect_evidence.cc +++ b/nullability/inference/collect_evidence.cc
@@ -291,6 +291,26 @@ getReturnTypeNullabilityAnnotations(D, Defaults)); } +static bool isOrIsConstructedFromNullPointerConstant( + absl::Nonnull<const Expr *> E, ASTContext &Ctx) { + if (E->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNotNull) != + Expr::NPCK_NotNull) { + return true; + } + const Expr *SubExpr = &dataflow::ignoreCFGOmittedNodes(*E); + if (auto *MaterializeTempExpr = dyn_cast<MaterializeTemporaryExpr>(SubExpr)) { + SubExpr = MaterializeTempExpr->getSubExpr(); + } + if (auto *BindTemp = dyn_cast<CXXBindTemporaryExpr>(SubExpr)) { + SubExpr = BindTemp->getSubExpr(); + } + auto *CE = dyn_cast<CXXConstructExpr>(SubExpr->IgnoreImpCasts()); + if (!CE) return false; + return CE != nullptr && CE->getNumArgs() == 1 && + CE->getArg(0)->isNullPointerConstant( + Ctx, Expr::NPC_ValueDependentIsNotNull) != Expr::NPCK_NotNull; +} + namespace { class DefinitionEvidenceCollector { public: @@ -1018,9 +1038,13 @@ return; } + bool NullptrDefaultInit = + IsDefaultInitializer && isOrIsConstructedFromNullPointerConstant( + InitExpr, Field->getASTContext()); + fromAssignmentLike(*Field, *InitExpr, InitExpr->getExprLoc(), - IsDefaultInitializer - ? Evidence::NULLABLE_DEFAULT_MEMBER_INITIALIZER + NullptrDefaultInit + ? Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER : Evidence::ASSIGNED_FROM_NULLABLE); } @@ -1309,25 +1333,6 @@ return llvm::Error::success(); } -static bool isOrIsConstructedFromNullPointerConstant( - absl::Nonnull<const Expr *> E, ASTContext &Ctx) { - if (E->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNotNull) != - Expr::NPCK_NotNull) { - return true; - } - const Expr *SubExpr = &dataflow::ignoreCFGOmittedNodes(*E); - if (auto *MaterializeTempExpr = dyn_cast<MaterializeTemporaryExpr>(SubExpr)) { - SubExpr = MaterializeTempExpr->getSubExpr(); - } - auto *BindTemp = dyn_cast<CXXBindTemporaryExpr>(SubExpr); - if (BindTemp == nullptr) return false; - auto *CE = - dyn_cast<CXXConstructExpr>(BindTemp->getSubExpr()->IgnoreImpCasts()); - return CE != nullptr && CE->getNumArgs() == 1 && - CE->getArg(0)->isNullPointerConstant( - Ctx, Expr::NPC_ValueDependentIsNotNull) != Expr::NPCK_NotNull; -} - static void collectEvidenceFromDefaultArgument( const clang::FunctionDecl &Fn, const clang::ParmVarDecl &ParamDecl, Slot ParamSlot, llvm::function_ref<EvidenceEmitter> Emit) {
diff --git a/nullability/inference/collect_evidence_test.cc b/nullability/inference/collect_evidence_test.cc index 991ce90..33083ad 100644 --- a/nullability/inference/collect_evidence_test.cc +++ b/nullability/inference/collect_evidence_test.cc
@@ -1211,6 +1211,35 @@ functionNamed("Target")))); } +TEST(CollectEvidenceFromDefinitionTest, DefaultFieldInitializerNullptr) { + static constexpr llvm::StringRef Src = R"cc( + struct Target { + Target() {}; + int* I = nullptr; + }; + )cc"; + EXPECT_THAT(collectFromDefinitionMatching( + cxxConstructorDecl(isDefaultConstructor()), Src), + UnorderedElementsAre(evidence( + Slot(0), Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER, + fieldNamed("Target::I")))); +} + +TEST(CollectEvidenceFromDefinitionTest, DefaultFieldInitializerNullable) { + static constexpr llvm::StringRef Src = R"cc( + Nullable<int*> G; + struct Target { + Target() {}; + int* I = G; + }; + )cc"; + EXPECT_THAT( + collectFromDefinitionMatching(cxxConstructorDecl(isDefaultConstructor()), + Src), + UnorderedElementsAre(evidence(Slot(0), Evidence::ASSIGNED_FROM_NULLABLE, + fieldNamed("Target::I")))); +} + TEST(CollectEvidenceFromDefinitionTest, IndirectFieldInitializerFromAssignmentToType) { static constexpr llvm::StringRef Src = R"cc( @@ -1244,7 +1273,7 @@ collectFromDefinitionMatching( cxxConstructorDecl(isDefaultConstructor(), hasName("Target")), Src), UnorderedElementsAre( - evidence(Slot(0), Evidence::NULLABLE_DEFAULT_MEMBER_INITIALIZER, + evidence(Slot(0), Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER, fieldNamed("Target@Sa::I")))); } @@ -1367,7 +1396,7 @@ collectFromDefinitionMatching( cxxConstructorDecl(isDefaultConstructor(), hasName("Target")), Src), UnorderedElementsAre( - evidence(Slot(0), Evidence::NULLABLE_DEFAULT_MEMBER_INITIALIZER, + evidence(Slot(0), Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER, fieldNamed("Target::I")))); }
diff --git a/nullability/inference/inference.proto b/nullability/inference/inference.proto index fdb83f1..eb280cf 100644 --- a/nullability/inference/inference.proto +++ b/nullability/inference/inference.proto
@@ -83,12 +83,16 @@ // A pointer was used with an arithmetic operator without being checked for // null first. ARITHMETIC = 14; - // A non-static member variable has a default initializer that is nullable. - // This is considered to be a weaker signal than other assignments to - // nullable, due to the common use of nullptr as a default value to avoid - // quieter uninitialized memory errors in favor of loud segfaults, so we - // differentiate the evidence. - NULLABLE_DEFAULT_MEMBER_INITIALIZER = 15; + // A non-static member variable has a default initializer that is a literal + // nullptr or is simply constructed from a literal nullptr. This is + // considered to be a weaker signal than other assignments to nullable, due + // to the common use of nullptr as a default value to avoid quieter + // uninitialized memory errors in favor of loud segfaults, so we + // differentiate the evidence. Default initializers that are nullable but + // not using literal nullptrs use the stronger evidence + // ASSIGNED_TO_NULLABLE, as they likely indicate more explicit Nullable + // intent. + NULLPTR_DEFAULT_MEMBER_INITIALIZER = 15; // __attribute((nonnull[(optional_param_indices)])) was applied to a // function or parameter declaration or __attribute((returns_nonnull)) was // applied to a function declaration.
diff --git a/nullability/inference/merge.cc b/nullability/inference/merge.cc index 890ed7e..486326e 100644 --- a/nullability/inference/merge.cc +++ b/nullability/inference/merge.cc
@@ -144,7 +144,7 @@ if (!Counts[Evidence::NULLABLE_ARGUMENT] && !Counts[Evidence::UNKNOWN_ARGUMENT] && Counts[Evidence::NONNULL_ARGUMENT]) return {Nullability::NONNULL}; - if (Counts[Evidence::NULLABLE_DEFAULT_MEMBER_INITIALIZER]) + if (Counts[Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER]) return {Nullability::NULLABLE}; return {Nullability::UNKNOWN};
diff --git a/nullability/inference/merge_test.cc b/nullability/inference/merge_test.cc index 2f842f0..412d78e 100644 --- a/nullability/inference/merge_test.cc +++ b/nullability/inference/merge_test.cc
@@ -276,7 +276,7 @@ } TEST_F(InferTest, NullableDefaultMemberInitializer) { - add(Evidence::NULLABLE_DEFAULT_MEMBER_INITIALIZER); + add(Evidence::NULLPTR_DEFAULT_MEMBER_INITIALIZER); EXPECT_EQ(Nullability::NULLABLE, infer()); add(Evidence::UNCHECKED_DEREFERENCE); EXPECT_EQ(Nullability::NONNULL, infer());