Turn `literal` and `literal_hash` into functions #3071

elazarg · 2017-03-28T09:32:56Z

This PR groups the handling of literal/literal_hash in one place, and removes the auxiliary attributes. I think it is clearer this way, and removes clutter from nodes.py. It is easier to check for consistency (e.g. serialization is not possible at all).

I don't know what is the impact performance-wise; in general it should reduce memory footprint for AST nodes (many can become NamedTuple now), which might have some cost in execution time. If it is noticeable, I guess lru_cache can eliminate that.

The functions can be moved to a different module (binder.py?) but they curry dependencies with them.

elazarg · 2017-03-28T09:35:10Z

The idea has been discussed briefly in #2238.

elazarg · 2017-03-28T09:41:16Z

If it means anything, the tests take the same time to execute.

gvanrossum

I'm honestly not sure this is worth doing. Leaving the decision up to Jukka (who may also have PRs in flight that would require rebasing against this).

gvanrossum · 2017-03-28T19:41:01Z

mypy/checkexpr.py

@@ -21,7 +21,7 @@
    DictionaryComprehension, ComplexExpr, EllipsisExpr, StarExpr, AwaitExpr, YieldExpr,
    YieldFromExpr, TypedDictExpr, PromoteExpr, NewTypeExpr, NamedTupleExpr, TypeVarExpr,
    TypeAliasExpr, BackquoteExpr, ARG_POS, ARG_NAMED, ARG_STAR, ARG_STAR2, MODULE_REF,
-    UNBOUND_TVAR, BOUND_TVAR, LITERAL_TYPE
+    UNBOUND_TVAR, BOUND_TVAR, LITERAL_TYPE, literal


Trailing comma.

gvanrossum · 2017-03-28T19:41:11Z

mypy/binder.py

@@ -2,7 +2,7 @@
 from contextlib import contextmanager

 from mypy.types import Type, AnyType, PartialType
-from mypy.nodes import (Key, Node, Expression, Var, RefExpr, SymbolTableNode)
+from mypy.nodes import (Key, Expression, Var, RefExpr, literal, literal_hash)


While you're at it drop the parentheses.

gvanrossum · 2017-03-28T19:41:23Z

mypy/checker.py

@@ -21,7 +21,7 @@
    Context, Decorator, PrintStmt, LITERAL_TYPE, BreakStmt, PassStmt, ContinueStmt,
    ComparisonExpr, StarExpr, EllipsisExpr, RefExpr, ImportFrom, ImportAll, ImportBase,
    ARG_POS, CONTRAVARIANT, COVARIANT, ExecStmt, GlobalDecl, Import, NonlocalDecl,
-    MDEF, Node
+    MDEF, Node, literal, literal_hash


Add trailing comma.

gvanrossum · 2017-03-28T19:44:28Z

mypy/nodes.py

+#   of an index expression, or the operands of an operator expression).
+
+
+def literal_hash(e: Expression) -> Key:


This is just begging to become a visitor.

I will use a visitor if the PR will be accepted.
(I don't know why exactly, but it feels cleaner this way. OTOH a visitor might help catching non-exhaustive match)

gvanrossum · 2017-03-28T19:45:22Z

mypy/nodes.py

+    return None
+
+
+def literal(e: Expression) -> int:


(This less so.)

gvanrossum · 2017-03-28T19:45:32Z

mypy/semanal.py

@@ -66,7 +66,7 @@
    YieldExpr, ExecStmt, Argument, BackquoteExpr, ImportBase, AwaitExpr,
    IntExpr, FloatExpr, UnicodeExpr, EllipsisExpr, TempNode,
    COVARIANT, CONTRAVARIANT, INVARIANT, UNBOUND_IMPORTED, LITERAL_YES, ARG_OPT, nongen_builtins,
-    collections_type_aliases, get_member_expr_fullname,
+    collections_type_aliases, get_member_expr_fullname, literal


Trailing comma.

JukkaL · 2017-04-25T16:59:34Z

Can you fix the merge conflicts?

elazarg · 2017-04-25T17:39:33Z

(Fixed online, I hope it'll pass)

elazarg · 2017-07-04T20:42:22Z

mypy/nodes.py

@@ -155,9 +155,6 @@ def accept(self, visitor: StatementVisitor[T]) -> T:

 class Expression(Node):
    """An expression node."""
-    literal = LITERAL_NO
-    literal_hash = None  # type: Key


@ilevkivskyi isn't this a violation of strict optional? This value is actually read, it's not just a declaration. I assume this should be Optional[Key]?
(This is why the build for this PR breaks; the new literal_hash() returns None when there's no match, which is what the client code expects).

This is an exception for historical reasons. Before PEP 526 there was no way do define an instance variable without a default value x: int. Therefore it is allowed to write this even with --strict-optional (the same code should fail at module and function scopes).

And yes, if this is not a declaration, then you should use explicitly Optional[Key] here.

OK. I will open a PR.

JukkaL · 2017-07-21T12:05:16Z

The general idea of this factoring looks good to me -- it moves spread-out special-purpose logic away from node classes to a single location. This makes it easier to verify that the literal hash logic is consistent. I prefer node classes to be (at least mostly) pure data. This should also reduce memory use somewhat.

Some things are needed:

Fix merge conflicts.
We need to make sure that this doesn't significantly slow down type checking. I can test the PR against Dropbox internal codebases once you've fixed merge conflicts. Performance is currently one of the top issues at Dropbox so we'll prefer retaining minor code quality problems over slowing things down.
Use visitor for literal_hash. This might even be necessary to avoid a performance hit.

I will do a detailed review once the above things have been resolved.

JukkaL

Explicitly requesting the above changes so that the review status is shown correctly in the PR list.

Conflicts: mypy/binder.py mypy/nodes.py

elazarg · 2017-07-21T16:07:14Z

I will make the visitor later, it's much more verbose (code duplication and implementing empty methods) so I'll try to see the cleanest way to do it.

In the meantime, is it possible to check the performance on against the internal codebase before that? I am not sure that a visitor will be faster, since it adds a method call (or two) at each level, and several cases are grouped to a single isinstance check.

ilevkivskyi · 2017-07-21T16:12:01Z

@elazarg

and several cases are grouped to a single isinstance check.

isinstance() can be quite slow, especially with ABCs, and we have some of them in mypy.

elazarg · 2017-07-21T16:22:34Z

There are no checks against ABCs in this code.

elazarg · 2017-08-10T10:59:36Z

@JukkaL did you have the opportunity to test the performance implications of this PR?

JukkaL · 2017-08-10T12:24:28Z

I'm now back from vacation but I'll have more vacation starting a week from now. I may be able to check performance next week, but not sure yet.

JukkaL

Thanks for the updates! Looks good now (just a few minor comments). I verified the performance with internal Dropbox repos and mypy actually seems slightly faster with this PR than before, which is excellent. Memory should be lower as well, but I didn't check it. The suggested changes are optional except for the IndexExpr thing.

JukkaL · 2017-08-15T15:07:22Z

mypy/nodes.py

+
+    elif isinstance(e, IndexExpr):
+        if literal(e.index) == LITERAL_YES:
+            return literal(e.base)


Add else where we return LITERAL_NO. Otherwise, it looks like we can return LITERAL_YES because of the if literal_hash(e): check.

JukkaL · 2017-08-15T15:16:33Z

mypy/nodes.py

+            return ('Index', literal_hash(e.base), literal_hash(e.index))
+        return None
+
+    def visit_call_expr(self, e: CallExpr) -> None:


Maybe you can have a single method definition and then create a bunch of aliases. Example (not tried):

def visit_non_literal(self, e: Node) -> None: return None visit_call_expr = visit_non_literal visit_slice_expr = visit_non_literal ...

This would reduce some of the verbosity.

This result in Incompatible types in assignment (expression has type Callable[[Node], None], base class "ExpressionVisitor" defined the type as Callable[[CallExpr], Tuple[Any, ...]])
Which kinda looks like a bug to me since the returned value should be defined as Optional[Key].

JukkaL · 2017-08-15T15:20:30Z

mypy/nodes.py

@@ -2559,3 +2470,215 @@ def check_arg_names(names: List[Optional[str]], nodes: List[T], fail: Callable[[
            fail("Duplicate argument '{}' in {}".format(name, description), node)
            break
        seen_names.add(name)
+
+# [Note Literals and literal_hash]


Might be worth moving all the literal and literal hash specific code and comments to a new module to make mypy.nodes smaller. After all, these aren't relevant for most users of nodes. For example, it could be named mypy.literals.

JukkaL · 2017-08-16T18:57:53Z

Thanks for the updates! This almost ready to be merged. Can you fix the merge conflicts and the build failures?

elazarg · 2017-08-16T19:26:53Z

Oops. Sorry about that, I got busy and didn't look at the build results. Just a moment.

…name to _Hasher

elazarg · 2017-08-16T20:02:05Z

Done

elazarg added 3 commits March 28, 2017 11:59

group all literal and literal_hash

f3e18d3

Ignore concatenation with Tuple[Any, ...]

48decc1

Fix typo

286348a

gvanrossum reviewed Mar 28, 2017

View reviewed changes

elazarg added 2 commits March 29, 2017 01:50

Lint imports

3f25101

shrink cases

9611e6e

gvanrossum assigned JukkaL Mar 29, 2017

elazarg added 2 commits April 3, 2017 14:25

Merge branch 'master' into literal_hash1

3c37478

Merge branch 'master' into literal_hash1

36c9c0f

Merge branch 'master' into literal_hash1

04b2c3c

elazarg added 3 commits April 25, 2017 20:56

fix merge

4e363d3

Merge branch 'master' into literal_hash1

8c3941f

Merge branch 'master' into literal_hash1

f81d941

elazarg commented Jul 4, 2017

View reviewed changes

elazarg mentioned this pull request Jul 4, 2017

Declare literal_hash as Optional[Key] #3658

Merged

JukkaL requested changes Jul 21, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into literal_hash1

9be2951

Conflicts: mypy/binder.py mypy/nodes.py

elazarg force-pushed the literal_hash1 branch from ef840cb to 9be2951 Compare July 21, 2017 15:38

elazarg added 2 commits July 23, 2017 01:16

make a Hasher visitor

80d2d9b

Merge branch 'master' into literal_hash1

8b63d02

JukkaL reviewed Aug 15, 2017

View reviewed changes

add "else LITERAL_NO". Move to mypy.literals

859dbf3

Merge branch 'master' into literal_hash1

f239978

tuple -> Tuple[Any, ...]; move isinstance check to literal module; re…

dd44288

…name to _Hasher

JukkaL approved these changes Aug 30, 2017

View reviewed changes

JukkaL merged commit abc228e into python:master Aug 30, 2017

		# of an index expression, or the operands of an operator expression).


		def literal_hash(e: Expression) -> Key:

Uh oh!

Turn literal and literal_hash into functions #3071

Turn literal and literal_hash into functions #3071

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

C0DA Uh oh!

Uh oh!

Uh oh!

Turn `literal` and `literal_hash` into functions #3071

Turn `literal` and `literal_hash` into functions #3071