resolve: allow use of predeclared name prior to a global def (if AllowGlobalReassign)

[alandonovan]

This change causes the AllowGlobalReassign flag (used for legacy Bazel
semantics) to also allow use of predeclared names prior to a global
binding of the same name, as in:

print(len); len=1; print(len)

This effectively reverses github.com/google/skylark/pull/123 behind an option.

See github.com/google/skylark/issues/116 for discussion.

[laurentlb]

the Bazel implementation lags behind the spec

This should be fixed with my commit from yesterday: bazelbuild/bazel@3a979f7

To be released in Bazel 0.21 (end of December).

[alandonovan]

@laurentlb: Consistent though the Skylark spec may be in its treatment of globals and locals, disallowing the first print(len) in the example above creates a problem no smaller than the one it solves, and gratuitously different from Python. The restriction was motivated by the idea that a reader shouldn't have to remember where they are in the file to know which version of len they are referring to---in particular, they don't have to look up. But now they have to look down to see whether a global declaration follows.

I think we should revert the spec to follow Python. Happily this would require no code change to Bazel.

[jayconrod]

This change seems reasonable, as long as it's hidden behind AllowGlobalReassign.

I don't personally see a clear advantage to one behavior over the other. As specified, you have to look down to see whether there's an assignment. With the Python behavior, you have to look up, and you may not be able to tell statically whether an assignment was executed.

To reduce confusion, I think it would be better to follow the Python semantics unless there's good reason to deviate. What's the rationale for the current specified behavior?

[laurentlb]

In addition to being more consistent (same mechanism for local variables and global variables), it is important to be able to resolve symbols statically. For example, we need that for renaming symbols, code completion, jump to definition in an IDE, Kythe, linters and other static analyzers. This can also enable compiler optimizations.

The only place when the reassignment is useful is in the REPL.

[alandonovan]

@laurentlb, no-one disputes the value of static name resolution; that's not the issue here. The REPL is also irrelevant because each gob of input is actually a new file evaluated in an old environment.

The issue is that we can statically resolve the first len in print(len); len=1; print(len) in two different ways: the Python way, which resolves to the definition "above", that is, the predeclared one, or the Skylark-spec way, which resolves the definition "below".

The Python way is clearly useful. For example, you might want to define a local variant of a built-in (print, say), using the same name:

builtin_print = print
def print(x): ....builtin_print(x)...
...
print(...) # calls the user-defined one

There are plenty of other examples in BUILD files.

But the Skylark-spec way is literally never useful. It's a guaranteed dynamic error.

The Python way is static, inconsistent but familiar, and useful. The Skylark-spec way is static, consistent but surprising, and not useful. How is that better?

[laurentlb]

In the Python way, the meaning of the first len changes:

def foo(x):
  print(len(x))   # here

foo(2)

def len(x): return 5

foo(3)

This means that it's not possible to do renaming, code completion, etc. on len.

The Skylark-spec way is static, consistent but surprising, and not useful. How is that better?

The spec-way tells you to avoid reusing the same name with multiple meanings. Note that many (Python) linters will also discourage you from redefining len or print as in your example.

It's a guaranteed dynamic error.

An error is often better than something that silently does the wrong thing.

[alandonovan]

This means that it's not possible to do renaming, code completion, etc. on len.

No, it doesn't; it means that static analysis tools have to implement the same static semantics as the language.

The spec-way tells you to avoid reusing the same name with multiple meanings.

But sometimes that is exactly what the user wants to do, and now they cannot. I gave you one realistic example already, and I have encountered several others since we changed the spec. You keep asserting that this restriction enables static tooling, but that turns out not to be the case; it is perfectly feasible to build tools that understand the Python semantics.

What problem do the spec semantics actually solve?

An error is often better than something that silently does the wrong thing.

True, but an error is often worse than a program that silently does the right thing. Let's be specific.

[laurentlb]

To take a more precise example: I have a tool that can rename global symbols. It finds all references and renames them.

With the Python semantics, that means that it's not possible to rename the def len from the example above, because the reference above sometimes refers to the builtin len instead.

Being able to find the cross-references is an important feature.

[alandonovan]

I have a tool that can rename global symbols. It finds all references and renames them. With the Python semantics, that means that it's not possible to rename the def len from the example above, because the reference above sometimes refers to the builtin len instead.

No, it would mean the tool had a bug. If we implemented the Python semantics, the correct behavior of the tool would be to resolve all the identifiers then rename only those that refer to the global symbol. The first len would not be one of them, so the tool would leave it alone.

Being able to find the cross-references is an important feature.

I agree, but that's irrelevant.

[laurentlb]

I think there's a misunderstanding.

If you run this code with Python:

def foo():
  print(len("abc"))

foo()

def len(x): return x

foo()

you get:

3
abc

There's no way to rename the def len (and the cross-references) so that the behavior of the code remains the same.

[alandonovan]

Ah, I forgot that unlike Skylark-in-Go, Python doesn't statically discriminate global and predeclared symbols---they compile to the same operation, and it dynamically tries one then the other. This does indeed violate the property you care about for tooling.

The fully static semantics I was alluding to, which we have established are not those of Python, would cause the len in foo to be resolved as a reference to the predeclared len function. Every time foo is called, that's what you get, regardless of any later global definition of the name len.