Skip to content

selftype in namedtuple methods #2408

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

Merged
merged 2 commits into from
Nov 7, 2016
Merged

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Nov 6, 2016

Fix one item from #2090.

@gvanrossum gvanrossum merged commit baa4628 into python:master Nov 7, 2016
@elazarg elazarg deleted the namedtuple_selftype branch November 7, 2016 16:57
@gvanrossum
Copy link
Member

I'm going to revert this. Here's a problem we encountered that was caused by this PR:

from typing import NamedTuple
NT = NamedTuple('NT', [('x', str)])
class C:
  def __init__(self) -> None:
    self.a = NT('')
  def foo(self) -> None:
    self.a = self.a._replace(x='')  # Error here

The error is

__tmp__.py: note: In member "foo" of class "C":
__tmp__.py:7: error: Incompatible types in assignment (expression has type "__tmp__.NT", variable has type "NT")

@elazarg
Copy link
Contributor Author

elazarg commented Nov 7, 2016

Ouch. Sorry.

@gvanrossum
Copy link
Member

gvanrossum commented Nov 7, 2016 via email

gvanrossum added a commit that referenced this pull request Nov 7, 2016

Reverts #2408

This appears to have a bug; see my comment to the PR for an explanation.
@elazarg
Copy link
Contributor Author

elazarg commented Nov 7, 2016

My fault is not the bug, but the fact that this PR has no reasonable tests. I should be more careful.

This whole method of constructing a new Instance inside the code is volatile. I'd rather have a superclass and a magic parameter. Something like:

class NamedTuple(metaclass=NamedTupleMeta):
    def _replace(self: T, **kwargs: __FIELDS__=...) -> T: ...

It should probably be somehow internal to mypy. It will also make the implementation of the new-style syntax trivial.

@gvanrossum
Copy link
Member

Yeah, I should have realized that this requires new tests and asked you to add some. I'm not sure I understand your code snippet -- where should that go? What is __FIELDS__?

@elazarg
Copy link
Contributor Author

elazarg commented Nov 7, 2016

It should go in typeshed, or probably "shadow" typeshed (do we have such thing?).

One problem with namedtuple currently is that the signature of _replace and __init__ depends on the declared fields, thus requiring dedicated generating code inside mypy.

__FIELDS__ is a magic annotation that tells mypy to create a new method for the subclass, with the appropriate parameters (e.g. x: int = ..., y: str = ... if the new subclass has fields x : int and y : str).

(In general, I am trying to figure out a "language" that will allow expressing common patterns in metaclasses, decorators and other type-manipulating constructs. selftype is helpful, but I think we need more. The intention is not to be an pyi-style interface, but more of a plugin. In the extreme, I was thinking about a very small subset of Python, or something similar to template-metaprogramming. I don't have any concrete suggestion so there's no need to reject it just yet :) )

@gvanrossum
Copy link
Member

gvanrossum commented Nov 7, 2016 via email

@elazarg
Copy link
Contributor Author

elazarg commented Nov 7, 2016

I meant somewhere we can put signatures that are mypy-specific, possibly experimental.

@gvanrossum
Copy link
Member

gvanrossum commented Nov 8, 2016 via email

@elazarg elazarg mentioned this pull request Nov 11, 2016
gvanrossum pushed a commit that referenced this pull request Nov 13, 2016
(Reopening #2408 after the revert #2414. This fixes parts of #2090)
This PR does three things:

    Fix the handling of TupleType (pass original_type recursively)
    Fix the handling of TypeType (avoid ad-hoc buggy special casing)
    Make NamedTuple's _replace() and _make() return selftype, serving as test case for (1) and (2)

As a consequence of (1), some error messages are changed, as exemplified in check-isinstance.test. I think it's better now, but if it isn't, perhaps we should separate original_type and report_type as discussed in #2193.
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