Warn when 'assume_nonnull' infers nullability within an array.

[jordan_rose]

Summary

...or within a reference. Both of these add an extra level of indirection that make us less certain that the pointer really was supposed to be non-nullable. However, changing the default behavior would be a breaking change, so we'll just make it a warning instead.

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

[jordan_rose]

Depends on D26108 too, for the tests to apply cleanly.

[doug.gregor]

LGTM!

[jordan_rose]

Committed as rL286521.

include/clang/Basic/DiagnosticGroups.td

 def MismatchedTags : DiagGroup<"mismatched-tags">;
 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 NullabilityInferredOnNestedType : DiagGroup<"nullability-inferred-on-nested-type">;
 def NullableToNonNullConversion : DiagGroup<"nullable-to-nonnull-conversion">;
 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 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

   "%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 warn_nullability_inferred_on_nested_type : Warning<
+  "inferring '_Nonnull' for pointer type within %select{array|reference}0 is "
+  "deprecated">,
+  InGroup<NullabilityInferredOnNestedType>;
+
 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

     MultiLevelPointer,
     // CFFooRef*
     MaybePointerToCFRef,
     // CFErrorRef*
     CFErrorRefPointer,
     // NSError**
     NSErrorPointerPointer,
   };
+
+  /// Describes a declarator chunk wrapping a pointer that marks inference as
+  /// unexpected.
+  // These values must be kept in sync with diagnostics.
+  enum class PointerWrappingDeclaratorKind {
+    /// Pointer is top-level.
+    None = -1,
+    /// Pointer is an array element.
+    Array = 0,
+    /// Pointer is the referent type of a C++ reference.
+    Reference = 1
+  };
 } // end anonymous namespace
 
 /// Classify the given declarator, whose type-specified is \c type, based on
 /// what kind of pointer it refers to.
 ///
 /// This is used to determine the default nullability.
-static PointerDeclaratorKind classifyPointerDeclarator(Sema &S,
-                                                       QualType type,
-                                                       Declarator &declarator) {
+static PointerDeclaratorKind
+classifyPointerDeclarator(Sema &S, QualType type, Declarator &declarator,
+                          PointerWrappingDeclaratorKind &wrappingKind) {
   unsigned numNormalPointers = 0;
 
   // For any dependent type, we consider it a non-pointer.
   if (type->isDependentType())
     return PointerDeclaratorKind::NonPointer;
 
   // Look through the declarator chunks to identify pointers.
   for (unsigned i = 0, n = declarator.getNumTypeObjects(); i != n; ++i) {
     DeclaratorChunk &chunk = declarator.getTypeObject(i);
     switch (chunk.Kind) {
     case DeclaratorChunk::Array:
+      if (numNormalPointers == 0)
+        wrappingKind = PointerWrappingDeclaratorKind::Array;
+      break;
+
     case DeclaratorChunk::Function:
     case DeclaratorChunk::Pipe:
       break;
 
     case DeclaratorChunk::BlockPointer:
     case DeclaratorChunk::MemberPointer:
       return numNormalPointers > 0 ? PointerDeclaratorKind::MultiLevelPointer
                                    : PointerDeclaratorKind::SingleLevelPointer;
 
     case DeclaratorChunk::Paren:
+      break;
+
     case DeclaratorChunk::Reference:
-      continue;
+      if (numNormalPointers == 0)
+        wrappingKind = PointerWrappingDeclaratorKind::Reference;
+      break;
 
     case DeclaratorChunk::Pointer:
       ++numNormalPointers;
       if (numNormalPointers > 2)
         return PointerDeclaratorKind::MultiLevelPointer;
-      continue;
+      break;
     }
   }
 
   // Then, dig into the type specifier itself.
   unsigned numTypeSpecifierPointers = 0;
   do {
     // Decompose normal pointers.
     if (auto ptrType = type->getAs<PointerType>()) {

     /// Complain on the inner pointers (but not the outermost
     /// pointer).
     CAMN_InnerPointers,
     /// Complain about any pointers that don't have nullability
     /// specified or inferred.
     CAMN_Yes
   } complainAboutMissingNullability = CAMN_No;
   unsigned NumPointersRemaining = 0;
+  auto complainAboutInferringWithinChunk = PointerWrappingDeclaratorKind::None;
 
   if (IsTypedefName) {
     // For typedefs, we do not infer any nullability (the default),
     // and we only complain about missing nullability specifiers on
     // inner pointers.
     complainAboutMissingNullability = CAMN_InnerPointers;
 
     if (T->canHaveNullability() && !T->getNullability(S.Context)) {

       if (state.getDeclarator().isObjCWeakProperty() && inAssumeNonNullRegion) {
         inferNullability = NullabilityKind::Nullable;
         break;
       }
 
       // fallthrough
 
     case Declarator::FileContext:
-    case Declarator::KNRTypeListContext:
+    case Declarator::KNRTypeListContext: {
       complainAboutMissingNullability = CAMN_Yes;
 
       // Nullability inference depends on the type and declarator.
-      switch (classifyPointerDeclarator(S, T, D)) {
+      auto wrappingKind = PointerWrappingDeclaratorKind::None;
+      switch (classifyPointerDeclarator(S, T, D, wrappingKind)) {
       case PointerDeclaratorKind::NonPointer:
       case PointerDeclaratorKind::MultiLevelPointer:
         // Cannot infer nullability.
         break;
 
       case PointerDeclaratorKind::SingleLevelPointer:
         // Infer _Nonnull if we are in an assumes-nonnull region.
         if (inAssumeNonNullRegion) {
+          complainAboutInferringWithinChunk = wrappingKind;
           inferNullability = NullabilityKind::NonNull;
           inferNullabilityCS = (context == Declarator::ObjCParameterContext ||
                                 context == Declarator::ObjCResultContext);
         }
         break;
 
       case PointerDeclaratorKind::CFErrorRefPointer:
       case PointerDeclaratorKind::NSErrorPointerPointer:

               inferNullability = NullabilityKind::Nullable;
               inferNullabilityInnerOnly = true;
             }
           }
         }
         break;
       }
       break;
+    }
 
     case Declarator::ConversionIdContext:
       complainAboutMissingNullability = CAMN_Yes;
       break;
 
     case Declarator::AliasDeclContext:
     case Declarator::AliasTemplateContext:
     case Declarator::BlockContext:

 
       spliceAttrIntoList(*nullabilityAttr, attrs);
 
       if (inferNullabilityCS) {
         state.getDeclarator().getMutableDeclSpec().getObjCQualifiers()
           ->setObjCDeclQualifier(ObjCDeclSpec::DQ_CSNullability);
       }
 
+      if (pointerLoc.isValid() &&
+          complainAboutInferringWithinChunk !=
+            PointerWrappingDeclaratorKind::None) {
+        SourceLocation fixItLoc = S.getLocForEndOfToken(pointerLoc);
+        StringRef insertionText = " _Nonnull ";
+        if (const char *nextChar = S.SourceMgr.getCharacterData(fixItLoc)) {
+          if (isWhitespace(*nextChar)) {
+            insertionText = insertionText.drop_back();
+          } else if (!isIdentifierBody(nextChar[0], /*allow dollar*/true) &&
+                     !isIdentifierBody(nextChar[-1], /*allow dollar*/true)) {
+            insertionText = insertionText.drop_back().drop_front();
+          }
+        }
+
+        S.Diag(pointerLoc, diag::warn_nullability_inferred_on_nested_type)
+          << static_cast<int>(complainAboutInferringWithinChunk)
+          << FixItHint::CreateInsertion(fixItLoc, insertionText);
+      }
+
       if (inferNullabilityInnerOnly)
         inferNullabilityInnerOnlyComplete = true;
       return nullabilityAttr;
     }
 
     // If we're supposed to complain about missing nullability, do so
     // now if it's truly missing.
     switch (complainAboutMissingNullability) {

test/FixIt/nullability.mm

+// RUN: %clang_cc1 -fsyntax-only -fblocks -std=gnu++11 -verify %s
+// RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fblocks -std=gnu++11 %s 2>&1 | FileCheck %s
+
+#pragma clang assume_nonnull begin
+
+extern void *array[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull "
+
+extern void* array2[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:" _Nonnull"
+
+extern void *nestedArray[2][3]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull "
+
+
+typedef const void *CFTypeRef;
+
+extern CFTypeRef typedefArray[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:" _Nonnull"
+
+
+extern void *&ref; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:"_Nonnull"
+
+extern void * &ref2; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull"
+
+extern void *&&ref3; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:"_Nonnull"
+
+extern void * &&ref4; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull"
+
+extern void *(&arrayRef)[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:"_Nonnull"
+
+extern void * (&arrayRef2)[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull"
+
+extern CFTypeRef &typedefRef; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:" _Nonnull"
+extern CFTypeRef& typedefRef2; // expected-warning {{inferring '_Nonnull' for pointer type within reference is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:" _Nonnull "
+
+
+void arrayNameless(void *[]); // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:26-[[@LINE-1]]:26}:"_Nonnull"
+
+void arrayNameless2(void * []); // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:27-[[@LINE-1]]:27}:" _Nonnull"
+
+void arrayNamelessTypedef(CFTypeRef[]); // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:36-[[@LINE-1]]:36}:" _Nonnull "
+
+void arrayNamelessTypedef2(CFTypeRef []); // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:37}:" _Nonnull"
+
+
+extern int (*pointerToArray)[2]; // no-warning
+int checkTypePTA = pointerToArray; // expected-error {{cannot initialize a variable of type 'int' with an lvalue of type 'int (* _Nonnull)[2]'}}
+
+int **arrayOfNestedPointers[2]; // no-warning
+int checkTypeANP = arrayOfNestedPointers; // expected-error {{cannot initialize a variable of type 'int' with an lvalue of type 'int **[2]'}}
+
+CFTypeRef *arrayOfNestedPointersTypedef[2]; // no-warning
+int checkTypeANPT = arrayOfNestedPointersTypedef; // expected-error {{cannot initialize a variable of type 'int' with an lvalue of type 'CFTypeRef *[2]'}}
+
+#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
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type %s -Wno-nullability-completeness -Werror
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type -x objective-c %s -D ARRAYS_CHECKED=1 -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type -x objective-c %s -D ARRAYS_CHECKED=0 -Wno-nullability-completeness-on-arrays -verify
+// RUN: %clang_cc1 -fsyntax-only -fblocks -I %S/Inputs -Wno-nullability-inferred-on-nested-type -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) { }