BindingEnv elision in manifest parser breaks evaluation order

[rprichard]

I was studying the BindingEnv::LookupWithFallback function today, and I noticed some curious behavior:

rule cc
  command = echo cc
build test1: cc
build test2: cc
  not_used = blank
command = echo parent

Output:

$ ninja test1
[1/1] echo parent
parent
$ ninja test2
[1/1] echo cc
cc

Edge lookups should happen in this order:

  /// This is tricky.  Edges want lookup scope to go in this order:
  /// 1) value set on edge itself (edge_->env_)
  /// 2) value set on rule, with expansion in the edge's scope
  /// 3) value set on enclosing scope of edge (edge_->env_->parent_)
  /// This function takes as parameters the necessary info to do (2).

However, there's an optimization in the manifest parser that's changing the behavior:

  // Bindings on edges are rare, so allocate per-edge envs only when needed.
  bool has_indent_token = lexer_.PeekToken(Lexer::INDENT);
  BindingEnv* env = has_indent_token ? new BindingEnv(env_) : env_;
  while (has_indent_token) {
    ...
    env->AddBinding(key, val.Evaluate(env_));
    ...
  }

  Edge* edge = state_->AddEdge(rule);
  edge->env_ = env;

If the edge has no bindings, then edge->env_ is actually its enclosing scope, which changes the lookup order to something like:

• value set directly on the edge's enclosing scope
• value set on the rule, with expansion in the edge's scope
• value set on the edge's grandparent scope

[michaelforney]

I'm curious about how this will be resolved so I can match the behavior in my ninja clone.

In samurai, I was previously looking up variables in the edge, then parent, then grandparent, then rule, which matches ninja's behavior when there are no edge bindings and no relevant grandparent bindings.

As of the above commit, I'm now using the documented evaluation rules, but I'm worried there might be projects out there that rely on the quirky behavior to build correctly.