Add -Wnullability-completeness-on-arrays.

[jordan_rose]

Summary

This is an addition to (and sub-warning of) -Wnullability-completeness that warns when an array parameter is missing nullability. When the specific warning is switched off, the compiler falls back to only warning on pointer types written as pointer types.

Note that use of nullability within an array triggers the completeness checks regardless of whether or not the array-specific warning is enabled; the intent there is simply to determine whether a particular header is trying to be nullability-aware at all.

Depends on D25850. Part of rdar://problem/25846421.

[ahatanak]

Hi Jordan,

It seems that this patch doesn't apply cleanly. Can you rebase it?

[jordan_rose]

It works fine for me, though note the "depends on D25850". The other patches in the series do seem to have been thrown off by D26226 landing first, though, so I'll update those.

[doug.gregor]

Looks good! I appreciate the refactoring of recordNullabilitySeen

[jordan_rose]

Ah, my apologies, @ahatanak. I was testing against the Swift branch of Clang.

[ahatanak]

In D26108#591270, @jordan_rose wrote:

Ah, my apologies, @ahatanak. I was testing against the Swift branch of Clang.

That's OK. I haven't tested it yet, but the patch itself looks fine to me.

lib/Sema/SemaType.cpp

3988

This isn't something that is related to the changes made in this patch, but it looks like we issue a -Wnullability-completeness warning when there is a generic lambda like this:

auto G2 = [](auto a){};

Is this expected?

[jordan_rose]

lib/Sema/SemaType.cpp

3988

It's not desired, but it's not a regression either. D26226 mentions how this comes up with explicit templates, so it doesn't surprise me that it happens with generic lambdas too.

[ahatanak]

lib/Sema/SemaType.cpp

3988

It turns out it has nothing to do with this patch or your other patches. The warning is issued when there is a variable initialized with any kind of lambda:

auto G2 = [](int a){};

This happens because Type::canHaveNullability() returns true when it is an undeduced auto and checkNullabilityConsistency records its location.

[jordan_rose]

Committed as rL286520, with a slight fix-up for MSVC in rL286525.

[rsanchezsaez]

I think this makes the warning to trigger on methods such as:

- (instancetype)initWithValues:(const int32_t [])values;
- (void)updateWithRange:(NSRange)range floatArray:(CGFloat[])array;

(experienced on the just released Xcode 8.3).

Perhaps I'm misunderstanding something, but shouldn't the warning only apply to arrays of pointer types?

[jordan_rose]

No, the arrays themselves need to be marked as nullable or non-nullable. Remember that in C array parameters are passed as pointers.

The compiler should give you a fix-it that shows the correct syntax.

[rsanchezsaez]

Oh, I see. Thanks for the explanation.

The fix-it syntax got me confused:

- (instancetype)initWithValues:(const int32_t [_Null_unspecified])values

[jordan_rose]

Yep, that's the correct syntax. It's not wonderful, but it's already where the C standard lets you put const, so we're just following established practice. (Learn something new and awful about C every day!)

[rsanchezsaez]

Haha! That makes sense. (I miss GitHub's comment reactions on Phabricator, consider your comments 👍'ed.)

include/clang/Basic/DiagnosticGroups.td

 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleConflict : DiagGroup<"module-conflict">;
 def ModuleFileExtension : DiagGroup<"module-file-extension">;
 def NewlineEOF : DiagGroup<"newline-eof">;
 def Nullability : DiagGroup<"nullability">;
 def NullabilityDeclSpec : DiagGroup<"nullability-declspec">;
 def NullableToNonNullConversion : DiagGroup<"nullable-to-nonnull-conversion">;
-def NullabilityCompleteness : DiagGroup<"nullability-completeness">;
+def NullabilityCompletenessOnArrays : DiagGroup<"nullability-completeness-on-arrays">;
+def NullabilityCompleteness : DiagGroup<"nullability-completeness",
+                                        [NullabilityCompletenessOnArrays]>;
 def NullArithmetic : DiagGroup<"null-arithmetic">;
 def NullCharacter : DiagGroup<"null-character">;
 def NullDereference : DiagGroup<"null-dereference">;
 def InitializerOverrides : DiagGroup<"initializer-overrides">;
 def NonNull : DiagGroup<"nonnull">;
 def NonPODVarargs : DiagGroup<"non-pod-varargs">;
 def ClassVarargs : DiagGroup<"class-varargs", [NonPODVarargs]>;
 def : DiagGroup<"nonportable-cfstrings">;

 
 def OptionIgnored : DiagGroup<"option-ignored">;
 
 def UnknownArgument : DiagGroup<"unknown-argument">;
 
 // A warning group for warnings about code that clang accepts when
 // compiling OpenCL C/C++ but which is not compatible with the SPIR spec.
 def SpirCompat : DiagGroup<"spir-compat">;
 No newline at end of file

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_null_resettable_setter : Warning<
   "synthesized setter %0 for null_resettable property %1 does not handle nil">,
   InGroup<Nullability>;
 
 def warn_nullability_missing : Warning<
   "%select{pointer|block pointer|member pointer}0 is missing a nullability "
   "type specifier (_Nonnull, _Nullable, or _Null_unspecified)">,
   InGroup<NullabilityCompleteness>;
+def warn_nullability_missing_array : Warning<
+  "array parameter is missing a nullability type specifier (_Nonnull, "
+  "_Nullable, or _Null_unspecified)">,
+  InGroup<NullabilityCompletenessOnArrays>;
 
 def err_objc_type_arg_explicit_nullability : Error<
   "type argument %0 cannot explicitly specify nullability">;
 
 def err_objc_type_param_bound_explicit_nullability : Error<
   "type parameter %0 bound %1 cannot explicitly specify nullability">;
 
 }

lib/Sema/SemaType.cpp

 
 namespace {
   /// A simple notion of pointer kinds, which matches up with the various
   /// pointer declarators.
   enum class SimplePointerKind {
     Pointer,
     BlockPointer,
     MemberPointer,
+    Array,
   };
 } // end anonymous namespace
 
 IdentifierInfo *Sema::getNullabilityKeyword(NullabilityKind nullability) {
   switch (nullability) {
   case NullabilityKind::NonNull:
     if (!Ident__Nonnull)
       Ident__Nonnull = PP.getIdentifierInfo("_Nonnull");

   if (fileInfo.getFileCharacteristic() != SrcMgr::C_User &&
       S.Diags.getSuppressSystemWarnings()) {
     return FileID();
   }
 
   return file;
 }
 
-/// Check for consistent use of nullability.
-static void checkNullabilityConsistency(TypeProcessingState &state,
+/// Complains about missing nullability if the file containing \p pointerLoc
+/// has other uses of nullability (either the keywords or the \c assume_nonnull
+/// pragma).
+///
+/// If the file has \e not seen other uses of nullability, this particular
+/// pointer is saved for possible later diagnosis. See recordNullabilitySeen().
+static void checkNullabilityConsistency(Sema &S,
                                         SimplePointerKind pointerKind,
                                         SourceLocation pointerLoc) {
-  Sema &S = state.getSema();
-
   // Determine which file we're performing consistency checking for.
   FileID file = getNullabilityCompletenessCheckFileID(S, pointerLoc);
   if (file.isInvalid())
     return;
 
   // If we haven't seen any type nullability in this file, we won't warn now
   // about anything.
   FileNullability &fileNullability = S.NullabilityMap[file];
   if (!fileNullability.SawTypeNullability) {
-    // If this is the first pointer declarator in the file, record it.
+    // If this is the first pointer declarator in the file, and the appropriate
+    // warning is on, record it in case we need to diagnose it retroactively.
+    diag::kind diagKind;
+    if (pointerKind == SimplePointerKind::Array)
+      diagKind = diag::warn_nullability_missing_array;
+    else
+      diagKind = diag::warn_nullability_missing;
+
     if (fileNullability.PointerLoc.isInvalid() &&
-        !S.Context.getDiagnostics().isIgnored(diag::warn_nullability_missing,
-                                              pointerLoc)) {
+        !S.Context.getDiagnostics().isIgnored(diagKind, pointerLoc)) {
       fileNullability.PointerLoc = pointerLoc;
       fileNullability.PointerKind = static_cast<unsigned>(pointerKind);
     }
 
     return;
   }
 
   // Complain about missing nullability.
-  S.Diag(pointerLoc, diag::warn_nullability_missing)
-    << static_cast<unsigned>(pointerKind);
+  if (pointerKind == SimplePointerKind::Array) {
+    S.Diag(pointerLoc, diag::warn_nullability_missing_array);
+  } else {
+    S.Diag(pointerLoc, diag::warn_nullability_missing)
+      << static_cast<unsigned>(pointerKind);
+  }
+}
+
+/// Marks that a nullability feature has been used in the file containing
+/// \p loc.
+///
+/// If this file already had pointer types in it that were missing nullability,
+/// the first such instance is retroactively diagnosed.
+///
+/// \sa checkNullabilityConsistency
+static void recordNullabilitySeen(Sema &S, SourceLocation loc) {
+  FileID file = getNullabilityCompletenessCheckFileID(S, loc);
+  if (file.isInvalid())
+    return;
+
+  FileNullability &fileNullability = S.NullabilityMap[file];
+  if (fileNullability.SawTypeNullability)
+    return;
+  fileNullability.SawTypeNullability = true;
+
+  // If we haven't seen any type nullability before, now we have. Retroactively
+  // diagnose the first unannotated pointer, if there was one.
+  if (fileNullability.PointerLoc.isInvalid())
+    return;
+
+  if (fileNullability.PointerKind ==
+        static_cast<unsigned>(SimplePointerKind::Array)) {
+    S.Diag(fileNullability.PointerLoc, diag::warn_nullability_missing_array);
+  } else {
+    S.Diag(fileNullability.PointerLoc, diag::warn_nullability_missing)
+      << fileNullability.PointerKind;
+  }
 }
 
 /// Returns true if any of the declarator chunks before \p endIndex include a
 /// level of indirection: array, pointer, reference, or pointer-to-member.
 ///
 /// Because declarator chunks are stored in outer-to-inner order, testing
 /// every chunk before \p endIndex is testing all chunks that embed the current
 /// chunk as part of their type.

   // Determine whether we should infer _Nonnull on pointer types.
   Optional<NullabilityKind> inferNullability;
   bool inferNullabilityCS = false;
   bool inferNullabilityInnerOnly = false;
   bool inferNullabilityInnerOnlyComplete = false;
 
   // Are we in an assume-nonnull region?
   bool inAssumeNonNullRegion = false;
-  if (S.PP.getPragmaAssumeNonNullLoc().isValid()) {
+  SourceLocation assumeNonNullLoc = S.PP.getPragmaAssumeNonNullLoc();
+  if (assumeNonNullLoc.isValid()) {
     inAssumeNonNullRegion = true;
-    // Determine which file we saw the assume-nonnull region in.
-    FileID file = getNullabilityCompletenessCheckFileID(
-                    S, S.PP.getPragmaAssumeNonNullLoc());
-    if (file.isValid()) {
-      FileNullability &fileNullability = S.NullabilityMap[file];
-
-      // If we haven't seen any type nullability before, now we have.
-      if (!fileNullability.SawTypeNullability) {
-        if (fileNullability.PointerLoc.isValid()) {
-          S.Diag(fileNullability.PointerLoc, diag::warn_nullability_missing)
-            << static_cast<unsigned>(fileNullability.PointerKind);
-        }
-
-        fileNullability.SawTypeNullability = true;
-      }
-    }
+    recordNullabilitySeen(S, assumeNonNullLoc);
   }
 
   // Whether to complain about missing nullability specifiers or not.
   enum {
     /// Never complain.
     CAMN_No,
     /// Complain on the inner pointers (but not the outermost
     /// pointer).

       break;
 
     case CAMN_InnerPointers:
       if (NumPointersRemaining == 0)
         break;
       // Fallthrough.
 
     case CAMN_Yes:
-      checkNullabilityConsistency(state, pointerKind, pointerLoc);
+      checkNullabilityConsistency(S, pointerKind, pointerLoc);
     }
     return nullptr;
   };
 
   // If the type itself could have nullability but does not, infer pointer
   // nullability and perform consistency checking.
-  if (T->canHaveNullability() && S.ActiveTemplateInstantiations.empty() &&
-      !T->getNullability(S.Context)) {
+  if (S.ActiveTemplateInstantiations.empty()) {
+    if (T->canHaveNullability() && !T->getNullability(S.Context)) {
       SimplePointerKind pointerKind = SimplePointerKind::Pointer;
       if (T->isBlockPointerType())
         pointerKind = SimplePointerKind::BlockPointer;
       else if (T->isMemberPointerType())
         pointerKind = SimplePointerKind::MemberPointer;
-
+
       if (auto *attr = inferPointerNullability(
                          pointerKind, D.getDeclSpec().getTypeSpecTypeLoc(),
                          D.getMutableDeclSpec().getAttributes().getListRef())) {
         T = Context.getAttributedType(
               AttributedType::getNullabilityAttrKind(*inferNullability), T, T);
         attr->setUsedAsTypeAttr();
+      }
+    }
+    if (complainAboutMissingNullability == CAMN_Yes &&
+        T->isArrayType() && !T->getNullability(S.Context) &&
+        D.isPrototypeContext() &&
+        !hasOuterPointerLikeChunk(D, D.getNumTypeObjects())) {
+      checkNullabilityConsistency(S, SimplePointerKind::Array,
+                                  D.getDeclSpec().getTypeSpecTypeLoc());
     }
   }
 
   // Walk the DeclTypeInfo, building the recursive type as we go.
   // DeclTypeInfos are ordered from the identifier out, which is
   // opposite of what we want :).
   for (unsigned i = 0, e = D.getNumTypeObjects(); i != e; ++i) {
     unsigned chunkIndex = e - i - 1;

           if (ASM == ArrayType::Static)
             ASM = ArrayType::Normal;
           ATI.TypeQuals = 0;
           D.setInvalidType(true);
         }
       }
       const AutoType *AT = T->getContainedAutoType();
       // Allow arrays of auto if we are a generic lambda parameter.
       // i.e. [](auto (&array)[5]) { return array[0]; }; OK
       if (AT && D.getContext() != Declarator::LambdaExprParameterContext) {
         // We've already diagnosed this for decltype(auto).
         if (!AT->isDecltypeAuto())
           S.Diag(DeclType.Loc, diag::err_illegal_decl_array_of_auto)
             << getPrintableNameForEntity(Name) << T;
         T = QualType();
         break;
       }
 
+      // Array parameters can be marked nullable as well, although it's not
+      // necessary if they're marked 'static'.
+      if (complainAboutMissingNullability == CAMN_Yes &&
+          !hasNullabilityAttr(DeclType.getAttrs()) &&
+          ASM != ArrayType::Static &&
+          D.isPrototypeContext() &&
+          !hasOuterPointerLikeChunk(D, chunkIndex)) {
+        checkNullabilityConsistency(S, SimplePointerKind::Array, DeclType.Loc);
+      }
+
       T = S.BuildArrayType(T, ASM, ArraySize, ATI.TypeQuals,
                            SourceRange(DeclType.Loc, DeclType.EndLoc), Name);
       break;
     }
     case DeclaratorChunk::Function: {
       // If the function declarator has a prototype (i.e. it is not () and
       // does not have a K&R-style identifier list), then the arguments are part
       // of the type, otherwise the argument list is ().

 
 bool Sema::checkNullabilityTypeSpecifier(QualType &type,
                                          NullabilityKind nullability,
                                          SourceLocation nullabilityLoc,
                                          bool isContextSensitive,
                                          bool allowOnArrayType,
                                          bool implicit,
                                          bool overrideExisting) {
-  if (!implicit) {
-    // We saw a nullability type specifier. If this is the first one for
-    // this file, note that.
-    FileID file = getNullabilityCompletenessCheckFileID(*this, nullabilityLoc);
-    if (!file.isInvalid()) {
-      FileNullability &fileNullability = NullabilityMap[file];
-      if (!fileNullability.SawTypeNullability) {
-        // If we have already seen a pointer declarator without a nullability
-        // annotation, complain about it.
-        if (fileNullability.PointerLoc.isValid()) {
-          Diag(fileNullability.PointerLoc, diag::warn_nullability_missing)
-            << static_cast<unsigned>(fileNullability.PointerKind);
-        }
-
-        fileNullability.SawTypeNullability = true;
-      }
-    }
-  }
+  if (!implicit)
+    recordNullabilitySeen(*this, nullabilityLoc);
 
   // Check for existing nullability attributes on the type.
   QualType desugared = type;
   while (auto attributed = dyn_cast<AttributedType>(desugared.getTypePtr())) {
     // Check whether there is already a null
     if (auto existingNullability = attributed->getImmediateNullability()) {
       // Duplicated nullability.
       if (nullability == *existingNullability) {

test/SemaObjCXX/Inputs/nullability-consistency-arrays.h

+void firstThingInTheFileThatNeedsNullabilityIsAnArray(int ints[]);
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+int *secondThingInTheFileThatNeedsNullabilityIsAPointer;
+#if !ARRAYS_CHECKED
+// expected-warning@-2 {{pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+int *_Nonnull triggerConsistencyWarnings;
+
+void test(
+    int ints[],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+    void *ptrs[], // expected-warning {{pointer is missing a nullability type specifier}}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+    void **nestedPtrs[]); // expected-warning 2 {{pointer is missing a nullability type specifier}}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+void testArraysOK(
+    int ints[_Nonnull],
+    void *ptrs[_Nonnull], // expected-warning {{pointer is missing a nullability type specifier}}
+    void **nestedPtrs[_Nonnull]); // expected-warning 2 {{pointer is missing a nullability type specifier}}
+void testAllOK(
+    int ints[_Nonnull],
+    void * _Nullable ptrs[_Nonnull],
+    void * _Nullable * _Nullable nestedPtrs[_Nonnull]);
+
+void nestedArrays(int x[5][1]) {}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+void nestedArraysOK(int x[_Nonnull 5][1]) {}
+
+#if !__cplusplus
+void staticOK(int x[static 5][1]){}
+#endif
+
+int globalArraysDoNotNeedNullability[5];
+
+typedef int INTS[4];
+
+void typedefTest(
+    INTS x,
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+    INTS _Nonnull x2,
+    _Nonnull INTS x3,
+    INTS y[2],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+    INTS y2[_Nonnull 2]);
+
+
+#pragma clang assume_nonnull begin
+void testAssumeNonnull(
+  int ints[],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+  void *ptrs[],
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+  void **nestedPtrs[]); // expected-warning 2 {{pointer is missing a nullability type specifier}}
+#if ARRAYS_CHECKED
+// expected-warning@-2 {{array parameter is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}
+#endif
+
+void testAssumeNonnullAllOK(
+    int ints[_Nonnull],
+    void * _Nullable ptrs[_Nonnull],
+    void * _Nullable * _Nullable nestedPtrs[_Nonnull]);
+void testAssumeNonnullAllOK2(
+    int ints[_Nonnull],
+    void * ptrs[_Nonnull], // backwards-compatibility
+    void * _Nullable * _Nullable nestedPtrs[_Nonnull]);
+#pragma clang assume_nonnull end

test/SemaObjCXX/nullability-consistency-arrays.mm

+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs %s -Wno-nullability-completeness -Werror
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -x objective-c %s -Wno-nullability-completeness -Werror
+
+#include "nullability-consistency-arrays.h"
+
+void h1(int *ptr) { } // don't warn
+
+void h2(int * _Nonnull p) { }