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

Implement a reveal_locals expression. (#3387) #3425

Merged
merged 3 commits into from May 15, 2018

Conversation

Projects
None yet
4 participants
@aisipos
Copy link
Contributor

aisipos commented May 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

aisipos commented May 23, 2017

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

This comment has been minimized.

Copy link
Member

gvanrossum commented May 23, 2017

I prefer just locals; global could be too much.

@ddfisher ddfisher self-requested a review May 23, 2017

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

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented May 24, 2017

@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 aisipos force-pushed the aisipos:reveal_locals branch 3 times, most recently from 5c1230e to 62402cf Jun 28, 2017

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented Jun 28, 2017

@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

This comment has been minimized.

Copy link
Collaborator

ddfisher commented Jun 28, 2017

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

@aisipos aisipos force-pushed the aisipos:reveal_locals branch from b1c3a77 to 05d55b6 Jul 16, 2017

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented Jul 16, 2017

@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

This comment has been minimized.

Copy link
Collaborator

JukkaL commented Nov 24, 2017

@aisipos Are you interested in finishing this up?

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented Nov 25, 2017

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 aisipos force-pushed the aisipos:reveal_locals branch from 64a824f to 81bf4a8 Feb 18, 2018

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented Feb 18, 2018

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.

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

@JukkaL
Copy link
Collaborator

JukkaL left a comment

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.

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

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

Is this missing a closing quote?

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:

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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.

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

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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.

This comment has been minimized.

@aisipos

aisipos May 15, 2018

Author Contributor

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?

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

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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

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

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

Style nit: We use spaces around =.

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

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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

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)))

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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

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

[case testRevealLocalsFunction]

This comment has been minimized.

@JukkaL

JukkaL May 14, 2018

Collaborator

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

This comment has been minimized.

@aisipos

aisipos May 15, 2018

Author Contributor

Is the test testRevealLocalsOnClassVars sufficient for this?

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

@aisipos

This comment has been minimized.

Copy link
Contributor Author

aisipos commented May 15, 2018

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

@JukkaL

JukkaL approved these changes May 15, 2018

Copy link
Collaborator

JukkaL left a comment

Thanks for the updates! Looks good now.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.