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

Process NameTuple and fix Union types using | #13788

Closed
wants to merge 2 commits into from

Conversation

matejsp
Copy link

@matejsp matejsp commented Oct 1, 2022

SubdivisionEntity = typing.NamedTuple(
    'SubdivisionEntity',
    [
        ('subdivision', str),
        ('entity', str),
    ],
)

from

SubdivisionEntity = Incomplete

to

class SubdivisionAffiliation(NamedTuple):
    subdivision: str
    entity: str

and small fix for: #12929 fixing missing Union import

[case testImportUnion]
def func(a: str | int) -> str | int:
  pass

[out]
from typing import Union

def func(a: Union[str, int]) -> Union[str, int]: ...

@@ -595,6 +606,17 @@ class X(NamedTuple):
a: Incomplete
b: Incomplete

[case testNamedtupleWithTypes]
import collections, x
X = typing.NamedTuple('X', [('a', str), ('b', str)])
Copy link
Member

Choose a reason for hiding this comment

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

Can you please also test a case like X = typing.NamedTuple('X', [('@', int)])?

Copy link
Member

Choose a reason for hiding this comment

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

@sobolevn, won't that fail at runtime?

Copy link
Author

Choose a reason for hiding this comment

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

I will fail at runtime:
E ValueError: Type names and field names must be valid identifiers: '@'

I have added isidentifier validation.

isinstance(callee, MemberExpr) and callee.name == "namedtuple"
return (
isinstance(callee, NameExpr)
and (callee.name.endswith("namedtuple") or callee.name.endswith("NamedTuple"))
Copy link
Member

Choose a reason for hiding this comment

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

Would this match something like notanamedtuple?

Copy link
Author

@matejsp matejsp Oct 2, 2022

Choose a reason for hiding this comment

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

Yes I don't know what mypy support for "namedtuple" is, but I wanted the same behaviour for Namedtuple (NameExpr).

Original code was:

        return (isinstance(callee, NameExpr) and callee.name.endswith("namedtuple")) or (
            isinstance(callee, MemberExpr) and callee.name == "namedtuple"

I just added NamedTuple :)

collections.namedtuple vs typing.NamedTuple.

It seems that NameExpr can be qualified name:

self.name = name  # Name referred to (may be qualified)

Copy link
Member

Choose a reason for hiding this comment

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

Both NameExpr and MemberExpr have .fullname attribute. Maybe it is better to use it instead?

Copy link
Author

Choose a reason for hiding this comment

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

Hm I tried ... but fullname is resolved only when running stubgen from cmd line but not when running unit tests (it is None) and all namedtuple tests failing. Any idea?

Copy link
Author

Choose a reason for hiding this comment

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

So for fullname I need _semanal tests otherwise parseonly is enabled for unit tests. I can switch to semanal for namedtuple tests but it seems excessive. Thoughts?

isinstance(callee, MemberExpr) and callee.name == "namedtuple"
return (
isinstance(callee, NameExpr)
and (callee.name.endswith("namedtuple") or callee.name.endswith("NamedTuple"))
Copy link
Member

Choose a reason for hiding this comment

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

Both NameExpr and MemberExpr have .fullname attribute. Maybe it is better to use it instead?

@@ -595,6 +606,33 @@ class X(NamedTuple):
a: Incomplete
b: Incomplete

[case testNamedtupleUsingInvalidIdent]
import collections, x
X = collections.namedtuple('X', ['@'])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that it is correct. It just drops a field that is present in the original code.

I think a proper way to fix is to use X = collections.namedtuple('X', ['@']) in stubs.

There are several typeshed examples where we actually do this.

Copy link
Author

Choose a reason for hiding this comment

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

Generating X = collections.namedtuple('X', ['@', 'a', 'b', 'c']) in the stubs would create invalid stub code.
Well imho I prefer partial correctness (and correct code), since we could resolve other fields (a, b, c in my case) and skip incorrect one.

hamdanal added a commit to hamdanal/mypy that referenced this pull request Feb 11, 2023
Fixes python#9901
Fixes python#13662

Fix inheriting from a call-based `collections.namedtuple` /
`typing.NamedTuple` definition that was omitted from the generated stub.

This automatically adds support for the call-based `NamedTuple` in
general not only as a based class (Closes python#13788).

<details>
<summary>An example before and after</summary>
Input:
```python
import collections
import typing
from collections import namedtuple
from typing import NamedTuple

CollectionsCall = namedtuple("CollectionsCall", ["x", "y"])

class CollectionsClass(namedtuple("CollectionsClass", ["x", "y"])):
    def f(self, a):
        pass

class CollectionsDotClass(collections.namedtuple("CollectionsClass", ["x", "y"])):
    def f(self, a):
        pass

TypingCall = NamedTuple("TypingCall", [("x", int | None), ("y", int)])

class TypingClass(NamedTuple):
    x: int | None
    y: str

    def f(self, a):
        pass

class TypingClassWeird(NamedTuple("TypingClassWeird", [("x", int | None), ("y", str)])):
    z: float | None

    def f(self, a):
        pass

class TypingDotClassWeird(typing.NamedTuple("TypingClassWeird", [("x", int | None), ("y", str)])):
    def f(self, a):
        pass
```

Output diff (before and after):
```diff
diff --git a/before.pyi b/after.pyi
index c88530e2c..95ef843b4 100644
--- a/before.pyi
+++ b/after.pyi
@@ -1,26 +1,29 @@
+import typing
 from _typeshed import Incomplete
 from typing_extensions import NamedTuple

 class CollectionsCall(NamedTuple):
     x: Incomplete
     y: Incomplete

-class CollectionsClass:
+class CollectionsClass(NamedTuple('CollectionsClass', [('x', Incomplete), ('y', Incomplete)])):
     def f(self, a) -> None: ...

-class CollectionsDotClass:
+class CollectionsDotClass(NamedTuple('CollectionsClass', [('x', Incomplete), ('y', Incomplete)])):
     def f(self, a) -> None: ...

-TypingCall: Incomplete
+class TypingCall(NamedTuple):
+    x: int | None
+    y: int

 class TypingClass(NamedTuple):
     x: int | None
     y: str
     def f(self, a) -> None: ...

-class TypingClassWeird:
+class TypingClassWeird(NamedTuple('TypingClassWeird', [('x', int | None), ('y', str)])):
     z: float | None
     def f(self, a) -> None: ...

-class TypingDotClassWeird:
+class TypingDotClassWeird(typing.NamedTuple('TypingClassWeird', [('x', int | None), ('y', str)])):
     def f(self, a) -> None: ...
```
</details>
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.

4 participants