Skip to content

TypedDict: Recognize __getitem__ and __setitem__ access. #2526

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
Dec 20, 2016

Conversation

davidfstr
Copy link
Contributor

Partially resolves #2486 . If the approach here looks good I can extend the diff to recognize __setitem__ as well, which would fully resolve #2486 .

@davidfstr davidfstr mentioned this pull request Dec 5, 2016
@gvanrossum
Copy link
Member

Hm... I'm not crazy about this approach. It seems to be type-checking the argument expression (e.g. 'x' in d['x']) at the point where it's supposed to just be looking for the __getitem__ function. I feel that maybe these checks ought to be done in check_op_local()?

@davidfstr
Copy link
Contributor Author

Is the concern that performing the check in the more-widely-used analyze_member_access() might be too general? I can certainly move the call up to check_op_local(), although I believe the semantics will be equivalent.

Moving the check up to check_op_local() may make it more difficult to recognize direct invocations like point.__getitem__('x') which we may also want to consider, although the current diff doesn't presently recognize that style of invocation.

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 7, 2016

Supporting point.__getitem__('x') doesn't seem important to me -- I can't find any examples of it in a large internal codebase. For consistency, visit_index_expr_helper seems like the right place for the __getitem__ support. We also special case tuples there, and TypedDict is sufficiently similar.

@davidfstr davidfstr changed the title TypedDict: Recognize __getitem__ access. TypedDict: Recognize __getitem__ and __setitem__ access. Dec 12, 2016
@davidfstr
Copy link
Contributor Author

I've moved __getitem__ recognition to visit_index_expr_helper() as suggested, and added __setitem__ recognition. This PR now covers the entirety of #2486 .

Squished and rebased to the tip of master.

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.

Looks good! Just some minor comments.

from mypy_extensions import TypedDict
TaggedPoint = TypedDict('TaggedPoint', {'type': str, 'x': int, 'y': int})
p = TaggedPoint(type='2d', x=42, y=1337)
p['x'] = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also try p['x'] = 'y' (should be an error) and p['type'] = 'a' (valid).

def typeddict_item_name_must_be_string_literal(self,
typ: TypedDictType,
context: Context):
self.fail('Cannot prove expression is a valid item name. Expected one of {}.'.format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style nit: we tend not to use periods in error messages. A more consistent message would be Cannot prove expression is a valid item name; expected one of [...] (no dot at the end).

Maybe also truncate the item name list (probably not useful to display dozens of names, for example).

typ: TypedDictType,
item_name: str,
context: Context):
self.fail('\'{}\' is not a valid item name. Expected one of {}.'.format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to above: a more consistent message would be ... is not a valid item name; expected one of [...] (not dot at the end).

Also consider truncating the list (e.g. ['x', 'y', 'z', 'a', 'b', ...]) if it's long enough.

@@ -1421,6 +1423,18 @@ def _get_value(self, index: Expression) -> Optional[int]:
return -1 * operand.value
return None

def visit_typeddict_index_expr(self, td_type: TypedDictType, index: Expression):
if not isinstance(index, (StrExpr, UnicodeExpr)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add test case for both 'foo' and u'foo' key names in Python 2 mode.

@davidfstr
Copy link
Contributor Author

davidfstr commented Dec 16, 2016

Got all the feedback integrated except for one item:

Add test case for both 'foo' and u'foo' key names in Python 2 mode.

When writing that test case, it crashes mypy when run inside the test runner but not when running mypy normally. Here's the problem: If you run py.test -k TypedDict there is a crash in semanal.py at:

    def normalize_type_alias(self, node: SymbolTableNode,
                             ctx: Context) -> SymbolTableNode:
        if node.fullname in type_aliases:
            # Node refers to an aliased type such as typing.List; normalize.
            node = self.lookup_qualified(type_aliases[node.fullname], ctx)  # <-- HERE
        if node.fullname == 'typing.DefaultDict':  # <-- THERE
            self.add_module_symbol('collections', '__mypy_collections__', False, ctx)
            node = self.lookup_qualified('__mypy_collections__.defaultdict', ctx)
        return node

At the line marked HERE, lookup_qualified is called with 'builtins.dict' which fails, returning a None node. That None node crashes the line marked THERE with a null pointer exception.

How is it that lookup_qualified('__builtins__.dict') could fail? Maybe there's something different in Python 2.7 mode?

@gvanrossum
Copy link
Member

gvanrossum commented Dec 16, 2016 via email

@davidfstr
Copy link
Contributor Author

Here is the test in question:

[case testCanGetItemOfTypedDictWithValidBytesOrUnicodeLiteralKey]
# flags: --python-version 2.7
(...)
[builtins fixtures/dict.pyi]
  • If I run it as-is (in Python 2.7 mode), I get the missing __builtins__.dict problem.
  • If I remove the # flags: --python-version 2.7 line then the test succeeds in Python 3.x mode.
  • If I remove the # flags: --python-version 2.7 line and the [builtins fixtures/dict.pyi] fixture then I get the missing __builtins__.dict problem again.

The preceding behavior suggests that the [builtins fixtures/dict.pyi] line is being ignored in Python 2.7 mode or perhaps a different dict.pyi fixture is being used. Does this sound plausible?

I will keep investigating...

@davidfstr
Copy link
Contributor Author

Ha! Apparently I need to say [builtins_py2 fixtures/dict.pyi] rather than [builtins fixtures/dict.pyi] when in Python 2.x mode.

@davidfstr
Copy link
Contributor Author

@JukkaL , I've integrated all the requested feedback. Ready for another look.

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 20, 2016

Looks good now! Thanks for the updates!

@JukkaL JukkaL merged commit 8453dda into python:master Dec 20, 2016
@davidfstr davidfstr deleted the typeddict_getitem branch January 22, 2017 22:45
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.

Getting and setting items of TypedDict
3 participants