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

Fix callable instance variable support #10548

Merged
merged 3 commits into from
Jun 13, 2021

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented May 28, 2021

Fixes #708 and Fixes #5485

Prevent handling as bounded method of callable members declared as instance variables.

@wyfo wyfo marked this pull request as draft May 28, 2021 23:26
@wyfo wyfo marked this pull request as ready for review May 28, 2021 23:26
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

This is great! It's a very long-lasting bug so it's wonderful to get it fixed. The mypy-primer output looks very promising. https://github.com/willmcgugan/rich/blob/master/rich/pager.py#L20 seems like it's missing a ClassVar.

@wyfo
Copy link
Contributor Author

wyfo commented May 29, 2021

Yes, mypy-primer looks very promising! (I'm also excited, thinking about all the # type: ignore I will remove from my code)
ClassVar would also be missing here https://github.com/sphinx-doc/sphinx/blob/830b3fbe2babcc8df33f767ce3a406b16c0cac1c/sphinx/domains/std.py#L54

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/validators.py:503: error: unused "type: ignore" comment
+ pydantic/main.py:556: error: Redundant cast to "Callable[[Any], Any]"  [redundant-cast]
+ pydantic/dataclasses.py:172: error: unused "type: ignore" comment
+ pydantic/dataclasses.py:173: error: unused "type: ignore" comment

sphinx (https://github.com/sphinx-doc/sphinx.git)
+ sphinx/domains/std.py: note: In member "handle_signature" of class "GenericObject":
+ sphinx/domains/std.py:58:20: error: Too few arguments
+ sphinx/domains/std.py:58:36: error: Argument 1 has incompatible type "BuildEnvironment"; expected "GenericObject"
+ sphinx/domains/std.py:58:46: error: Argument 2 has incompatible type "str"; expected "BuildEnvironment"
+ sphinx/domains/std.py:58:51: error: Argument 3 has incompatible type "desc_signature"; expected "str"

rich (https://github.com/willmcgugan/rich.git)
+ rich/pager.py:26: error: Too few arguments

graphql-core (https://github.com/graphql-python/graphql-core.git)
+ src/graphql/execution/execute.py:207: error: unused "type: ignore" comment
+ src/graphql/execution/execute.py:208: error: unused "type: ignore" comment
+ src/graphql/execution/execute.py:879: error: unused "type: ignore" comment

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thank you!

@msullivan msullivan merged commit 6ab0efc into python:master Jun 13, 2021
johtso added a commit to johtso/httpx-caching that referenced this pull request Jun 20, 2021
Currently bumping into a bug with py3.6
python/mypy#10548
@hauntsaninja
Copy link
Collaborator

Just wanted to say thank you again! This fixes a longstanding issue!

JukkaL added a commit that referenced this pull request Nov 17, 2021
This reverts commit 6ab0efc.

Reverting the change since it causes a significant backward
compatibility break. Code like this previously worked as expected:

```
class C:
    def f(self) -> None: pass

    g = f

C().g()
```

However, #10548 broke this in a subtle way (no error on definition),
and instead required the introduction of a ClassVar annotation for
`g`, which is non-intuitive and error-prone. For example, some
typeshed stubs use method aliases such as the above (e.g. stubs for
`logging`), and the change broke those stubs. It's also arguably
inconsistent, since normally ClassVar annotations are optional.

Any fix to the original issue should avoid breaking method aliases
such as the above. Hopefully the fix can be adjusted suitably.

The PR may have broken incremental mode somehow as well -- cached
and uncached runs sometimes produce different results. The root
cause is still unclear, however.
JukkaL added a commit that referenced this pull request Nov 17, 2021
This reverts commit 6ab0efc.

Reverting the change since it causes a significant backward
compatibility break. Code like this previously worked as expected:

```
class C:
    def f(self, ...) -> None: pass

    g = f

C().g(...)  # Now sometimes generates an error
```

However, #10548 broke this in a subtle way (no error on definition,
it sometimes generates an error on call), and instead required the 
introduction of a ClassVar annotation for `g`, which is non-intuitive 
and error-prone. For example, some typeshed stubs use method 
aliases such as the above (e.g. stubs for `logging`), and the change 
broke those stubs. It's also arguably inconsistent, since normally 
ClassVar annotations are optional.

Any fix to the original issue should avoid breaking method aliases
such as the above. Hopefully the fix can be adjusted suitably.

The PR may have broken incremental mode somehow as well -- cached
and uncached runs sometimes produce different results. The root
cause is still unclear, however.
JukkaL added a commit that referenced this pull request Nov 17, 2021
This reverts commit 6ab0efc.

Reverting the change since it causes a significant backward
compatibility break. Code like this previously worked as expected:

```
class C:
    def f(self, ...) -> None: pass

    g = f

C().g(...)  # Now sometimes generates an error
```

However, #10548 broke this in a subtle way (no error on definition,
it sometimes generates an error on call), and instead required the
introduction of a ClassVar annotation for `g`, which is non-intuitive
and error-prone. For example, some typeshed stubs use method
aliases such as the above (e.g. stubs for `logging`), and the change
broke those stubs. It's also arguably inconsistent, since normally
ClassVar annotations are optional.

Any fix to the original issue should avoid breaking method aliases
such as the above. Hopefully the fix can be adjusted suitably.

The PR may have broken incremental mode somehow as well -- cached
and uncached runs sometimes produce different results. The root
cause is still unclear, however.
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
…n#11571)

This reverts commit 6ab0efc.

Reverting the change since it causes a significant backward
compatibility break. Code like this previously worked as expected:

```
class C:
    def f(self, ...) -> None: pass

    g = f

C().g(...)  # Now sometimes generates an error
```

However, python#10548 broke this in a subtle way (no error on definition,
it sometimes generates an error on call), and instead required the 
introduction of a ClassVar annotation for `g`, which is non-intuitive 
and error-prone. For example, some typeshed stubs use method 
aliases such as the above (e.g. stubs for `logging`), and the change 
broke those stubs. It's also arguably inconsistent, since normally 
ClassVar annotations are optional.

Any fix to the original issue should avoid breaking method aliases
such as the above. Hopefully the fix can be adjusted suitably.

The PR may have broken incremental mode somehow as well -- cached
and uncached runs sometimes produce different results. The root
cause is still unclear, however.
ilevkivskyi pushed a commit to ilevkivskyi/mypy that referenced this pull request Aug 12, 2022
Fixes python#708 and Fixes python#5485

Prevent handling as bounded method of callable members declared as instance variables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class with function fields incorrectly thinks first argument is self Cannot assign to field of Callable type
4 participants