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

Do not use Any in NamedTuple() constructor #6489

Closed
wants to merge 2 commits into from
Closed

Do not use Any in NamedTuple() constructor #6489

wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Dec 4, 2021

While working on python/mypy#11206 I've found that this test now fails: https://github.com/python/mypy/blob/cee5d3e9a29f43a10a8eeca760976657bf1689c9/test-data/unit/check-flags.test#L1177-L1185

Looks like object is better suited here. Literal[1] is also an object, Union[int, str] is an object as well.
So, it should always work (I've tested that the current mypy agrees with me). And no Any related problems.

While working on python/mypy#11206 I've found that this test now fails: https://github.com/python/mypy/blob/cee5d3e9a29f43a10a8eeca760976657bf1689c9/test-data/unit/check-flags.test#L1177-L1185

Looks like `object` is better suited here. `Literal[1]` is also an object, `Union[int, str]` is an object as well.
So, it should always work (I've tested that the current `mypy` agrees with me). And no `Any` related problems.
@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Dec 4, 2021

This doesn't seem correct to me. The following fails at runtime:

NamedTuple("Foo", [("abc", 123)])
NamedTuple("Foo", None, abc=123)

@sobolevn
Copy link
Member Author

sobolevn commented Dec 4, 2021

@srittau NamedTuple is special enough, it is probably special-cased in type-checkers.

For example, mypy handles this correctly:

from typing import NamedTuple

Foo = NamedTuple("Foo", [("abc", 123)])
Bar = NamedTuple("Bar", None, abc=123)

Output:

out/ex.py:3: error: Invalid type: try using Literal[123] instead?
out/ex.py:4: error: Invalid type: try using Literal[123] instead?

But, I would like to drop an extra comment on object vs type type in fields / **kwargs here. It would be ideal to make them to work with type. But, right now _SpecialForm items like Literal[123] do not fit type annotation: out/ex.py:3: error: List item 0 has incompatible type "Tuple[str, object]"; expected "Tuple[str, type]". So, object is the least troublesome type for now 😞

@AlexWaygood
Copy link
Member

NamedTuple is special enough, it is probably special-cased in type-checkers.

Maybe if you added a comment clarifying that the stub is "deliberately incorrect" here? 🙂 Something like:

# NOTE: The overloads for __init__ are not strictly accurate
# (type for `fields` and `**kwargs` should be more precise than `object`)
# but typing `NamedTuple` like this makes it easier for mypy to special-case

@srittau
Copy link
Collaborator

srittau commented Dec 4, 2021

I think we should just keep Any, our marker for situations like this.

@sobolevn
Copy link
Member Author

sobolevn commented Dec 4, 2021

I think we should just keep Any, our marker for situations like this.

As I said in #6489 (comment), Any causes some problems in mypy when --disallow-any-expr is activated. The fix for it would not be as easy on mypy's side. But, probably doable in case this PR won't be accepted.

I will attach @AlexWaygood's comment. Maybe this would be good enough 🙂

@srittau
Copy link
Collaborator

srittau commented Dec 4, 2021

I'm hesitant to make "wrong" changes to typeshed to benefit a single type checker. Sometimes this is unavoidable, but I'd like to see other avenues explored first. I'd be interested what other maintainers think, though. Cc @JelleZijlstra @hauntsaninja @Akuli

@Akuli
Copy link
Collaborator

Akuli commented Dec 4, 2021

To me, it seems unlikely that anyone actually wants to use mypy with --disallow-any-expr and also create namedtuples using the Foo = NamedTuple(...) syntax, and not the class Foo(NamedTuple) syntax. If the maintainers of a project have the time to get rid of all Any types, why wouldn't they also switch to the class-based NamedTuple syntax, which in my opinion just looks more readable?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2021

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

Agree with @srittau.

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.

None yet

5 participants