Initial steps to improve diagnostics when there is a NULL and

a non-pointer on the two sides of a conditional expression.

Patch by Stephen Hines and Mihai Rusu.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@125995 91177308-0d34-0410-b5e6-96231b3b80d8

10 files changed, 140 insertions, 22 deletions

diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index 36644784c9..a17205c2b6 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -437,6 +437,22 @@ public:
   /// EvaluateAsLValue - Evaluate an expression to see if it's a lvalue.
   bool EvaluateAsAnyLValue(EvalResult &Result, const ASTContext &Ctx) const;
 
+  /// \brief Enumeration used to describe the kind of Null pointer constant
+  /// returned from \c isNullPointerConstant().
+  enum NullPointerConstantKind {
+    /// \brief Expression is not a Null pointer constant.
+    NPCK_NotNull = 0,
+
+    /// \brief Expression is a Null pointer constant built from a zero integer.
+    NPCK_ZeroInteger,
+
+    /// \brief Expression is a C++0X nullptr.
+    NPCK_CXX0X_nullptr,
+
+    /// \brief Expression is a GNU-style __null constant.
+    NPCK_GNUNull
+  };
+
   /// \brief Enumeration used to describe how \c isNullPointerConstant()
   /// should cope with value-dependent expressions.
   enum NullPointerConstantValueDependence {
@@ -452,11 +468,12 @@ public:
     NPC_ValueDependentIsNotNull
   };
   
-  /// isNullPointerConstant - C99 6.3.2.3p3 -  Return true if this is either an
-  /// integer constant expression with the value zero, or if this is one that is
-  /// cast to void*.
-  bool isNullPointerConstant(ASTContext &Ctx,
-                             NullPointerConstantValueDependence NPC) const;
+  /// isNullPointerConstant - C99 6.3.2.3p3 - Test if this reduces down to
+  /// a Null pointer constant. The return value can further distinguish the
+  /// kind of NULL pointer constant that was detected.
+  NullPointerConstantKind isNullPointerConstant(
+      ASTContext &Ctx,
+      NullPointerConstantValueDependence NPC) const;
 
   /// isOBJCGCCandidate - Return true if this expression may be used in a read/
   /// write barrier.
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 7b28463349..9592cbb36f 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3147,6 +3147,8 @@ def err_incomplete_type_used_in_type_trait_expr : Error<
   "incomplete type %0 used in type trait expression">;
 def err_expected_ident_or_lparen : Error<"expected identifier or '('">;
 
+def err_typecheck_cond_incompatible_operands_null : Error<
+  "non-pointer operand type %0 incompatible with %select{NULL|nullptr}1">;
 } // End of general sema category.
 
 // inline asm.
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 8d8cf09912..4e3173ab09 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -4774,6 +4774,9 @@ public:
   QualType FindCompositeObjCPointerType(Expr *&LHS, Expr *&RHS,
                                         SourceLocation questionLoc);
 
+  bool DiagnoseConditionalForNull(Expr *LHS, Expr *RHS,
+                                  SourceLocation QuestionLoc);
+
   /// type checking for vector binary operators.
   QualType CheckVectorOperands(SourceLocation l, Expr *&lex, Expr *&rex);
   QualType CheckVectorCompareOperands(Expr *&lex, Expr *&rx,
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index 7e46d4fe45..391b26ab48 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -2132,11 +2132,14 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef) const {
   return isEvaluatable(Ctx);
 }
 
-/// isNullPointerConstant - C99 6.3.2.3p3 -  Return true if this is either an
-/// integer constant expression with the value zero, or if this is one that is
-/// cast to void*.
-bool Expr::isNullPointerConstant(ASTContext &Ctx,
-                                 NullPointerConstantValueDependence NPC) const {
+/// isNullPointerConstant - C99 6.3.2.3p3 - Return whether this is a null 
+/// pointer constant or not, as well as the specific kind of constant detected.
+/// Null pointer constants can be integer constant expressions with the
+/// value zero, casts of zero to void*, nullptr (C++0X), or __null
+/// (a GNU extension).
+Expr::NullPointerConstantKind
+Expr::isNullPointerConstant(ASTContext &Ctx,
+                            NullPointerConstantValueDependence NPC) const {
   if (isValueDependent()) {
     switch (NPC) {
     case NPC_NeverValueDependent:
@@ -2144,10 +2147,13 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx,
       // If the unthinkable happens, fall through to the safest alternative.
         
     case NPC_ValueDependentIsNull:
-      return isTypeDependent() || getType()->isIntegralType(Ctx);
+      if (isTypeDependent() || getType()->isIntegralType(Ctx))
+        return NPCK_ZeroInteger;
+      else
+        return NPCK_NotNull;
         
     case NPC_ValueDependentIsNotNull:
-      return false;
+      return NPCK_NotNull;
     }
   }
 
@@ -2176,12 +2182,12 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx,
     return DefaultArg->getExpr()->isNullPointerConstant(Ctx, NPC);
   } else if (isa<GNUNullExpr>(this)) {
     // The GNU __null extension is always a null pointer constant.
-    return true;
+    return NPCK_GNUNull;
   }
 
   // C++0x nullptr_t is always a null pointer constant.
   if (getType()->isNullPtrType())
-    return true;
+    return NPCK_CXX0X_nullptr;
 
   if (const RecordType *UT = getType()->getAsUnionType())
     if (UT && UT->getDecl()->hasAttr<TransparentUnionAttr>())
@@ -2193,12 +2199,14 @@ bool Expr::isNullPointerConstant(ASTContext &Ctx,
   // This expression must be an integer type.
   if (!getType()->isIntegerType() || 
       (Ctx.getLangOptions().CPlusPlus && getType()->isEnumeralType()))
-    return false;
+    return NPCK_NotNull;
 
   // If we have an integer constant expression, we need to *evaluate* it and
   // test for the value 0.
   llvm::APSInt Result;
-  return isIntegerConstantExpr(Result, Ctx) && Result == 0;
+  bool IsNull = isIntegerConstantExpr(Result, Ctx) && Result == 0;
+
+  return (IsNull ? NPCK_ZeroInteger : NPCK_NotNull);
 }
 
 /// \brief If this expression is an l-value for an Objective C
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 7e6eee7fb7..792eb8af98 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -5229,6 +5229,46 @@ ExprResult Sema::ActOnParenOrParenListExpr(SourceLocation L,
   return Owned(expr);
 }
 
+/// \brief Emit a specialized diagnostic when one expression is a null pointer
+/// constant and the other is not a pointer.
+bool Sema::DiagnoseConditionalForNull(Expr *LHS, Expr *RHS,
+                                      SourceLocation QuestionLoc) {
+  Expr *NullExpr = LHS;
+  Expr *NonPointerExpr = RHS;
+  Expr::NullPointerConstantKind NullKind =
+      NullExpr->isNullPointerConstant(Context,
+                                      Expr::NPC_ValueDependentIsNotNull);
+
+  if (NullKind == Expr::NPCK_NotNull) {
+    NullExpr = RHS;
+    NonPointerExpr = LHS;
+    NullKind =
+        NullExpr->isNullPointerConstant(Context,
+                                        Expr::NPC_ValueDependentIsNotNull);
+  }
+
+  if (NullKind == Expr::NPCK_NotNull)
+    return false;
+
+  if (NullKind == Expr::NPCK_ZeroInteger) {
+    // In this case, check to make sure that we got here from a "NULL"
+    // string in the source code.
+    NullExpr = NullExpr->IgnoreParenImpCasts();
+    SourceManager& SM = Context.getSourceManager();
+    SourceLocation Loc = SM.getInstantiationLoc(NullExpr->getExprLoc());
+    unsigned Len =
+        Lexer::MeasureTokenLength(Loc, SM, Context.getLangOptions());
+    if (Len != 4 || memcmp(SM.getCharacterData(Loc), "NULL", 4))
+      return false;
+  }
+
+  int DiagType = (NullKind == Expr::NPCK_CXX0X_nullptr);
+  Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands_null)
+      << NonPointerExpr->getType() << DiagType
+      << NonPointerExpr->getSourceRange();
+  return true;
+}
+
 /// Note that lhs is not null here, even if this is the gnu "x ?: y" extension.
 /// In that case, lhs = cond.
 /// C99 6.5.15
@@ -5471,6 +5511,12 @@ QualType Sema::CheckConditionalOperands(Expr *&Cond, Expr *&LHS, Expr *&RHS,
     return LHSTy;
   }
 
+  // Emit a better diagnostic if one of the expressions is a null pointer
+  // constant and the other is not a pointer type. In this case, the user most
+  // likely forgot to take the address of the other expression.
+  if (DiagnoseConditionalForNull(LHS, RHS, QuestionLoc))
+    return QualType();
+
   // Otherwise, the operands are not compatible.
   Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands)
     << LHSTy << RHSTy << LHS->getSourceRange() << RHS->getSourceRange();
diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
index 44508540d0..b78e52303e 100644
--- a/lib/Sema/SemaExprCXX.cpp
+++ b/lib/Sema/SemaExprCXX.cpp
@@ -2874,13 +2874,14 @@ static bool TryClassUnification(Sema &Self, Expr *From, Expr *To,
 /// value operand is a class type, overload resolution is used to find a
 /// conversion to a common type.
 static bool FindConditionalOverload(Sema &Self, Expr *&LHS, Expr *&RHS,
-                                    SourceLocation Loc) {
+                                    SourceLocation QuestionLoc) {
   Expr *Args[2] = { LHS, RHS };
-  OverloadCandidateSet CandidateSet(Loc);
-  Self.AddBuiltinOperatorCandidates(OO_Conditional, Loc, Args, 2, CandidateSet);
+  OverloadCandidateSet CandidateSet(QuestionLoc);
+  Self.AddBuiltinOperatorCandidates(OO_Conditional, QuestionLoc, Args, 2,
+                                    CandidateSet);
 
   OverloadCandidateSet::iterator Best;
-  switch (CandidateSet.BestViableFunction(Self, Loc, Best)) {
+  switch (CandidateSet.BestViableFunction(Self, QuestionLoc, Best)) {
     case OR_Success:
       // We found a match. Perform the conversions on the arguments and move on.
       if (Self.PerformImplicitConversion(LHS, Best->BuiltinTypes.ParamTypes[0],
@@ -2891,13 +2892,20 @@ static bool FindConditionalOverload(Sema &Self, Expr *&LHS, Expr *&RHS,
       return false;
 
     case OR_No_Viable_Function:
-      Self.Diag(Loc, diag::err_typecheck_cond_incompatible_operands)
+
+      // Emit a better diagnostic if one of the expressions is a null pointer
+      // constant and the other is a pointer type. In this case, the user most
+      // likely forgot to take the address of the other expression.
+      if (Self.DiagnoseConditionalForNull(LHS, RHS, QuestionLoc))
+        return true;
+
+      Self.Diag(QuestionLoc, diag::err_typecheck_cond_incompatible_operands)
         << LHS->getType() << RHS->getType()
         << LHS->getSourceRange() << RHS->getSourceRange();
       return true;
 
     case OR_Ambiguous:
-      Self.Diag(Loc, diag::err_conditional_ambiguous_ovl)
+      Self.Diag(QuestionLoc, diag::err_conditional_ambiguous_ovl)
         << LHS->getType() << RHS->getType()
         << LHS->getSourceRange() << RHS->getSourceRange();
       // FIXME: Print the possible common types by printing the return types of
diff --git a/test/Sema/conditional-expr.c b/test/Sema/conditional-expr.c
index 6e248bc3cf..7a8c9e9f36 100644
--- a/test/Sema/conditional-expr.c
+++ b/test/Sema/conditional-expr.c
@@ -75,3 +75,16 @@ int f2(int x) {
   // We can suppress this because the immediate context wants an int.
   return (x != 0) ? 0U : x;
 }
+
+#define NULL (void*)0
+
+void PR9236() {
+  struct A {int i;} A1;
+  (void)(1 ? A1 : NULL); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}}
+  (void)(1 ? NULL : A1); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}}
+  (void)(1 ? 0 : A1); // expected-error{{incompatible operand types}}
+  (void)(1 ? (void*)0 : A1); // expected-error{{incompatible operand types}}
+  (void)(1 ? A1: (void*)0); // expected-error{{incompatible operand types}}
+  (void)(1 ? A1 : (NULL)); // expected-error{{non-pointer operand type 'struct A' incompatible with NULL}}
+}
+
diff --git a/test/SemaCXX/__null.cpp b/test/SemaCXX/__null.cpp
index 3583655134..1989a45fb5 100644
--- a/test/SemaCXX/__null.cpp
+++ b/test/SemaCXX/__null.cpp
@@ -12,3 +12,10 @@ void f() {
   // Verify that null is evaluated as 0.
   int b[__null ? -1 : 1];
 }
+
+struct A {};
+
+void g() {
+  (void)(0 ? __null : A()); // expected-error {{non-pointer operand type 'A' incompatible with NULL}}
+  (void)(0 ? A(): __null); // expected-error {{non-pointer operand type 'A' incompatible with NULL}}
+}
diff --git a/test/SemaCXX/conditional-expr.cpp b/test/SemaCXX/conditional-expr.cpp
index 8ac0a9736a..3881765cff 100644
--- a/test/SemaCXX/conditional-expr.cpp
+++ b/test/SemaCXX/conditional-expr.cpp
@@ -307,3 +307,15 @@ namespace PR7598 {
   }
 
 }
+
+namespace PR9236 {
+#define NULL 0L
+  void f() {
+    (void)(true ? A() : NULL); // expected-error{{non-pointer operand type 'A' incompatible with NULL}}
+    (void)(true ? NULL : A()); // expected-error{{non-pointer operand type 'A' incompatible with NULL}}
+    (void)(true ? 0 : A()); // expected-error{{incompatible operand types}}
+    (void)(true ? nullptr : A()); // expected-error{{non-pointer operand type 'A' incompatible with nullptr}}
+    (void)(true ? __null : A()); // expected-error{{non-pointer operand type 'A' incompatible with NULL}}
+    (void)(true ? (void*)0 : A()); // expected-error{{incompatible operand types}}
+  }
+}
diff --git a/test/SemaCXX/nullptr.cpp b/test/SemaCXX/nullptr.cpp
index 666701c3c6..7385fd42ad 100644
--- a/test/SemaCXX/nullptr.cpp
+++ b/test/SemaCXX/nullptr.cpp
@@ -46,6 +46,8 @@ nullptr_t f(nullptr_t null)
   (void)(1 + nullptr); // expected-error {{invalid operands to binary expression}}
   (void)(0 ? nullptr : 0); // expected-error {{incompatible operand types}}
   (void)(0 ? nullptr : (void*)0);
+  (void)(0 ? nullptr : A()); // expected-error {{non-pointer operand type 'A' incompatible with nullptr}}
+  (void)(0 ? A() : nullptr); // expected-error {{non-pointer operand type 'A' incompatible with nullptr}}
 
   // Overloading
   int t = o1(nullptr);