Added a warning when referencing an if's condition variable in the

"else" clause, e.g.,

  if (int X = foo()) {
  } else {
    if (X) { // warning: X is always zero in this context
    }
  }

Fixes rdar://6425550 and lets me think about something other than
DeclContext.

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

diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h
index b4a1bf83da93c6b400f61f356e4d95cd5f0361c7..8fbb3f00e9ef69ea7db688736b873a0b26204796 100644 (file)
--- a/include/clang/AST/Decl.h
+++ b/include/clang/AST/Decl.h
@@ -331,7 +331,11 @@ private:
   unsigned SClass : 3;
   bool ThreadSpecified : 1;
   bool HasCXXDirectInit : 1; 
-  
+
+  /// DeclaredInCondition - Whether this variable was declared in a
+  /// condition, e.g., if (int x = foo()) { ... }.
+  bool DeclaredInCondition : 1;
+
   // Move to DeclGroup when it is implemented.
   SourceLocation TypeSpecStartLoc;
   friend class StmtIteratorBase;
@@ -341,7 +345,9 @@ protected:
           SourceLocation TSSL = SourceLocation())
     : ValueDecl(DK, DC, L, Id, T, PrevDecl), Init(0),
           ThreadSpecified(false), HasCXXDirectInit(false),
-          TypeSpecStartLoc(TSSL) { SClass = SC; }
+          DeclaredInCondition(false), TypeSpecStartLoc(TSSL) { 
+    SClass = SC; 
+  }
 public:
   static VarDecl *Create(ASTContext &C, DeclContext *DC,
                          SourceLocation L, IdentifierInfo *Id,
@@ -373,6 +379,18 @@ public:
     return HasCXXDirectInit;
   }
   
+  /// isDeclaredInCondition - Whether this variable was declared as
+  /// part of a condition in an if/switch/while statement, e.g.,
+  /// @code
+  /// if (int x = foo()) { ... }
+  /// @endcode
+  bool isDeclaredInCondition() const {
+    return DeclaredInCondition;
+  }
+  void setDeclaredInCondition(bool InCondition) { 
+    DeclaredInCondition = InCondition; 
+  }
+
   /// hasLocalStorage - Returns true if a variable with function scope
   ///  is a non-static local variable.
   bool hasLocalStorage() const {

diff --git a/include/clang/Basic/DiagnosticKinds.def b/include/clang/Basic/DiagnosticKinds.def
index 20337f0aed683e24b92f858ac6edaa826c756085..bba58791d1a0555703b46ff834d9ce7bcd4b279b 100644 (file)
--- a/include/clang/Basic/DiagnosticKinds.def
+++ b/include/clang/Basic/DiagnosticKinds.def
@@ -1294,6 +1294,10 @@ DIAG(err_invalid_declarator_in_function, ERROR,
 DIAG(err_not_tag_in_scope, ERROR,
      "%0 does not name a tag member in the specified scope")
 
+DIAG(warn_value_always_zero, WARNING,
+     "%0 is always zero in this context")
+DIAG(warn_value_always_false, WARNING,
+     "%0 is always false in this context")
 
 // assignment related diagnostics (also for argument passing, returning, etc).
 // FIXME: %2 is an english string here.

diff --git a/include/clang/Parse/Scope.h b/include/clang/Parse/Scope.h
index 5695fc0be19da8101ec010d1416526741f92b7b1..f8815ffd4f4fa985ba0f406185b8146e6f04fc86 100644 (file)
--- a/include/clang/Parse/Scope.h
+++ b/include/clang/Parse/Scope.h
@@ -75,6 +75,10 @@ private:
   /// interrelates with other control flow statements.
   unsigned Flags : 8;
   
+  /// WithinElse - Whether this scope is part of the "else" branch in
+  /// its parent ControlScope.
+  bool WithinElse : 1;
+
   /// FnParent - If this scope has a parent scope that is a function body, this
   /// pointer is non-null and points to it.  This is used for label processing.
   Scope *FnParent;
@@ -84,6 +88,11 @@ private:
   /// there is no containing break/continue scope.
   Scope *BreakParent, *ContinueParent;
 
+  /// ControlParent - This is a direct link to the immediately
+  /// preceeding ControlParent if this scope is not one, or null if
+  /// there is no containing control scope.
+  Scope *ControlParent;
+
   /// BlockParent - This is a direct link to the immediately containing
   /// BlockScope if this scope is not one, or null if there is none.
   Scope *BlockParent;
@@ -151,6 +160,9 @@ public:
     return const_cast<Scope*>(this)->getBreakParent();
   }
  
+  Scope *getControlParent() { return ControlParent; }
+  const Scope *getControlParent() const { return ControlParent; }
+  
   Scope *getBlockParent() { return BlockParent; }
   const Scope *getBlockParent() const { return BlockParent; }  
 
@@ -193,6 +205,12 @@ public:
     return getFlags() & Scope::TemplateParamScope;
   }
 
+  /// isWithinElse - Whether we are within the "else" of the
+  /// ControlParent (if any).
+  bool isWithinElse() const { return WithinElse; }
+
+  void setWithinElse(bool WE) { WithinElse = WE; }
+
   /// Init - This is used by the parser to implement scope caching.
   ///
   void Init(Scope *Parent, unsigned ScopeFlags) {
@@ -204,17 +222,23 @@ public:
       FnParent       = AnyParent->FnParent;
       BreakParent    = AnyParent->BreakParent;
       ContinueParent = AnyParent->ContinueParent;
+      ControlParent = AnyParent->ControlParent;
       BlockParent  = AnyParent->BlockParent;
       TemplateParamParent = AnyParent->TemplateParamParent;
+      WithinElse = AnyParent->WithinElse;
+
     } else {
       FnParent = BreakParent = ContinueParent = BlockParent = 0;
+      ControlParent = 0;
       TemplateParamParent = 0;
+      WithinElse = false;
     }
     
     // If this scope is a function or contains breaks/continues, remember it.
     if (Flags & FnScope)                   FnParent = this;
     if (Flags & BreakScope)                BreakParent = this;
     if (Flags & ContinueScope)             ContinueParent = this;
+    if (Flags & ControlScope)       ControlParent = this;
     if (Flags & BlockScope)                BlockParent = this;
     if (Flags & TemplateParamScope) TemplateParamParent = this;
     DeclsInScope.clear();

diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
index 8b5277bc365360a4bcb29d721c3acf5b404c9084..cb35740465a5e2231460ed86e70208d0581a8703 100644 (file)
--- a/lib/Parse/ParseStmt.cpp
+++ b/lib/Parse/ParseStmt.cpp
@@ -509,8 +509,11 @@ Parser::StmtResult Parser::ParseIfStatement() {
     ParseScope InnerScope(this, Scope::DeclScope, 
                           C99orCXX && Tok.isNot(tok::l_brace));
 
+    bool WithinElse = CurScope->isWithinElse();
+    CurScope->setWithinElse(true);
     ElseStmtLoc = Tok.getLocation();
     ElseStmt = ParseStatement();
+    CurScope->setWithinElse(WithinElse);
 
     // Pop the 'else' scope if needed.
     InnerScope.Exit();

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 8cbbca3fbf0b9d5183920eb00bbe78a694b95c57..3870f0736ec22df8982a444a0dea74c831cbe7e0 100644 (file)
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -483,6 +483,27 @@ Sema::ExprResult Sema::ActOnDeclarationNameExpr(Scope *S, SourceLocation Loc,
   // check if referencing an identifier with __attribute__((deprecated)).
   if (VD->getAttr<DeprecatedAttr>())
     Diag(Loc, diag::warn_deprecated) << VD->getDeclName();
+  
+  if (VarDecl *Var = dyn_cast<VarDecl>(VD)) {
+    if (Var->isDeclaredInCondition() && Var->getType()->isScalarType()) {
+      Scope *CheckS = S;
+      while (CheckS) {
+        if (CheckS->isWithinElse() && 
+            CheckS->getControlParent()->isDeclScope(Var)) {
+          if (Var->getType()->isBooleanType())
+            Diag(Loc, diag::warn_value_always_false) << Var->getDeclName();
+          else
+            Diag(Loc, diag::warn_value_always_zero) << Var->getDeclName();
+          break;
+        }
+
+        // Move up one more control parent to check again.
+        CheckS = CheckS->getControlParent();
+        if (CheckS)
+          CheckS = CheckS->getParent();
+      }
+    }
+  }
 
   // Only create DeclRefExpr's for valid Decl's.
   if (VD->isInvalidDecl())

diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
index c4cab3316ba6ea0f480962165dc89c219bc5ae28..27b0ba34d19dc019d37c6880026d303626e6613c 100644 (file)
--- a/lib/Sema/SemaExprCXX.cpp
+++ b/lib/Sema/SemaExprCXX.cpp
@@ -648,6 +648,10 @@ Sema::ActOnCXXConditionDeclarationExpr(Scope *S, SourceLocation StartLoc,
     return true;
   AddInitializerToDecl(Dcl, AssignExprVal);
 
+  // Mark this variable as one that is declared within a conditional.
+  if (VarDecl *VD = dyn_cast<VarDecl>((Decl *)Dcl))
+    VD->setDeclaredInCondition(true);
+
   return new CXXConditionDeclExpr(StartLoc, EqualLoc,
                                        cast<VarDecl>(static_cast<Decl *>(Dcl)));
 }

diff --git a/test/SemaCXX/warn-for-var-in-else.cpp b/test/SemaCXX/warn-for-var-in-else.cpp
new file mode 100644 (file)
index 0000000..dfbf35d
--- /dev/null
+++ b/test/SemaCXX/warn-for-var-in-else.cpp
@@ -0,0 +1,30 @@
+// RUN: clang -fsyntax-only -verify %s
+// rdar://6425550
+int bar();
+void do_something(int);
+
+int foo() {
+  if (int X = bar()) {
+    return X;
+  } else {
+    do_something(X); // expected-warning{{'X' is always zero in this context}}
+  }
+}
+
+bool foo2() {
+  if (bool B = bar()) {
+    if (int Y = bar()) {
+      return B;
+    } else {
+      do_something(Y); // expected-warning{{'Y' is always zero in this context}}
+      return B;
+    }
+  } else {
+    if (bool B2 = B) { // expected-warning{{'B' is always false in this context}}
+      do_something(B); // expected-warning{{'B' is always false in this context}}
+    } else if (B2) {  // expected-warning{{'B2' is always false in this context}}
+      do_something(B); // expected-warning{{'B' is always false in this context}}
+    }
+    return B; // expected-warning{{'B' is always false in this context}}
+  }
+}