[clang][nullability] allow _Nonnull etc on nullable class types #82705

sam-mccall · 2024-02-22T23:33:25Z

This enables clang and external nullability checkers to make use of
these annotations on nullable C++ class types like unique_ptr.

These types are recognized by the presence of the _Nullable attribute.
Nullable standard library types implicitly receive this attribute.

Existing static warnings for raw pointers are extended to smart pointers:

nullptr used as return value or argument for non-null functions
(-Wnonnull)
assigning or initializing nonnull variables with nullable values
(-Wnullable-to-nonnull-conversion)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

Support can be detected as __has_feature(nullability_on_classes).
This is needed for back-compatibility, as previously clang would issue a
hard error when _Nullable appears on a smart pointer.

UBSan's -fsanitize=nullability will not check smart-pointer types.
It can be made to do so by synthesizing calls to operator bool, but
that's left for future work.

Discussion: https://discourse.llvm.org/t/rfc-allowing-nonnull-etc-on-smart-pointers/77201/26

github-actions · 2024-02-22T23:35:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-03-02T04:09:54Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Sam McCall (sam-mccall)

Changes

This enables external nullability checkers to make use of these
annotations on smart pointer types.

Existing static warnings for raw pointers are extended to smart pointers:

nullptr used as return value or argument for non-null functions
(-Wnonnull)
assigning or initializing nonnull variables with nullable values
(-Wnullable-to-nonnull-conversion)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

UBSan's -fsanitize=nullability will not check smart-pointer types.
It can be made to do so by synthesizing calls to operator bool, but
that's left for future work.

Patch is 25.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82705.diff

18 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+2-1)
(modified) clang/include/clang/Basic/AttrDocs.td (+16)
(modified) clang/include/clang/Basic/Features.def (+1)
(modified) clang/include/clang/Parse/Parser.h (+1)
(modified) clang/include/clang/Sema/Sema.h (+3)
(modified) clang/lib/AST/Type.cpp (+19-10)
(modified) clang/lib/CodeGen/CGCall.cpp (+2-1)
(modified) clang/lib/CodeGen/CodeGenFunction.cpp (+2-1)
(modified) clang/lib/Parse/ParseDeclCXX.cpp (+24-9)
(modified) clang/lib/Sema/SemaAttr.cpp (+12)
(modified) clang/lib/Sema/SemaChecking.cpp (+9)
(modified) clang/lib/Sema/SemaDecl.cpp (+3-1)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+18)
(modified) clang/lib/Sema/SemaInit.cpp (+5)
(modified) clang/lib/Sema/SemaOverload.cpp (+7)
(modified) clang/lib/Sema/SemaTemplate.cpp (+1)
(modified) clang/lib/Sema/SemaType.cpp (+14-4)
(modified) clang/test/SemaCXX/nullability.cpp (+60-2)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index fa191c7378dba4..f75aa126cbb939 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2156,9 +2156,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index b96fbddd51154c..8c248fb052f19e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4096,6 +4096,11 @@ non-underscored keywords. For example:
       @property (assign, nullable) NSView *superview;
       @property (readonly, nonnull) NSArray *subviews;
     @end
+
+As well as built-in pointer types, ithe nullability attributes can be attached
+to nullable types from the C++ standard library such as ``std::unique_ptr`` and
+``std::function``, as well as C++ classes marked with the ``_Nullable``
+attribute.
   }];
 }
 
@@ -4130,6 +4135,17 @@ The ``_Nullable`` nullability qualifier indicates that a value of the
     int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+    class _Nullable ArenaPointer { ... };
+
+    ArenaPointer _Nonnull x = ...;
+    ArenaPointer _Nullable y = nullptr;
   }];
 }
 
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index 5fad5fc3623cb6..7c0755b7318306 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -94,6 +94,7 @@ EXTENSION(define_target_os_macros,
 FEATURE(enumerator_attributes, true)
 FEATURE(nullability, true)
 FEATURE(nullability_on_arrays, true)
+FEATURE(nullability_on_classes, true)
 FEATURE(nullability_nullable_result, true)
 FEATURE(memory_sanitizer,
         LangOpts.Sanitize.hasOneOf(SanitizerKind::Memory |
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 071520f535bc95..3d85868b69d580 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3007,6 +3007,7 @@ class Parser : public CodeCompletionHandler {
   void DiagnoseAndSkipExtendedMicrosoftTypeAttributes();
   SourceLocation SkipExtendedMicrosoftTypeAttributes();
   void ParseMicrosoftInheritanceClassAttributes(ParsedAttributes &attrs);
+  void ParseNullabilityClassAttributes(ParsedAttributes &attrs);
   void ParseBorlandTypeAttributes(ParsedAttributes &attrs);
   void ParseOpenCLKernelAttributes(ParsedAttributes &attrs);
   void ParseOpenCLQualifiers(ParsedAttributes &Attrs);
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index fcccac10f4733a..9511cf58f16327 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8004,6 +8004,9 @@ class Sema final {
   /// Add [[gsl::Pointer]] attributes for std:: types.
   void inferGslPointerAttribute(TypedefNameDecl *TD);
 
+  /// Add _Nullable attributes for std:: types.
+  void inferNullableClassAttribute(CXXRecordDecl *CRD);
+
   void CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record);
 
   /// Check that the C++ class annoated with "trivial_abi" satisfies all the
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 78dcd3f4007a5a..bb0a5e7bc59e9e 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4556,16 +4556,15 @@ bool Type::canHaveNullability(bool ResultIfUnknown) const {
   case Type::Auto:
     return ResultIfUnknown;
 
-  // Dependent template specializations can instantiate to pointer
-  // types unless they're known to be specializations of a class
-  // template.
+  // Dependent template specializations could instantiate to pointer types.
   case Type::TemplateSpecialization:
-    if (TemplateDecl *templateDecl
-          = cast<TemplateSpecializationType>(type.getTypePtr())
-              ->getTemplateName().getAsTemplateDecl()) {
-      if (isa<ClassTemplateDecl>(templateDecl))
-        return false;
-    }
+    // If it's a known class template, we can already check if it's nullable.
+    if (TemplateDecl *templateDecl =
+            cast<TemplateSpecializationType>(type.getTypePtr())
+                ->getTemplateName()
+                .getAsTemplateDecl())
+      if (auto *CTD = dyn_cast<ClassTemplateDecl>(templateDecl))
+        return CTD->getTemplatedDecl()->hasAttr<TypeNullableAttr>();
     return ResultIfUnknown;
 
   case Type::Builtin:
@@ -4622,6 +4621,17 @@ bool Type::canHaveNullability(bool ResultIfUnknown) const {
     }
     llvm_unreachable("unknown builtin type");
 
+  case Type::Record: {
+    const RecordDecl *RD = cast<RecordType>(type)->getDecl();
+    // For template specializations, look only at primary template attributes.
+    // This is a consistent regardless of whether the instantiation is known.
+    if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD))
+      return CTSD->getSpecializedTemplate()
+          ->getTemplatedDecl()
+          ->hasAttr<TypeNullableAttr>();
+    return RD->hasAttr<TypeNullableAttr>();
+  }
+
   // Non-pointer types.
   case Type::Complex:
   case Type::LValueReference:
@@ -4639,7 +4649,6 @@ bool Type::canHaveNullability(bool ResultIfUnknown) const {
   case Type::DependentAddressSpace:
   case Type::FunctionProto:
   case Type::FunctionNoProto:
-  case Type::Record:
   case Type::DeducedTemplateSpecialization:
   case Type::Enum:
   case Type::InjectedClassName:
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index d05cf1c6e1814e..83cfc8831b036c 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4373,7 +4373,8 @@ void CodeGenFunction::EmitNonNullArgCheck(RValue RV, QualType ArgType,
     NNAttr = getNonNullAttr(AC.getDecl(), PVD, ArgType, ArgNo);
 
   bool CanCheckNullability = false;
-  if (SanOpts.has(SanitizerKind::NullabilityArg) && !NNAttr && PVD) {
+  if (SanOpts.has(SanitizerKind::NullabilityArg) && !NNAttr && PVD &&
+      !PVD->getType()->isRecordType()) {
     auto Nullability = PVD->getType()->getNullability();
     CanCheckNullability = Nullability &&
                           *Nullability == NullabilityKind::NonNull &&
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 1ad905078d349c..c492929f061798 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -974,7 +974,8 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
   // return value. Initialize the flag to 'true' and refine it in EmitParmDecl.
   if (SanOpts.has(SanitizerKind::NullabilityReturn)) {
     auto Nullability = FnRetTy->getNullability();
-    if (Nullability && *Nullability == NullabilityKind::NonNull) {
+    if (Nullability && *Nullability == NullabilityKind::NonNull &&
+        !FnRetTy->isRecordType()) {
       if (!(SanOpts.has(SanitizerKind::ReturnsNonnullAttribute) &&
             CurCodeDecl && CurCodeDecl->getAttr<ReturnsNonNullAttr>()))
         RetValNullabilityPrecondition =
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 62632b2d79792e..839113f644bc23 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1494,6 +1494,15 @@ void Parser::ParseMicrosoftInheritanceClassAttributes(ParsedAttributes &attrs) {
   }
 }
 
+void Parser::ParseNullabilityClassAttributes(ParsedAttributes &attrs) {
+  while (Tok.is(tok::kw__Nullable)) {
+    IdentifierInfo *AttrName = Tok.getIdentifierInfo();
+    auto Kind = Tok.getKind();
+    SourceLocation AttrNameLoc = ConsumeToken();
+    attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0, Kind);
+  }
+}
+
 /// Determine whether the following tokens are valid after a type-specifier
 /// which could be a standalone declaration. This will conservatively return
 /// true if there's any doubt, and is appropriate for insert-';' fixits.
@@ -1675,15 +1684,21 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
 
   ParsedAttributes attrs(AttrFactory);
   // If attributes exist after tag, parse them.
-  MaybeParseAttributes(PAKM_CXX11 | PAKM_Declspec | PAKM_GNU, attrs);
-
-  // Parse inheritance specifiers.
-  if (Tok.isOneOf(tok::kw___single_inheritance, tok::kw___multiple_inheritance,
-                  tok::kw___virtual_inheritance))
-    ParseMicrosoftInheritanceClassAttributes(attrs);
-
-  // Allow attributes to precede or succeed the inheritance specifiers.
-  MaybeParseAttributes(PAKM_CXX11 | PAKM_Declspec | PAKM_GNU, attrs);
+  for (;;) {
+    MaybeParseAttributes(PAKM_CXX11 | PAKM_Declspec | PAKM_GNU, attrs);
+    // Parse inheritance specifiers.
+    if (Tok.isOneOf(tok::kw___single_inheritance,
+                    tok::kw___multiple_inheritance,
+                    tok::kw___virtual_inheritance)) {
+      ParseMicrosoftInheritanceClassAttributes(attrs);
+      continue;
+    }
+    if (Tok.is(tok::kw__Nullable)) {
+      ParseNullabilityClassAttributes(attrs);
+      continue;
+    }
+    break;
+  }
 
   // Source location used by FIXIT to insert misplaced
   // C++11 attributes
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index 0dcf42e4899713..a5dd158808f26b 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -215,6 +215,18 @@ void Sema::inferGslOwnerPointerAttribute(CXXRecordDecl *Record) {
   inferGslPointerAttribute(Record, Record);
 }
 
+void Sema::inferNullableClassAttribute(CXXRecordDecl *CRD) {
+  static llvm::StringSet<> Nullable{
+      "auto_ptr",         "shared_ptr", "unique_ptr",         "exception_ptr",
+      "coroutine_handle", "function",   "move_only_function",
+  };
+
+  if (CRD->isInStdNamespace() && Nullable.count(CRD->getName()) &&
+      !CRD->hasAttr<TypeNullableAttr>())
+    for (Decl *Redecl : CRD->redecls())
+      Redecl->addAttr(TypeNullableAttr::CreateImplicit(Context));
+}
+
 void Sema::ActOnPragmaOptionsAlign(PragmaOptionsAlignKind Kind,
                                    SourceLocation PragmaLoc) {
   PragmaMsStackAction Action = Sema::PSK_Reset;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index e8bfb215a5b4c5..1778f632c12aad 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -27,6 +27,7 @@
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/FormatString.h"
+#include "clang/AST/IgnoreExpr.h"
 #include "clang/AST/NSAPI.h"
 #include "clang/AST/NonTrivialTypeVisitor.h"
 #include "clang/AST/OperationKinds.h"
@@ -7154,6 +7155,14 @@ bool Sema::getFormatStringInfo(const FormatAttr *Format, bool IsCXXMember,
 ///
 /// Returns true if the value evaluates to null.
 static bool CheckNonNullExpr(Sema &S, const Expr *Expr) {
+  // Treat (smart) pointers constructed from nullptr as null, whether we can
+  // const-evaluate them or not.
+  // This must happen first: the smart pointer expr might have _Nonnull type!
+  if (isa<CXXNullPtrLiteralExpr>(
+          IgnoreExprNodes(Expr, IgnoreImplicitAsWrittenSingleStep,
+                          IgnoreElidableImplicitConstructorSingleStep)))
+    return true;
+
   // If the expression has non-null type, it doesn't evaluate to null.
   if (auto nullability = Expr->IgnoreImplicit()->getType()->getNullability()) {
     if (*nullability == NullabilityKind::NonNull)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 10b5c271f25c10..123e210960f517 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -18171,8 +18171,10 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
   if (PrevDecl)
     mergeDeclAttributes(New, PrevDecl);
 
-  if (auto *CXXRD = dyn_cast<CXXRecordDecl>(New))
+  if (auto *CXXRD = dyn_cast<CXXRecordDecl>(New)) {
     inferGslOwnerPointerAttribute(CXXRD);
+    inferNullableClassAttribute(CXXRD);
+  }
 
   // If there's a #pragma GCC visibility in scope, set the visibility of this
   // record.
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 8a204b1d3b88d9..aa04179b2b43f4 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5955,6 +5955,20 @@ static void handleBuiltinAliasAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context) BuiltinAliasAttr(S.Context, AL, Ident));
 }
 
+static void handleNullableTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (AL.isUsedAsTypeAttr())
+    return;
+
+  if (auto *CRD = dyn_cast<CXXRecordDecl>(D);
+      !D || !(CRD->isClass() || CRD->isStruct())) {
+    S.Diag(AL.getRange().getBegin(), diag::err_attribute_wrong_decl_type_str)
+        << AL << AL.isRegularKeywordAttribute() << "classes";
+    return;
+  }
+
+  handleSimpleAttribute<TypeNullableAttr>(S, D, AL);
+}
+
 static void handlePreferredTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!AL.hasParsedType()) {
     S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
@@ -9862,6 +9876,10 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_UsingIfExists:
     handleSimpleAttribute<UsingIfExistsAttr>(S, D, AL);
     break;
+
+  case ParsedAttr::AT_TypeNullable:
+    handleNullableTypeAttr(S, D, AL);
+    break;
   }
 }
 
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 0fd458837163e5..89ed4e19fb8fb7 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7050,6 +7050,11 @@ PerformConstructorInitialization(Sema &S,
       hasCopyOrMoveCtorParam(S.Context,
                              getConstructorInfo(Step.Function.FoundDecl));
 
+  // A smart pointer constructed from a nullable pointer is nullable.
+  if (NumArgs == 1 && !Kind.isExplicitCast())
+    S.diagnoseNullableToNonnullConversion(
+        Entity.getType(), Args.front()->getType(), Kind.getLocation());
+
   // Determine the arguments required to actually perform the constructor
   // call.
   if (S.CompleteConstructorCall(Constructor, Step.Type, Args, Loc,
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index f7645422348b65..03ac17c5be4b04 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14706,6 +14706,13 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
           }
         }
 
+        // Check for nonnull = nullable.
+        // This won't be caught in the arg's initialization: the parameter to
+        // the assignment operator is not marked nonnull.
+        if (Op == OO_Equal)
+          diagnoseNullableToNonnullConversion(Args[0]->getType(),
+                                              Args[1]->getType(), OpLoc);
+
         // Convert the arguments.
         if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(FnDecl)) {
           // Best->Access is only meaningful for class members.
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 1a975a8d0a0df5..c8d8b78cf57bb4 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2164,6 +2164,7 @@ DeclResult Sema::CheckClassTemplate(
 
   AddPushedVisibilityAttribute(NewClass);
   inferGslOwnerPointerAttribute(NewClass);
+  inferNullableClassAttribute(NewClass);
 
   if (TUK != TUK_Friend) {
     // Per C++ [basic.scope.temp]p2, skip the template parameter scopes.
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 1e43e36016a66f..517152397a0a68 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -4705,6 +4705,18 @@ static bool DiagnoseMultipleAddrSpaceAttributes(Sema &S, LangAS ASOld,
   return false;
 }
 
+// Whether this is a type broadly expected to have nullability attached.
+// These types are affected by `#pragma assume_nonnull`, and missing nullability
+// will be diagnosed with -Wnullability-completeness.
+static bool shouldHaveNullability(QualType T) {
+  return T->canHaveNullability(/*ResultIfUnknown=*/false) &&
+         // For now, do not infer/require nullability on C++ smart pointers.
+         // It's unclear whether the pragma's behavior is useful for C++.
+         // e.g. treating type-aliases and template-type-parameters differently
+         // from types of declarations can be surprising.
+         !isa<RecordType>(T);
+}
+
 static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
                                                 QualType declSpecType,
                                                 TypeSourceInfo *TInfo) {
@@ -4823,8 +4835,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
     // inner pointers.
     complainAboutMissingNullability = CAMN_InnerPointers;
 
-    if (T->canHaveNullability(/*ResultIfUnknown*/ false) &&
-        !T->getNullability()) {
+    if (shouldHaveNullability(T) && !T->getNullability()) {
       // Note that we allow but don't require nullability on dependent types.
       ++NumPointersRemaining;
     }
@@ -5047,8 +5058,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
   // If the type itself could have nullability but does not, infer pointer
   // nullability and perform consistency checking.
   if (S.CodeSynthesisContexts.empty()) {
-    if (T->canHaveNullability(/*ResultIfUnknown*/ false) &&
-        !T->getNullability()) {
+    if (shouldHaveNullability(T) && !T->getNullability()) {
       if (isVaList(T)) {
         // Record that we've seen a pointer, but do nothing else.
         if (NumPointersRemaining > 0)
diff --git a/clang/test/SemaCXX/nullability.cpp b/clang/test/SemaCXX/nullability.cpp
index 8d0c4dc195a6bd..5d2df9d1d2e8eb 100644
--- a/clang/test/SemaCXX/nullability.cpp
+++ b/clang/test/SemaCXX/nullability.cpp
@@ -4,6 +4,10 @@
 #else
 #  error nullability feature should be defined
 #endif
+#if __has_feature(nullability_on_classes)
+#else
+#  error smart-pointer feature should be defined
+#endif
 
 #include "nullability-completeness.h"
 
@@ -27,6 +31,7 @@ template<typename T>
 struct AddNonNull {
   typedef _Nonnull T type; // expected-error{{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'int'}}
   // expected-error@-1{{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'std::nullptr_t'}}
+  // expected-error@-2{{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'NotPtr'}}
 };
 
 typedef AddNonNull<int *>::type nonnull_int_ptr_1;
@@ -35,6 +40,33 @@ typedef AddNonNull<nullptr_t>::type nonnull_int_ptr_3; // expected-note{{in inst
 
 typedef AddNonNull<int>::type nonnull_non_pointer_1; // expected-note{{in instantiation of template class 'AddNonNull<int>' requested here}}
 
+// Nullability on C++ class types (smart pointers).
+struct NotPtr{};
+typedef AddNonNull<NotPtr>::type nonnull_non_pointer_2; // expected-note{{in instantiation}}
+struct _Nullable SmartPtr{
+  SmartPtr();
+  SmartPtr(nullptr_t);
+  SmartPtr(const SmartPtr&);
+  SmartPtr(SmartPtr&&);
+  SmartPtr &operator=(const SmartPtr&);
+  SmartPtr &operator=(SmartPtr&&);
+};
+typedef AddNonNull<SmartPtr>::type nonnull_smart_pointer_1;
+template<class> struct _Nullable SmartPtrTemplate{};
+typedef AddNonNull<SmartPtrTemplate<int>>::type nonnull_smart_pointer_2;
+namespace std { inline namespace __1 {
+  template <class> class unique_ptr {};
+  template <class> class function;
+  template <class Ret, class... Args> class function<Ret(Args...)> {};
+} }
+typedef AddNonNull<std::unique_ptr<int>>::type nonnull_smart_pointer_3;
+typedef AddNonNull<std::function<int()>>::type nonnull_smart_pointer_4;
+
+class Derived : public SmartPtr {};
+Derived _Nullable x; // expected-error {{'_Nullable' cannot be applied}}
+class DerivedPrivate : private SmartPtr {};
+DerivedPrivate _Nullable y; // expected-error {{'_Nullable' cannot be applied}}
+
 // Non-null checking within a template.
 template<typename T>
 struct AddNonNull2 {
@@ -54,6 +86,7 @@ void (*& accepts_nonnull_2)(_Nonnull int *ptr) = accepts_nonnul...
[truncated]

AaronBallman

Thank you for the changes! Please be sure to include a release note in clang/docs/ReleaseNotes.rst so users know about the new functionality.

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/AttrDocs.td

clang/lib/Parse/ParseDeclCXX.cpp

clang/lib/Sema/SemaAttr.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/SemaCXX/nullability.cpp

AaronBallman

LGTM though there may be a minor change to the docs that could make sense (feel free to change when landing if you agree).

AaronBallman · 2024-03-13T15:36:29Z

clang/include/clang/Basic/AttrDocs.td

+``move_only_function`` and ``coroutine_handle``.
+
+Types should be marked nullable only where the type itself leaves nullability
+ambiguous. For example, ``std::optional`` is not marked ``_Nullable``, because


Should we mention std::weak_ptr here like we do for std::optional?

This enables clang and external nullability checkers to make use of these annotations on nullable C++ class types like unique_ptr. These types are recognized by the presence of the _Nullable attribute. Nullable standard library types implicitly receive this attribute. Existing static warnings for raw pointers are extended to smart pointers: - nullptr used as return value or argument for non-null functions (`-Wnonnull`) - assigning or initializing nonnull variables with nullable values (`-Wnullable-to-nonnull-conversion`) It doesn't implicitly add these attributes based on the assume_nonnull pragma, nor warn on missing attributes where the pragma would apply them. I'm not confident that the pragma's current behavior will work well for C++ (where type-based metaprogramming is much more common than C/ObjC). We'd like to revisit this once we have more implementation experience. Support can be detected as `__has_feature(nullability_on_classes)`. This is needed for back-compatibility, as previously clang would issue a hard error when _Nullable appears on a smart pointer. UBSan's `-fsanitize=nullability` will not check smart-pointer types. It can be made to do so by synthesizing calls to `operator bool`, but that's left for future work.

tahonermann · 2024-03-14T21:22:17Z

clang/docs/ReleaseNotes.rst

+  This works for standard library types including ``unique_ptr``, ``shared_ptr``
+  and ``function``. See `the attribute reference
+documentation <https://llvm.org/docs/AttributeReference.html#nullability-attributes>`_
+for the full list.


Doc builds seem to be unhappy with this change:

Warning, treated as error: /home/runner/work/llvm-project/llvm-project/clang-build/tools/clang/docs/ReleaseNotes.rst:208:Inline interpreted text or phrase reference start-string without end-string.

The doc build run for this PR failed for a different reason that will be fixed by #85310.

I'll just fix this with #85310.

…es (#82705)" This reverts commit 92a09c0. This is triggering a bunch of new -Wnullability-completeness warnings in code with existing raw pointer nullability annotations. The intent was the new nullability locations wouldn't affect those warnings, so this is a bug at least for now.

…pes (#82705)" This reverts commit ca4c4a6. This was intended not to introduce new consistency diagnostics for smart pointer types, but failed to ignore sugar around types when detecting this. Fixed and test added.

dyung · 2024-03-29T07:44:54Z

The reapplication of this change in bbbcc1d is causing many build bots to fail due to a missing header file as outlined in #87023. The failing bots include the following:

@sam-mccall I notice that the test that is failing, clang/test/SemaObjCXX/nullability-consistency.mm was not included in the set of changes that are part of this pull request. Was this additional change intentional?

…pes (llvm#82705)" This reverts commit 28760b6. The last commit was missing the new testcase, now fixed.

…pes (#82705)" (#87325) This reverts commit 28760b6. The last commit was missing the new testcase, now fixed.

…`_Nullable`. llvm/llvm-project#82705 added support for annotating a class with the `_Nullable` attribute, and this has the advantage over the `absl_nullability_compatible` tag that it can be applied to forward declarations, not just definitions. PiperOrigin-RevId: 631424947 Change-Id: Ic3adcae0e4cd2fca850911b4b6d980983c93a526

jyknight · 2024-05-09T16:24:41Z

I note that this commit (probably unintentionally) fixed an issue with the -Wnonnull warning for return values in Objective-C++ code.

Previously, id _Nonnull foo() { return nullptr; } does not warn under clang -fobjc-arc -target x86_64-apple-macos11 -x objective-c++ -fsyntax-only. It did warn with -fno-objc-arc, however. Now, it does warn in both cases. This seems like an improvement -- but the state of things is still pretty weird.

But, that only started working with return nullptr, not with return 0, though I'd expect those to be treated the same way. It also didn't start working for -x objective-c mode with a return (void*)0;.

(https://godbolt.org/z/1voxqhKbE)

I'd also have expected that this change in behavior would have required test changes to the objc nullability tests, but it didn't, because mostly tests don't enable -fobjc-arc. The test case returnsNilObjCInstanceDirectly in clang/test/Analysis/nullability_nullonly.mm does seem like it'd notice the change in behavior, but it has defined nil as 0, not nullptr. (Also, that's primarily a test for Clang Static Analyzer's nullability checker, not Sema's nullability warning. The same function in clang/test/Analysis/nullability-no-arc.mm does see the Sema -Wnonnull warning too, however.)

Anyways, I don't think it's bad that this commit has had this particular effect, but it's weird, and I wonder if this weirdness might affect other situations than ObjC ARC types, too.

sam-mccall force-pushed the smartptr branch 2 times, most recently from 090c0b0 to 77baccc Compare February 22, 2024 23:55

sam-mccall force-pushed the smartptr branch 3 times, most recently from 5df9923 to eccc468 Compare March 2, 2024 04:07

sam-mccall requested review from gribozavr and AaronBallman March 2, 2024 04:08

sam-mccall marked this pull request as ready for review March 2, 2024 04:09

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Mar 2, 2024

sam-mccall changed the title ~~[clang][nullability] allow _Nonnull etc on gsl::Pointer types~~ [clang][nullability] allow _Nonnull etc on nullable class types Mar 2, 2024

AaronBallman reviewed Mar 5, 2024

View reviewed changes

AaronBallman requested a review from erichkeane March 5, 2024 16:44

gribozavr reviewed Mar 7, 2024

View reviewed changes

clang/test/SemaCXX/nullability.cpp Outdated Show resolved Hide resolved

sam-mccall requested a review from Endilll as a code owner March 11, 2024 13:58

AaronBallman approved these changes Mar 13, 2024

View reviewed changes

sam-mccall added 5 commits March 13, 2024 17:58

Address review comments

172bb12

Extend docs

0579c69

Mention weak_ptr in docs

62cbf6c

fix bad merge

d284e35

sam-mccall force-pushed the smartptr branch from bab1cc1 to d284e35 Compare March 14, 2024 15:45

sam-mccall merged commit 92a09c0 into llvm:main Mar 14, 2024

tahonermann reviewed Mar 14, 2024

View reviewed changes

Sirraide mentioned this pull request Mar 28, 2024

[Clang] Missing header for Objective-C++ test #87023

Closed

sam-mccall added a commit to sam-mccall/llvm-project that referenced this pull request Apr 2, 2024

Reapply "[clang][nullability] allow _Nonnull etc on nullable class ty…

dfcb5dd

…pes (llvm#82705)" This reverts commit 28760b6. The last commit was missing the new testcase, now fixed.

sam-mccall added a commit that referenced this pull request Apr 2, 2024

Reapply "[clang][nullability] allow _Nonnull etc on nullable class ty…

7ef602b

…pes (#82705)" (#87325) This reverts commit 28760b6. The last commit was missing the new testcase, now fixed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][nullability] allow _Nonnull etc on nullable class types #82705

[clang][nullability] allow _Nonnull etc on nullable class types #82705

Uh oh!

sam-mccall commented Feb 22, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 22, 2024 •

edited

Loading

Uh oh!

llvmbot commented Mar 2, 2024 •

edited

Loading

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Uh oh!

AaronBallman Mar 13, 2024

Uh oh!

tahonermann Mar 14, 2024

Uh oh!

tahonermann Mar 14, 2024

Uh oh!

dyung commented Mar 29, 2024 •

edited

Loading

Uh oh!

jyknight commented May 9, 2024

Uh oh!

Uh oh!

[clang][nullability] allow _Nonnull etc on nullable class types #82705

[clang][nullability] allow _Nonnull etc on nullable class types #82705

Uh oh!

Conversation

sam-mccall commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

tahonermann Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

tahonermann Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

dyung commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyknight commented May 9, 2024

Uh oh!

Uh oh!

sam-mccall commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024 •

edited

Loading

llvmbot commented Mar 2, 2024 •

edited

Loading

dyung commented Mar 29, 2024 •

edited

Loading