Skip to content

Now TypeVar with values correctly handles is_subtype check #11378

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
wants to merge 2 commits into from
Closed

Now TypeVar with values correctly handles is_subtype check #11378

wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

pip (https://github.com/pypa/pip.git)
+ src/pip/_internal/req/req_uninstall.py:186: error: unused "type: ignore" comment

@sobolevn
Copy link
Member Author

sobolevn commented Nov 4, 2021

It was successfully tested by the OP: python/typing#909 (reply in thread)

Thanks - I got a chance to move to master tonight. I ran into a few incompatibilities from 0.910 related to some definitions in hashlib.pyi and logger.pyi, but I was able to sort them out without too much trouble.

I can confirm that the PR in #11378 eliminates the errors I was previously seeing in the example listed above.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Unfortunately, I don't think that this approach works.

else:
return False
if isinstance(right, TypeVarType) and right.values:
return any(self._is_subtype(left, r_val) for r_val in right.values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the correct fix. Now this invalid code generates no error:

from typing import AnyStr, Generic

class Foo(Generic[AnyStr]):
    def method1(self, s: AnyStr, t: AnyStr) -> None:
        print(s)

class Bar(Foo[AnyStr]):
    def method1(self, s: AnyStr, t: AnyStr) -> None:
        print('Child before')
        super().method1('x', b'y')  # Should be an error
        print('Child after')

We type check the method by substituting all instances of AnyStr in the method first with str, and later with bytes. Currently we don't perform the substitution in SuperExpr, which seems to be the root cause of the problem.

SuperExpr should perhaps be modified to support type variable substitution somehow. For example, we could add a list of (type variable, substitution) tuples as an attribute of SuperExpr that are initially empty, but as substitutions are performed, we'd add an item to the list. When inferring the type of SuperExpr, we'd perform these substitutions. This way the type of super() could effectively be treated as Foo[str] or Foo[bytes] instead of Foo[AnyStr], which should make the original error go away.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I see 😞

I will refactor this one to expand SuperExpr() 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

One more idea: we can represent method1 as an @overload of two functions:

  • def method1(self, s: str, t: str) -> None:
  • def method1(self, s: bytes, t: bytes) -> None:

So, this will help us to represent correct argument combinations.
What do you think?

@sobolevn sobolevn marked this pull request as draft November 7, 2021 07:50
hauntsaninja added a commit to hauntsaninja/mypy that referenced this pull request Jul 28, 2022
sobolevn pushed a commit that referenced this pull request Jul 28, 2022
This pull request was closed.
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.

2 participants