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

Add LiteralType and stub methods to type visitors #5934

Merged
merged 2 commits into from
Nov 22, 2018

Conversation

Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Nov 21, 2018

This diff adds a 'LiteralType' class to types.py and adds a corresponding stub method to all of the type visitors.

Most of these stub methods just throw a 'NotImplementedError', though I did pencil in a few implementations that looked obvious.

The reason I'm doing this now instead of later is because I want to start by getting these kind of big, gross, multi-file changes out of the way early and try and have any subsequent pull requests be more focused.

I also want to formally confirm that the correct approach here is to create a new 'LiteralType' class before I start on the rest of the implementation work. (Other possible approaches would be to tack on some extra attribute to 'Instance', or make 'LiteralType' a subclass of 'Instance'.)

No tests, sorry. My plan is to work on modifying the parsing and semantic analysis steps next before returning back to these the unimplemented methods and add tests then -- here's the tracking issue for these TODOs: #5935

Tests shouldn't be necessary, in any case: everything I added just now is basically dead code.

This diff adds a 'LiteralType' class to types.py and adds a
corresponding stub method to all of the type visitors.

Most of these stub methods just throw a 'NotImplementedError',
though I did pencil in a few implementations that looked obvious.

The reason I'm doing this now instead of later is because I want
to start by getting these kind of big, gross, multi-file changes
out of the way early and try and have any subsequent pull requests
be more focused.

I also want to formally confirm that the correct approach here
*is* to create a new 'LiteralType' class before I start on the
rest of the implementation work. (Other possible approaches would
be to tack on some extra attribute to 'Instance', or make
'LiteralType' a subclass of 'Instance'.)

No tests, sorry. My plan is to work on modifying the parsing
and semantic analysis steps next before returning back to these
the unimplemented methods and add tests then. I'll be opening
a tracking issue momentarily to keep track of these TODOs.

Tests shouldn't be necessary, in any case: everything I added
just now is basically dead code.
@Michael0x2a Michael0x2a mentioned this pull request Nov 21, 2018
42 tasks
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.

I like this approach of getting started with implementing literal types. After this initial PR it should be easy to create multiple smaller follow-up PRs without too much trouble from merge conflicts.

@@ -78,6 +78,10 @@ def visit_tuple_type(self, t: TupleType) -> Type:
def visit_typeddict_type(self, t: TypedDictType) -> Type:
return t.fallback.accept(self)

def visit_literal_type(self, t: LiteralType) -> Type:
# TODO: Verify this implementation is correct
return t.fallback.accept(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe return t unmodified instead.

@@ -27,6 +27,19 @@

JsonDict = Dict[str, Any]

# The set of all valid expressions that can currently be contained
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add TODO comments here about some things? At least enum support is probably not going to happen immediately, so maybe it should be as a TODO comment?

# Literals can contain enum-values: we special-case those and
# store the value as a string.
#
# Note: this type also happens to correspond to types that can be
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is very useful, thanks!

mypy/types.py Outdated
# of 'LiteralType' rely on this, as well 'server.astdiff.SnapshotTypeVisitor'
# and 'types.TypeStrVisitor'. If we end up adding any non-JSON-serializable
# types to this list, we should make sure to edit those methods to match.
LiteralInnerExpr = Union[int, str, bool, None]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we need to be able to distinguish between unicode and str literals in Python 2? For example, the open() mode argument could well be a unicode literal if using from __future__ import unicode_literals.

A name like LiteralValue would perhaps be a bit more descriptive. The current name implies to me that this is an Expression object, which is not the case.

@@ -1240,6 +1253,43 @@ def zipall(self, right: 'TypedDictType') \
yield (item_name, None, right_item_type)


class LiteralType(Type):
__slots__ = ('value', 'fallback')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add docstring or at least a TODO comment about documenting this.

mypy/types.py Outdated
@@ -1693,6 +1743,9 @@ def item_str(name: str, typ: str) -> str:
suffix = ', fallback={}'.format(t.fallback.accept(self))
return 'TypedDict({}{}{})'.format(prefix, s, suffix)

def visit_literal_type(self, t: LiteralType) -> str:
return 'Literal[{}]'.format(t.value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use repr() here so that string literals that contain unusual characters won't cause trouble. It would also match the source syntax.

@Michael0x2a
Copy link
Collaborator Author

@JukkaL -- ok, done! I made all the changes you suggested in your comments above.

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.

LGTM!

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 22, 2018

(Feel free to merge once the build is green.)

@Michael0x2a Michael0x2a merged commit b099496 into python:master Nov 22, 2018
@Michael0x2a Michael0x2a deleted the add-literal-type-skeleton branch November 22, 2018 18:40
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