Don't try to emit nullability fix-its within/around macros.

The newly-added notes from r290132 are too noisy even when the fix-it is valid. For the existing warning from r286521, it's probably the right decision 95% of the time to put the change outside the macro if the array is outside the macro and inside otherwise, but I don't want to overthink it right now. Caught by the ASan bot! More rdar://problem/29524992 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290141 91177308-0d34-0410-b5e6-96231b3b80d8
johanst · Dec 19, 2016 · 64caf82 · 64caf82
1 parent 3efe802
commit 64caf82
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 22 deletions.
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
@@ -3443,31 +3443,39 @@ static FileID getNullabilityCompletenessCheckFileID(Sema &S,
 
 /// Creates a fix-it to insert a C-style nullability keyword at \p pointerLoc,
 /// taking into account whitespace before and after.
-static FixItHint fixItNullability(Sema &S, SourceLocation PointerLoc,
-                                  NullabilityKind Nullability) {
+static void fixItNullability(Sema &S, DiagnosticBuilder &Diag,
+                             SourceLocation PointerLoc,
+                             NullabilityKind Nullability) {
   assert(PointerLoc.isValid());
+  if (PointerLoc.isMacroID())
+    return;
+
+  SourceLocation FixItLoc = S.getLocForEndOfToken(PointerLoc);
+  if (!FixItLoc.isValid() || FixItLoc == PointerLoc)
+    return;
+
+  const char *NextChar = S.SourceMgr.getCharacterData(FixItLoc);
+  if (!NextChar)
+    return;
 
   SmallString<32> InsertionTextBuf{" "};
   InsertionTextBuf += getNullabilitySpelling(Nullability);
   InsertionTextBuf += " ";
   StringRef InsertionText = InsertionTextBuf.str();
 
-  SourceLocation FixItLoc = S.getLocForEndOfToken(PointerLoc);
-  if (const char *NextChar = S.SourceMgr.getCharacterData(FixItLoc)) {
-    if (isWhitespace(*NextChar)) {
-      InsertionText = InsertionText.drop_back();
-    } else if (NextChar[-1] == '[') {
-      if (NextChar[0] == ']')
-        InsertionText = InsertionText.drop_back().drop_front();
-      else
-        InsertionText = InsertionText.drop_front();
-    } else if (!isIdentifierBody(NextChar[0], /*allow dollar*/true) &&
-               !isIdentifierBody(NextChar[-1], /*allow dollar*/true)) {
+  if (isWhitespace(*NextChar)) {
+    InsertionText = InsertionText.drop_back();
+  } else if (NextChar[-1] == '[') {
+    if (NextChar[0] == ']')
       InsertionText = InsertionText.drop_back().drop_front();
-    }
+    else
+      InsertionText = InsertionText.drop_front();
+  } else if (!isIdentifierBody(NextChar[0], /*allow dollar*/true) &&
+             !isIdentifierBody(NextChar[-1], /*allow dollar*/true)) {
+    InsertionText = InsertionText.drop_back().drop_front();
   }
 
-  return FixItHint::CreateInsertion(FixItLoc, InsertionText);
+  Diag << FixItHint::CreateInsertion(FixItLoc, InsertionText);
 }
 
 static void emitNullabilityConsistencyWarning(Sema &S,
@@ -3482,11 +3490,14 @@ static void emitNullabilityConsistencyWarning(Sema &S,
       << static_cast<unsigned>(PointerKind);
   }
 
+  if (PointerLoc.isMacroID())
+    return;
+
   auto addFixIt = [&](NullabilityKind Nullability) {
-    S.Diag(PointerLoc, diag::note_nullability_fix_it)
-      << static_cast<unsigned>(Nullability)
-      << static_cast<unsigned>(PointerKind)
-      << fixItNullability(S, PointerLoc, Nullability);
+    auto Diag = S.Diag(PointerLoc, diag::note_nullability_fix_it);
+    Diag << static_cast<unsigned>(Nullability);
+    Diag << static_cast<unsigned>(PointerKind);
+    fixItNullability(S, Diag, PointerLoc, Nullability);
   };
   addFixIt(NullabilityKind::Nullable);
   addFixIt(NullabilityKind::NonNull);
@@ -3888,9 +3899,10 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
       if (pointerLoc.isValid() &&
           complainAboutInferringWithinChunk !=
             PointerWrappingDeclaratorKind::None) {
-        S.Diag(pointerLoc, diag::warn_nullability_inferred_on_nested_type)
-          << static_cast<int>(complainAboutInferringWithinChunk)
-          << fixItNullability(S, pointerLoc, NullabilityKind::NonNull);
+        auto Diag =
+            S.Diag(pointerLoc, diag::warn_nullability_inferred_on_nested_type);
+        Diag << static_cast<int>(complainAboutInferringWithinChunk);
+        fixItNullability(S, Diag, pointerLoc, NullabilityKind::NonNull);
       }
 
       if (inferNullabilityInnerOnly)

diff --git a/test/FixIt/Inputs/nullability.h b/test/FixIt/Inputs/nullability.h
@@ -17,3 +17,11 @@ void arrayParameterWithStar(int x[*]); // expected-warning {{array parameter is
 // expected-note@-2 {{insert '_Nonnull'}}
 // CHECK: fix-it:"{{.*}}nullability.h":{[[@LINE-3]]:35-[[@LINE-3]]:35}:"_Nullable "
 // CHECK: fix-it:"{{.*}}nullability.h":{[[@LINE-4]]:35-[[@LINE-4]]:35}:"_Nonnull "
+
+
+// No fix-its on either the macro definition or instantiation.
+// CHECK-NOT: fix-it:"{{.*}}nullability.h":{[[@LINE+2]]
+// CHECK-NOT: fix-it:"{{.*}}nullability.h":{[[@LINE+2]]
+#define PTR(X) X *
+PTR(int) a; // expected-warning{{pointer is missing a nullability type specifier}}
+#undef PTR
diff --git a/test/FixIt/nullability.mm b/test/FixIt/nullability.mm
@@ -16,6 +16,12 @@
 extern void *nestedArray[2][3]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
 // CHECK: fix-it:"{{.*}}nullability.mm":{[[@LINE-1]]:14-[[@LINE-1]]:14}:" _Nonnull "
 
+// No fix-its on either the macro definition or instantiation.
+// CHECK-NOT: fix-it:"{{.*}}nullability.mm":{[[@LINE+2]]
+// CHECK-NOT: fix-it:"{{.*}}nullability.mm":{[[@LINE+2]]
+#define PTR(X) X *
+extern PTR(void) array[2]; // expected-warning {{inferring '_Nonnull' for pointer type within array is deprecated}}
+
 
 typedef const void *CFTypeRef;