Implement a reveal_locals expression. (#3387) #3425

aisipos · 2017-05-23T18:35:52Z

This PR is a "rough draft" implementation of a reveal_locals expression, which as it stands can produce from this input:

def f(a: int, b: int) -> int:
    c = a + b
    reveal_locals()
    return c

this output:

sample.py:3: error: Revealed local types are '{'a': builtins.int, 'b': builtins.int, 'c': builtins.int}'

I am new to the codebase so likely this PR will need a fair bit more work and would like some feedback. I certainly need to add new tests and documentation but wanted to do that after a first round of feedback if possible.

Some notes:

Essentially, the implementation is just using the type_map attribute of the TypeChecker instance to produce the output. I don't know if this is the correct approach but it seemed the simplest to my admittedly cursory knowledge of the codebase.
I also am storing the locals attribute of the SemanticAnalyzer into each RevealLocalsExpr, but at the moment I am not using it. This could lead to other ways of implementing this but I wasn't sure of the best use of it.
Currently the output is on one line, dictionary style. I am open to other possibilities, e.g:

sample.py:3: error: Revealed type of 'a': builtins.int
sample.py:3: error: Revealed type of 'b': builtins.int
sample.py:3: error: Revealed type of 'c': builtins.int

However, it's worth considering who the "audience" of this output is. The dictionary output is more machine parseable, which could be useful for editor plugins that use this functionality. Admittedly, when I first thought of this feature, this was the use case I was thinking of.

There is at least one bug in the current implementation:

def f(a: int, b: int) -> int:
    reveal_locals()
    c = a + b
    reveal_locals()
    return c

gives:

sample.py:2: error: Revealed local types are '{}'
sample.py:4: error: Revealed local types are '{'a': builtins.int, 'b': builtins.int, 'c': builtins.int}'

The output should be the same for both lines. I do not know of an easy fix yet.

Feedback is welcomed. I realize there will be likely many changes to this before it is merge ready.

aisipos · 2017-05-23T21:04:41Z

Some discussion with @JukkaL :
as implemented, reveal_locals() does show variables from surrounding scopes, which is inconsistent with the name. The choices would be a new name (e.g., reveal_all or something), or to actually limit reveal_locals() to just local variables. Either is valuable, although perhaps if the primary audience would be editors, it might be useful to have more information than less, so I would lean to keeping the surrounding variables in the output but change the name.

I also fixed unnecessary moving of some classes in b0ca357

There is still a bug where reveal_locals() at the beginning of a function doesn't "see" the function arguments. I'm working to understand why.

gvanrossum · 2017-05-23T21:08:14Z

I prefer just locals; global could be too much.

aisipos · 2017-05-24T18:32:12Z

@ddfisher I fixed some of the issues we discussed. I unified RevealLocalsExpr into RevealTypeExpr, however in the various traversers I kept the distinction between visit_reveal_type_node and visit_reveal_locals_node. I did this to avoid having a giant if statement section inside each visit_reveal_type_node definition, however if you'd rather I unify the two of them I'm happy to make that change.

The main downside is I still cannot deal with the issue where:

def f(a: int, b: int) -> int:
    reveal_locals()
    c = a + b
    reveal_locals()
    return c

The second reveal_locals "sees" c,a,b, but the first reveal_locals sees no variables. I made some experimental commits to try to fix this but reverted them because they were probably a dead end. reveal_type is able to see them because it introduces a variable node of the variable it wants to reveal, but since reveal_locals() takes no arguments it doesn't introduce any variable nodes. I still don't understand why the TypeChecker doesn't see the types of the function arguments inside a function, but I will keep looking.

aisipos · 2017-06-28T07:29:28Z

@ddfisher OK, I've rebased this PR against master, added some tests, added some docs, and fixed some test breakage (Travis runs successfully now) . Some of the issues I had with type inference a month ago appear to be gone, I'm assuming changes in the past month have made the method used in this PR work now. Still needs a good review though. The commits are a bit of a mess, but I will squash them once I get some feedback. Thanks for your help.

ddfisher · 2017-06-28T23:22:36Z

Thanks for the updates! I'll try to take a look in the next couple days!

aisipos · 2017-07-16T21:41:18Z

@ddfisher Due to the recent mypy release, I've rebased this PR on top of master, fixing conflicts and fixing new typing linting errors. All tests are passing again.

JukkaL · 2017-11-24T16:30:09Z

@aisipos Are you interested in finishing this up?

aisipos · 2017-11-25T01:28:31Z

Hi @JukkaL, I would be happy to finish it up. It's been a while since I gave it any attention, but I will try to get it up to date again with master this weekend.

aisipos · 2018-02-18T23:44:08Z

Hi @JukkaL, I have rebased this on top of master and fixed all the tests. However, it should get a review from a maintainer. I'm happy to address any concerns about the functionality or implementation.

JukkaL

Looks good, I just had a few minor things. This feature will make it easier for users to troubleshoot cases when mypy doesn't work as expected. Apologies for the slow response, let's try to get this merged soon.

JukkaL · 2018-05-14T14:19:50Z

docs/source/common_issues.rst

+
+   a = 1
+   b = 'one'
+   reveal_locals()  # Revealed local types are '{'a': builtins.int, 'b': builtins.str}


Is this missing a closing quote?

JukkaL · 2018-05-14T14:22:38Z

mypy/checkexpr.py

-                self.msg.note("'reveal_type' always outputs 'Any' in unchecked functions", expr)
-        return revealed_type
+        if expr.kind == REVEAL_TYPE:
+            if expr.expr is not None:


Maybe using assert expr.expr is not None would make sense here? Now if the condition is false, we might read an undefined variable below.

JukkaL · 2018-05-14T14:47:42Z

mypy/messages.py

+        s = "{{{}}}".format(
+            ', '.join("'{}': {}".format(k, v) for k, v in sorted_locals.items())
+        )
+        self.fail('Revealed local types are \'{}\''.format(s), context)


What about using double quotes around this since the message contains single quotes? The current output is a bit unclear. I'd actually prefer displaying one item at a line, such as like this:

Revelead local types: a: int b: str c: ...

This could make it a little easier to find the type of a particular variable.

I'd be happy to change the implementation to write multiple lines, but then I don't know how to use the test functions to verify multi-line output. Is this possible?

JukkaL · 2018-05-14T14:48:27Z

mypy/nodes.py

-class RevealTypeExpr(Expression):
-    """Reveal type expression reveal_type(expr)."""
+class RevealExpr(Expression):
+    """Reveal type expression reveal_type(expr) or reveal locals expression."""


Maybe say "... reveal_locals() expression" to make this a bit more explicit?

JukkaL · 2018-05-14T14:48:53Z

mypy/nodes.py

+    def __init__(
+            self, kind: int,
+            expr: Optional[Expression] = None,
+            local_nodes: 'Optional[List[Var]]'=None) -> None:


Style nit: We use spaces around =.

JukkaL · 2018-05-14T14:50:44Z

mypy/traverser.py

+        if o.kind == REVEAL_TYPE:
+            from mypy.nodes import Expression
+            from typing import cast
+            expr = cast(Expression, o.expr)


I'd prefer to use assert o.expr is not None here, as it's a bit clearer.

JukkaL · 2018-05-14T15:06:29Z

mypy/treetransform.py

-        return RevealTypeExpr(self.expr(node.expr))
+    def visit_reveal_expr(self, node: RevealExpr) -> RevealExpr:
+        if node.kind == REVEAL_TYPE:
+            return RevealExpr(kind=REVEAL_TYPE, expr=self.expr(cast(Expression, node.expr)))


Again, I'd use assert node.expr is not None here.

JukkaL · 2018-05-14T15:07:18Z

test-data/unit/check-functions.test

@@ -2302,3 +2302,14 @@ f = lambda: 5
 reveal_type(f)
 [out]
 main:2: error: Revealed type is 'def () -> builtins.int'
+
+[case testRevealLocalsFunction]


Thanks for adding tests! I have one idea for additional test: using reveal_locals() inside class body.

Is the test testRevealLocalsOnClassVars sufficient for this?

aisipos · 2018-05-15T14:51:22Z

@JukkaL OK, I think I have addressed all your recent feedback and have pushed my changes. Have another look?

JukkaL

Thanks for the updates! Looks good now.

ddfisher self-requested a review May 23, 2017 23:14

aisipos force-pushed the reveal_locals branch from 5401837 to 7f1e1b1 Compare May 24, 2017 17:34

gvanrossum assigned ddfisher May 30, 2017

aisipos force-pushed the reveal_locals branch 3 times, most recently from 5c1230e to 62402cf Compare June 28, 2017 05:29

aisipos force-pushed the reveal_locals branch from b1c3a77 to 05d55b6 Compare July 16, 2017 21:19

JukkaL unassigned ddfisher Nov 24, 2017

aisipos force-pushed the reveal_locals branch from 64a824f to 81bf4a8 Compare February 18, 2018 23:27

aisipos force-pushed the reveal_locals branch from 81bf4a8 to 301b278 Compare May 6, 2018 20:50

JukkaL reviewed May 14, 2018

View reviewed changes

aisipos added 3 commits May 15, 2018 10:26

Implement a reveal_locals expression. (python#3387)

0d44aa2

Use assertions rather than casts in reveal_locals() implementation

46cb7f0

Use a multiline message for reveal_locals

2ec6c88

aisipos force-pushed the reveal_locals branch from 7534880 to 2ec6c88 Compare May 15, 2018 14:31

JukkaL approved these changes May 15, 2018

View reviewed changes

JukkaL merged commit e378601 into python:master May 15, 2018

cburgdorf mentioned this pull request Jun 21, 2018

Update mypy to version 0.610 ethereum/py-evm#942

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a reveal_locals expression. (#3387) #3425

Implement a reveal_locals expression. (#3387) #3425

aisipos commented May 23, 2017 •

edited

Loading

aisipos commented May 23, 2017

gvanrossum commented May 23, 2017

aisipos commented May 24, 2017 •

edited

Loading

aisipos commented Jun 28, 2017

ddfisher commented Jun 28, 2017

aisipos commented Jul 16, 2017

JukkaL commented Nov 24, 2017

aisipos commented Nov 25, 2017

aisipos commented Feb 18, 2018

JukkaL left a comment

JukkaL May 14, 2018

JukkaL May 14, 2018

JukkaL May 14, 2018

aisipos May 15, 2018

JukkaL May 14, 2018

JukkaL May 14, 2018

JukkaL May 14, 2018

JukkaL May 14, 2018

JukkaL May 14, 2018

aisipos May 15, 2018

aisipos commented May 15, 2018

JukkaL left a comment

Implement a reveal_locals expression. (#3387) #3425

Implement a reveal_locals expression. (#3387) #3425

Conversation

aisipos commented May 23, 2017 • edited Loading

aisipos commented May 23, 2017

gvanrossum commented May 23, 2017

aisipos commented May 24, 2017 • edited Loading

aisipos commented Jun 28, 2017

ddfisher commented Jun 28, 2017

aisipos commented Jul 16, 2017

JukkaL commented Nov 24, 2017

aisipos commented Nov 25, 2017

aisipos commented Feb 18, 2018

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aisipos commented May 15, 2018

JukkaL left a comment

Choose a reason for hiding this comment

aisipos commented May 23, 2017 •

edited

Loading

aisipos commented May 24, 2017 •

edited

Loading