Skip to content
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

UP006: Incorrect autofix when shadowing builtin #3284

Closed
jdahlin opened this issue Feb 28, 2023 · 7 comments · Fixed by #3286
Closed

UP006: Incorrect autofix when shadowing builtin #3284

jdahlin opened this issue Feb 28, 2023 · 7 comments · Fixed by #3286
Labels
bug Something isn't working

Comments

@jdahlin
Copy link

jdahlin commented Feb 28, 2023

The autofix for UP006 is not working correctly when shadowing a builtin such as type.

Test case

from typing import Type

class F:
    type = "abc"
    def foo(self) -> Type[list]:
        return list

Expected code after autofix:

import builtins


class F:
    type = "abc"
    def foo() -> builtins.type[list]:
        return list

Actual code after autofix:

class F:
    type = "abc"
    def foo() -> type[list]:
        return list
@charliermarsh charliermarsh added the bug Something isn't working label Feb 28, 2023
@charliermarsh
Copy link
Member

Thanks, you're right! These should check for redefined builtins.

@jdahlin
Copy link
Author

jdahlin commented Mar 1, 2023

Wow, thanks for the quick fix! Should I open up a new issue to handle the case when the builtins are overriden/shadowed?

@charliermarsh
Copy link
Member

What do you think the ideal behavior would be in that case?

@jdahlin
Copy link
Author

jdahlin commented Mar 1, 2023

What do you think the ideal behavior would be in that case?

To fetch the original object via the builtin module, e.g:

import builtins


class F:
    type = "abc"
    def foo() -> builtins.type[list]:
        return list

@charliermarsh
Copy link
Member

Yeah we can open an issue for that. It will be tricky to do right now because fixes can't introduce new imports. So for now, we could support this in the event that a module already imports builtins, but we couldn't "import builtins and change the reference". (Part of #835.)

@jdahlin
Copy link
Author

jdahlin commented Mar 2, 2023

@charliermarsh original builtins can also be found in __builtins__ which is always present in the current globals.

@charliermarsh
Copy link
Member

@jdahlin - True! Although that can also be overridden 😂 Feel free to open an issue for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants