Fix list of used environment variables

With the introduction of deprecated / obsolete variable support, we
started calling LookupVarInLocalScope on the variable we were setting
with = or :=. This was fine, except it also marked those variables as
used environment variables (whether they were set in the environment or
not). So changing one of these environment variables would cause kati to
regenerate the ninja file even though nothing would change.

To fix this, add new Peek* functions that don't record the variables as
used, but can still be used to check to see if the variable has been
deprecated or is obsolete.

7 files changed, 57 insertions, 5 deletions

diff --git a/eval.cc b/eval.cc
index 2206730..8cd7afd 100644
--- a/eval.cc
+++ b/eval.cc
@@ -72,20 +72,23 @@ Var* Evaluator::EvalRHS(Symbol lhs,
                                                       : VarOrigin::FILE));
 
   Var* rhs = NULL;
-  Var* prev = LookupVarInCurrentScope(lhs);
+  Var* prev = NULL;
   bool needs_assign = true;
 
   switch (op) {
     case AssignOp::COLON_EQ: {
+      prev = PeekVarInCurrentScope(lhs);
       SimpleVar* sv = new SimpleVar(origin);
       rhs_v->Eval(this, sv->mutable_value());
       rhs = sv;
       break;
     }
     case AssignOp::EQ:
+      prev = PeekVarInCurrentScope(lhs);
       rhs = new RecursiveVar(rhs_v, origin, orig_rhs);
       break;
     case AssignOp::PLUS_EQ: {
+      prev = LookupVarInCurrentScope(lhs);
       if (!prev->IsDefined()) {
         rhs = new RecursiveVar(rhs_v, origin, orig_rhs);
       } else if (prev->ReadOnly()) {
@@ -99,6 +102,7 @@ Var* Evaluator::EvalRHS(Symbol lhs,
       break;
     }
     case AssignOp::QUESTION_EQ: {
+      prev = LookupVarInCurrentScope(lhs);
       if (!prev->IsDefined()) {
         rhs = new RecursiveVar(rhs_v, origin, orig_rhs);
       } else {
@@ -109,10 +113,12 @@ Var* Evaluator::EvalRHS(Symbol lhs,
     }
   }
 
-  prev->Used(this, lhs);
-  if (prev->Deprecated()) {
-    if (needs_assign) {
-      rhs->SetDeprecated(prev->DeprecatedMessage());
+  if (prev != NULL) {
+    prev->Used(this, lhs);
+    if (prev->Deprecated()) {
+      if (needs_assign) {
+        rhs->SetDeprecated(prev->DeprecatedMessage());
+      }
     }
   }
 
@@ -392,6 +398,13 @@ Var* Evaluator::LookupVarInCurrentScope(Symbol name) {
   return LookupVarGlobal(name);
 }
 
+Var* Evaluator::PeekVarInCurrentScope(Symbol name) {
+  if (current_scope_) {
+    return current_scope_->Peek(name);
+  }
+  return name.PeekGlobalVar();
+}
+
 string Evaluator::EvalVar(Symbol name) {
   return LookupVar(name)->Eval(this);
 }
diff --git a/eval.h b/eval.h
index 2b62892..1dff4b7 100644
--- a/eval.h
+++ b/eval.h
@@ -105,6 +105,9 @@ class Evaluator {
 
   Var* LookupVarGlobal(Symbol name);
 
+  // Equivalent to LookupVarInCurrentScope, but doesn't mark as used.
+  Var* PeekVarInCurrentScope(Symbol name);
+
   unordered_map<Symbol, Vars*> rule_vars_;
   vector<const Rule*> rules_;
   unordered_map<Symbol, bool> exports_;
diff --git a/symtab.cc b/symtab.cc
index 0640ea3..b25e4d6 100644
--- a/symtab.cc
+++ b/symtab.cc
@@ -43,6 +43,13 @@ Symbol kShellSym = Symbol(Symbol::IsUninitialized());
 
 Symbol::Symbol(int v) : v_(v) {}
 
+Var* Symbol::PeekGlobalVar() const {
+  if (static_cast<size_t>(v_) >= g_symbol_data.size()) {
+    return kUndefined;
+  }
+  return g_symbol_data[v_].gv;
+}
+
 Var* Symbol::GetGlobalVar() const {
   if (static_cast<size_t>(v_) >= g_symbol_data.size()) {
     g_symbol_data.resize(v_ + 1);
diff --git a/symtab.h b/symtab.h
index 0469b65..e9788cf 100644
--- a/symtab.h
+++ b/symtab.h
@@ -49,6 +49,7 @@ class Symbol {
 
   bool IsValid() const { return v_ >= 0; }
 
+  Var* PeekGlobalVar() const;
   Var* GetGlobalVar() const;
   void SetGlobalVar(Var* v,
                     bool is_override = false,
diff --git a/testcase/ninja_regen.sh b/testcase/ninja_regen.sh
index 7f9b35e..5a1c0ff 100755
--- a/testcase/ninja_regen.sh
+++ b/testcase/ninja_regen.sh
@@ -39,10 +39,12 @@ fi
 
 sleep_if_necessary 1
 cat <<EOF > Makefile
+VAR3 := unused
 all:
 	echo bar
 	echo VAR=\$(VAR)
 	echo VAR2=\$(VAR2)
+	echo VAR3=\$(VAR3)
 	echo wildcard=\$(wildcard *.mk)
 other:
 	echo foo
@@ -74,6 +76,24 @@ if [ -e ninja.sh ]; then
   ./ninja.sh
 fi
 
+export VAR3=testing
+${mk} 2> ${log}
+if [ -e ninja.sh ]; then
+  if grep regenerating ${log} >/dev/null; then
+    echo 'Should not regenerate (unused env added)'
+  fi
+  ./ninja.sh
+fi
+
+export VAR3=test2
+${mk} 2> ${log}
+if [ -e ninja.sh ]; then
+  if grep regenerating ${log} >/dev/null; then
+    echo 'Should not regenerate (unused env changed)'
+  fi
+  ./ninja.sh
+fi
+
 export PATH=/random_path:$PATH
 ${mk} 2> ${log}
 if [ -e ninja.sh ]; then
diff --git a/var.cc b/var.cc
index 14ccee6..7485c90 100644
--- a/var.cc
+++ b/var.cc
@@ -136,6 +136,13 @@ Var* Vars::Lookup(Symbol name) const {
   return v;
 }
 
+Var* Vars::Peek(Symbol name) const {
+  auto found = find(name);
+  if (found == end())
+    return kUndefined;
+  return found->second;
+}
+
 void Vars::Assign(Symbol name, Var* v, bool* readonly) {
   *readonly = false;
   auto p = emplace(name, v);
diff --git a/var.h b/var.h
index 47d80c1..be6363f 100644
--- a/var.h
+++ b/var.h
@@ -190,6 +190,7 @@ class Vars : public unordered_map<Symbol, Var*> {
   ~Vars();
 
   Var* Lookup(Symbol name) const;
+  Var* Peek(Symbol name) const;
 
   void Assign(Symbol name, Var* v, bool* readonly);