[Sema] Put nullability fix-it after the end of the pointer.

[vsapsai]

Summary

Fixes nullability fix-it for id<SomeProtocol>. With this change
nullability specifier is inserted after ">" instead of between
"id" and "<".

rdar://problem/34260995

[jordan_rose]

Nice catch, Volodymyr! Looks good to me.

[vsapsai]

To preempt some of review feedback here are attempted and rejected approaches:

• Pass pointerLoc and pointerEndLoc as pointerRange. Such source range can be misleading as pointerLoc doesn't necesserily point at the beginning. For example, for int *x pointer range would be "*" and I would expect it to be "int *". So it's not really a range but 2 related locations.

• Use D.getDeclSpec().getLocEnd() instead of D.getDeclSpec().getTypeSpecTypeLoc(). In this case warning location points at the closing angle bracket and that can be confusing to developers. It looks like

./test.h:14:3: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)
  id<SomeProtocol> thingies;
                 ^

• Use pointerLoc for insert note instead of pointerEndLoc. It looks like

./test.h:14:3: note: insert '_Nullable' if the pointer may be null
  id<SomeProtocol> thingies;
  ^
                   _Nullable

compared to suggested

./test.h:14:18: note: insert '_Nullable' if the pointer may be null
  id<SomeProtocol> thingies;
                 ^
                   _Nullable

I don't expect developers to know that they should match whitespace preceding _Nullable to calculate where in the line it should be inserted. And I think developers shouldn't care about it. So put the cursor where you expect the text to be inserted.

[vsapsai]

In D38327#882540, @jordan_rose wrote:

Nice catch, Volodymyr! Looks good to me.

Thanks for the quick review, Jordan.

cfe/trunk/include/clang/Sema/Sema.h

 
 /// Describes whether we've seen any nullability information for the given
 /// file.
 struct FileNullability {
   /// The first pointer declarator (of any pointer kind) in the file that does
   /// not have a corresponding nullability annotation.
   SourceLocation PointerLoc;
 
+  /// The end location for the first pointer declarator in the file. Used for
+  /// placing fix-its.
+  SourceLocation PointerEndLoc;
+
   /// Which kind of pointer declarator we saw.
   uint8_t PointerKind;
 
   /// Whether we saw any type nullability annotations in the given file.
   bool SawTypeNullability = false;
 };
 
 /// A mapping from file IDs to a record of whether we've seen nullability

cfe/trunk/lib/Sema/SemaType.cpp

if (isWhitespace(*NextChar)) {
     InsertionText = InsertionText.drop_back().drop_front();
   }
 
   Diag << FixItHint::CreateInsertion(FixItLoc, InsertionText);
 }
 
 static void emitNullabilityConsistencyWarning(Sema &S,
                                               SimplePointerKind PointerKind,
-                                              SourceLocation PointerLoc) {
+                                              SourceLocation PointerLoc,
+                                              SourceLocation PointerEndLoc) {
   assert(PointerLoc.isValid());
 
   if (PointerKind == SimplePointerKind::Array) {
     S.Diag(PointerLoc, diag::warn_nullability_missing_array);
   } else {
     S.Diag(PointerLoc, diag::warn_nullability_missing)
       << static_cast<unsigned>(PointerKind);
   }
 
-  if (PointerLoc.isMacroID())
+  auto FixItLoc = PointerEndLoc.isValid() ? PointerEndLoc : PointerLoc;
+  if (FixItLoc.isMacroID())
     return;
 
   auto addFixIt = [&](NullabilityKind Nullability) {
-    auto Diag = S.Diag(PointerLoc, diag::note_nullability_fix_it);
+    auto Diag = S.Diag(FixItLoc, diag::note_nullability_fix_it);
     Diag << static_cast<unsigned>(Nullability);
     Diag << static_cast<unsigned>(PointerKind);
-    fixItNullability(S, Diag, PointerLoc, Nullability);
+    fixItNullability(S, Diag, FixItLoc, Nullability);
   };
   addFixIt(NullabilityKind::Nullable);
   addFixIt(NullabilityKind::NonNull);
 }
 
 /// 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) {
+static void
+checkNullabilityConsistency(Sema &S, SimplePointerKind pointerKind,
+                            SourceLocation pointerLoc,
+                            SourceLocation pointerEndLoc = SourceLocation()) {
   // 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, 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(diagKind, pointerLoc)) {
       fileNullability.PointerLoc = pointerLoc;
+      fileNullability.PointerEndLoc = pointerEndLoc;
       fileNullability.PointerKind = static_cast<unsigned>(pointerKind);
     }
 
     return;
   }
 
   // Complain about missing nullability.
-  emitNullabilityConsistencyWarning(S, pointerKind, pointerLoc);
+  emitNullabilityConsistencyWarning(S, pointerKind, pointerLoc, pointerEndLoc);
 }
 
 /// 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.
 ///
static void recordNullabilitySeen(Sema &S, SourceLocation loc) {
   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;
 
   auto kind = static_cast<SimplePointerKind>(fileNullability.PointerKind);
-  emitNullabilityConsistencyWarning(S, kind, fileNullability.PointerLoc);
+  emitNullabilityConsistencyWarning(S, kind, fileNullability.PointerLoc,
+                                    fileNullability.PointerEndLoc);
 }
 
 /// 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.
auto isVaList = [&S](QualType T) -> bool {
     } while (typedefTy);
     return false;
   };
 
   // Local function that checks the nullability for a given pointer declarator.
   // Returns true if _Nonnull was inferred.
   auto inferPointerNullability = [&](SimplePointerKind pointerKind,
                                      SourceLocation pointerLoc,
+                                     SourceLocation pointerEndLoc,
                                      AttributeList *&attrs) -> AttributeList * {
     // We've seen a pointer.
     if (NumPointersRemaining > 0)
       --NumPointersRemaining;
 
     // If a nullability attribute is present, there's nothing to do.
     if (hasNullabilityAttr(attrs))
       return nullptr;
case CAMN_No:
       break;
 
     case CAMN_InnerPointers:
       if (NumPointersRemaining == 0)
         break;
       // Fallthrough.
 
     case CAMN_Yes:
-      checkNullabilityConsistency(S, pointerKind, pointerLoc);
+      checkNullabilityConsistency(S, pointerKind, pointerLoc, pointerEndLoc);
     }
     return nullptr;
   };
 
   // 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(S.Context)) {
       if (isVaList(T)) {
         // Record that we've seen a pointer, but do nothing else.
         if (NumPointersRemaining > 0)
           --NumPointersRemaining;
       } else {
         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.getDeclSpec().getLocEnd(),
               D.getMutableDeclSpec().getAttributes().getListRef())) {
           T = Context.getAttributedType(
                 AttributedType::getNullabilityAttrKind(*inferNullability),T,T);
           attr->setUsedAsTypeAttr();
         }
       }
     }
 
case DeclaratorChunk::Paren:
       T = S.BuildParenType(T);
       break;
     case DeclaratorChunk::BlockPointer:
       // If blocks are disabled, emit an error.
       if (!LangOpts.Blocks)
         S.Diag(DeclType.Loc, diag::err_blocks_disable) << LangOpts.OpenCL;
 
       // Handle pointer nullability.
-      inferPointerNullability(SimplePointerKind::BlockPointer,
-                              DeclType.Loc, DeclType.getAttrListRef());
+      inferPointerNullability(SimplePointerKind::BlockPointer, DeclType.Loc,
+                              DeclType.EndLoc, DeclType.getAttrListRef());
 
       T = S.BuildBlockPointerType(T, D.getIdentifierLoc(), Name);
       if (DeclType.Cls.TypeQuals || LangOpts.OpenCL) {
         // OpenCL v2.0, s6.12.5 - Block variable declarations are implicitly
         // qualified with const.
         if (LangOpts.OpenCL)
           DeclType.Cls.TypeQuals |= DeclSpec::TQ_const;
         T = S.BuildQualifiedType(T, DeclType.Loc, DeclType.Cls.TypeQuals);
       }
       break;
     case DeclaratorChunk::Pointer:
       // Verify that we're not building a pointer to pointer to function with
       // exception specification.
       if (LangOpts.CPlusPlus && S.CheckDistantExceptionSpec(T)) {
         S.Diag(D.getIdentifierLoc(), diag::err_distant_exception_spec);
         D.setInvalidType(true);
         // Build the type anyway.
       }
 
       // Handle pointer nullability
       inferPointerNullability(SimplePointerKind::Pointer, DeclType.Loc,
-                              DeclType.getAttrListRef());
+                              DeclType.EndLoc, DeclType.getAttrListRef());
 
       if (LangOpts.ObjC1 && T->getAs<ObjCObjectType>()) {
         T = Context.getObjCObjectPointerType(T);
         if (DeclType.Ptr.TypeQuals)
           T = S.BuildQualifiedType(T, DeclType.Loc, DeclType.Ptr.TypeQuals);
         break;
       }
 
case DeclaratorChunk::Function: {
       break;
     }
     case DeclaratorChunk::MemberPointer: {
       // The scope spec must refer to a class, or be dependent.
       CXXScopeSpec &SS = DeclType.Mem.Scope();
       QualType ClsType;
 
       // Handle pointer nullability.
-      inferPointerNullability(SimplePointerKind::MemberPointer,
-                              DeclType.Loc, DeclType.getAttrListRef());
+      inferPointerNullability(SimplePointerKind::MemberPointer, DeclType.Loc,
+                              DeclType.EndLoc, DeclType.getAttrListRef());
 
       if (SS.isInvalid()) {
         // Avoid emitting extra errors if we already errored on the scope.
         D.setInvalidType(true);
       } else if (S.isDependentScopeSpecifier(SS) ||
                  dyn_cast_or_null<CXXRecordDecl>(S.computeDeclContext(SS))) {
         NestedNameSpecifier *NNS = SS.getScopeRep();
         NestedNameSpecifier *NNSPrefix = NNS->getPrefix();

cfe/trunk/test/FixIt/Inputs/nullability-objc.h

+@class Item;
+@class Container<ObjectType>;
+@protocol Protocol;
+
+// rdar://problem/34260995
+// The first pointer in the file is handled in a different way so need
+// a separate test for this case even if the parameter type is the same as in
+// objcIdParameterWithProtocol.
+void objcIdParameterWithProtocolFirstInFile(id<Protocol> i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull"
+
+int * _Nonnull forceNullabilityWarningsObjC(void);
+
+void objcClassParameter(Item *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:31-[[@LINE-3]]:31}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:31-[[@LINE-4]]:31}:" _Nonnull "
+
+void objcClassParameterWithProtocol(Item<Protocol> *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:53-[[@LINE-3]]:53}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:53-[[@LINE-4]]:53}:" _Nonnull "
+
+// rdar://problem/34260995
+void objcIdParameterWithProtocol(id<Protocol> i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:46-[[@LINE-3]]:46}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:46-[[@LINE-4]]:46}:" _Nonnull"
+
+// Class parameters don't have nullability type specifier.
+void objcParameterizedClassParameter(Container<Item *> *c); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull "
+
+// Class parameters don't have nullability type specifier.
+void objcParameterizedClassParameterWithProtocol(Container<id<Protocol>> *c); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:75-[[@LINE-3]]:75}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:75-[[@LINE-4]]:75}:" _Nonnull "

cfe/trunk/test/FixIt/nullability.mm

 // RUN: %clang_cc1 -fsyntax-only -fblocks -std=gnu++11 -I %S/Inputs -verify %s
 // RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fblocks -std=gnu++11 -I %S/Inputs %s >%t.txt 2>&1
 // RUN: FileCheck %s < %t.txt
 // RUN: FileCheck %S/Inputs/nullability.h < %t.txt
+// RUN: FileCheck %S/Inputs/nullability-objc.h < %t.txt
 
 #include "nullability.h"
+#include "nullability-objc.h"
 
 #pragma clang assume_nonnull begin
 
 extern void *array[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
 // CHECK: fix-it:"{{.*}}nullability.mm":{[[@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:"{{.*}}nullability.mm":{[[@LINE-1]]:13-[[@LINE-1]]:13}:" _Nonnull"

cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h

 void g1(int * _Nonnull);
 
 void g2(int (^block)(int, int)); // expected-warning{{block pointer is missing a nullability type specifier}}
 // expected-note@-1 {{insert '_Nullable' if the block pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the block pointer should never be null}}
 
 void g3(const
         id // expected-warning{{missing a nullability type specifier}}
+        volatile
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
-        volatile
         * // expected-warning{{missing a nullability type specifier}}
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
         ); 
 
 @interface SomeClass
 @property (retain,nonnull) id property1;
 @property (retain,nullable) SomeClass *property2;