Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Consider `--allow-redefinition` for mypy

Open mpharrigan opened this issue 4 years ago • 4 comments

See https://github.com/python/mypy/issues/1174 for full context, but there are common idioms where you may want to re-use a variable name

items: Set[Item] = set()
items.add(...)
items = frozenset(items)

Mypy currently complains. They added a flag: https://github.com/python/mypy/pull/6197 which may or may not become the default at some point. I say we add this to our mypy configuration.

mpharrigan avatar Feb 18 '21 17:02 mpharrigan

ref https://github.com/quantumlib/Cirq/issues/3767

balopat avatar Feb 18 '21 17:02 balopat

I have mixed feelings about this. Sometimes it's nice to redefine variables (I often encounter this when a function accepts an arg with a union type but we then convert it to a single type and want the rest of the code to use this "narrowed" type). But other times a redefinition is inadvertent and mypy flagging it can actually detect bugs (sometimes bugs like this only surface when refactoring). It looks like --allow-redefinition only applies in certain limited situations, which is good, but I'd still be a bit leery about adding it.

maffoo avatar Feb 18 '21 18:02 maffoo

I'm going through the numpy stuff and there are a non-zero number of places where redefinitions are used, especially when constructing arrays:

arr: List[float]
for thing in iterator:
  arr.append(complicated_function(thing))
arr: np.ndarray = np.asarray(arr)

mpharrigan avatar Apr 21 '21 19:04 mpharrigan

Discussion in Cirq sync:

  • particularly nice for e.g. redefining Optional values to non-optional type
  • not allowing redefinition enforces cleaner code, but mostly helps with redefinition in multiple scopes

From mypy: --allow_redefinition only allows redefinition within the same block and nesting depth.

95-martin-orion avatar Apr 28 '21 18:04 95-martin-orion