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

Don't resolve Callable NamedTuple fields to their return type #6576

Merged
merged 2 commits into from Mar 21, 2019

Conversation

@FuegoFro
Copy link
Contributor

commented Mar 20, 2019

NamedTuple fields are always marked an properties. In certain situations, this causes their type to be determined incorrectly when they hold Callables, resolving to the Callable's return type rather than the Callable itself. This prevents that and fixes #6575.

Don't resolve Callable NamedTuple fields to their return type
`NamedTuple` fields are always marked an properties. In certain situations, this causes their type to be determined incorrectly when they hold `Callable`s, resolving to the `Callable`'s return type rather than the `Callable` itself. This prevents that and fixes #6575.
@msullivan

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

I am going to look at the bug you filed and think a bit about the whether this is the best approach (or whether, as you suggested, we should change something on the namedtuple side setting properties), but separate from that: could you please add a test.

@msullivan msullivan self-requested a review Mar 21, 2019

@@ -622,7 +622,7 @@ def find_node_type(node: Union[Var, FuncBase], itype: Instance, subtype: Type) -
if isinstance(node, FuncBase) or isinstance(typ, FunctionLike) and not node.is_staticmethod:
assert isinstance(typ, FunctionLike)
signature = bind_self(typ, subtype)
if node.is_property:
if node.is_property and not node.info.is_named_tuple:

This comment has been minimized.

Copy link
@msullivan

msullivan Mar 21, 2019

Collaborator

Hm, it looks like this will still do bind_self on the type, which I think might cause trouble, since it will modify the type of the field in an unexpected way. Make sure to test with multiple arguments.

This comment has been minimized.

Copy link
@msullivan

msullivan Mar 21, 2019

Collaborator

Looking at https://github.com/python/mypy/blob/master/mypy/checkmember.py#L495, it seems that only is_initialized_in_class vars get the property/self binding treatment, so probably the right approach is to match that.

@FuegoFro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Thanks for the response! Yeah, I'll take a stab at adding some tests for various scenarios, including where the Callable takes multiple arguments 🙂

@FuegoFro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Thanks for the pointers here! I changed the 'self' binding check to match the code/condition you linked to 🙂 I also added some integration tests. I haven't really worked with this codebase much before, so I wasn't sure if there's a way I should try to make these work in the testsubtypes.py file. Let me know if there's a better way to add tests, or any other types of tests you'd like to see!

I did notice that a TestPEP561.test_typedpkg_editable test was failing locally, but it failed for me even on master and it seems unrelated to this change. However, if that does seem relevant and you have some pointers I can dig into it more 🙂

@msullivan
Copy link
Collaborator

left a comment

Thanks!

Just writing end-to-end typechecker tests is totally fine and pretty standard for mypy. While we have some unit tests, the easiest way to specify programming language constructs is generally with concrete syntax so...

The TestPEP561 are frustratingly fragile. Are you testing inside a venv? TestPEP561 uses a virtualenv and nesting virtualenvs inside venvs and vice-versa tend to break in "interesting" ways.

@msullivan msullivan merged commit a642784 into python:master Mar 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@FuegoFro FuegoFro deleted the FuegoFro:named_tuple_callable_field branch Mar 22, 2019

@FuegoFro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Thank you for you help and quick responses/turnaround! Don't think I was in a venv, but it sounds like this wasn't entirely unexpected, so I'll hold off on looking into my local setup until/unless I have more PRs to send 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.