-
Notifications
You must be signed in to change notification settings - Fork 652
Implement print_function future feature #23
Conversation
@@ -41,6 +63,7 @@ def __init__(self, block_): | |||
self.block = block_ | |||
self.writer = util.Writer() | |||
self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer) | |||
self.parser_flags = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the future behavior affect code only after the from future line or does it affect the whole file? If it's the latter then we probably have to do a separate pass to collect all the flags and pass those in to the StatementVisitor as init params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get away with a single pass. According to the 2.x docs:
A future statement must appear near the top of the module. The only lines that can appear before a future statement are:
- the module docstring (if any),
- comments,
- blank lines, and
- other future statements.
None of which any future feature would alter the parsing logic of. So I think we can safely proceed with any flags after they are encountered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that the ast module doesn't check that from __future__
imports satisfy these requirements so I think we should add a check and raise ParseError when from __future__
appears later in the file.
Not sure the best way to do this. Maybe override NodeVisitor.visit()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep -- that is what I came up with. I should have an update shortly.
for alias in node.names: | ||
name = alias.name | ||
if name in future_features: | ||
(flag, implemented) = future_features[name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: kill unnecessary parens:
flag, implemented = future_features[name]
elif name in redundant_future_features: | ||
# no-op | ||
pass | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kill the no-op elif and replace it with:
elif name not in redundant_future_features:
... raise ParseError
self.assertEqual((0, 'abc 123\nfoo bar\n'), _GrumpRun(textwrap.dedent("""\ | ||
print 'abc', | ||
print '123' | ||
print 'foo', 'bar'"""))) | ||
|
||
def testFutureFeaturePrintFunction(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a test against one of the unimplemented future features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
end = kwend.Value() | ||
case "file": | ||
// TODO: need to map Python sys.stdout, sys.stderr etc. to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap comments at 80 chars.
// to recover to the file descriptor probably | ||
} | ||
} | ||
for i, arg := range args { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe generalize grumpy.Print() instead of duplicating the logic here.
@@ -431,6 +432,44 @@ func builtinOrd(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) { | |||
return NewInt(result).ToObject(), nil | |||
} | |||
|
|||
func builtinPrint(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have a couple tests for this. I know it's a hassle grabbing the output and verifying, but there's some prior art already: https://github.com/google/grumpy/blob/master/runtime/core_test.go#L646
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks for taking this on!
It's probably too late to change this, but I would definitely recommend operating on the Python bytecode rather than the Python ast. There are a lot of subtleties in the ast->bytecode compilation process that take time to implement correctly, and I feel like we made a mistake in Pyston by operating on the ast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great. One last suggestion.
@@ -41,6 +63,7 @@ def __init__(self, block_): | |||
self.block = block_ | |||
self.writer = util.Writer() | |||
self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer) | |||
self.parser_flags = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that the ast module doesn't check that from __future__
imports satisfy these requirements so I think we should add a check and raise ParseError when from __future__
appears later in the file.
Not sure the best way to do this. Maybe override NodeVisitor.visit()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and also please confirm that make precommit
passes cleanly.
This allows the import statement `from __future__ import print_function` to work, by setting a flag on the parser to treat print statement/keyword as a syntax error.
45a5d9a
to
f761416
Compare
@trotterdylan I cleaned up some pylint errors from the code I added, but when I run |
|
||
def visit(self, node): | ||
root = node | ||
# If this is the module node, do an initial pass through the module body's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to bring this up late, but what do you think about having a separate FutureVisitor (or something) class that performs this logic ahead of time. The future flags can then be computed and passed into the ModuleBlock constructor (somewhere around here) and accessed by the StatementVisitor.
The nice thing about that is it reduces the amount of state managed by the StatementVisitor. I'm concerned about increasing the complexity of StatementVisitor because it could easily become a catch-all and get out of control given its centrality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, makes sense. I'll put something together.
@paulsmith I'm not sure what's up with the pylint errors. I don't see anything in a clean branch. Can you paste some of what you're seeing? |
@paulsmith I think pylint may be checking against Python 3 syntax. This line for example says a print statement is invalid syntax, which would be true for 3. My guess is that pylint is executing under your system python install instead of python2.7. You could hardcode in the Makefile here what Python to execute pylint with. This confusion has messed up the build for other folks as well. I'm going to add some checks to make sure the Python being used is always 2.7. |
@trotterdylan PTAL -- I refactored the future pass logic into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. This is coming along great. A few more suggestions.
|
||
|
||
def import_from_future(node): | ||
"""processes a future import statement, returning set of flags it defines.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize "processes..."
class StatementVisitor(ast.NodeVisitor): | ||
"""Outputs Go statements to a Writer for the given Python nodes.""" | ||
|
||
# pylint: disable=invalid-name,missing-docstring | ||
|
||
def __init__(self, block_): | ||
self.block = block_ | ||
self.future_features = self.block.future_features or FutureFeatures() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern I've used for accessing global state like this (and I'm not saying it's a good pattern, just an established one) is to put a property on Block that pulls the corresponding attribute from self._module_block. See for example full_package_name.
future_features seems like a similar kind of global state. Can we use that pattern instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self.writer = util.Writer() | ||
self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer) | ||
self.parser_flags = getattr(block_, 'parser_flags', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer seems necessary since everyone accesses parser_flags from the future_features attr?
func Print(f *Frame, args Args, nl bool) *BaseException { | ||
// TODO: Support outputting to files other than stdout and softspace. | ||
// pyPrint encapsulates the logic of the Python print function. | ||
func pyPrint(f *Frame, args Args, sep, end string, file io.Writer) *BaseException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to the end of the file. I generally try to keep private helper functions at the end of the file. Also, maybe call it printImpl.
@trotterdylan That did turn out to be the problem, re a Py3k version of pylint. Hard-coding to a 2.7 version made |
This is great! Thanks for taking the time to push this through! |
This allows the import statement
from __future__ import print_function
to work, by setting a flag on the parser to treat print statement/keyword as a syntax error, and adding support to the Go runtime as a builtin.Addresses #22