Skip to content

Consider --allow-redefinition for mypy #3832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mpharrigan opened this issue Feb 18, 2021 · 5 comments
Closed

Consider --allow-redefinition for mypy #3832

mpharrigan opened this issue Feb 18, 2021 · 5 comments
Labels
area/mypy kind/health For CI/testing/release process/refactoring/technical debt items triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@mpharrigan
Copy link
Collaborator

See python/mypy#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: python/mypy#6197 which may or may not become the default at some point. I say we add this to our mypy configuration.

@mpharrigan mpharrigan added the kind/health For CI/testing/release process/refactoring/technical debt items label Feb 18, 2021
@balopat
Copy link
Contributor

balopat commented Feb 18, 2021

ref #3767

@maffoo
Copy link
Contributor

maffoo commented Feb 18, 2021

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.

@viathor viathor added area/mypy triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Mar 16, 2021
@mpharrigan
Copy link
Collaborator Author

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)

@95-martin-orion 95-martin-orion added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Apr 28, 2021
@95-martin-orion
Copy link
Collaborator

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.

@mhucka
Copy link
Contributor

mhucka commented Mar 27, 2025

This is a tough call, because there are pros and cons for both options (add, or don't add). Based on the fact that (a) the default of not adding --allow-redefinition does not seem to have been problematic enough for people to need to change it in the last 4 years, (b) the option can be added manually on the command line to check/mypy if/when desired, and (c) it seems safer to leave this option off, I'm going ahead and closing this issue. If that's the wrong call and people want to revisit this, please feel free to leave comments here and reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/mypy kind/health For CI/testing/release process/refactoring/technical debt items triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
Status: Done
Development

No branches or pull requests

8 participants