Implement print_function future feature by paulsmith · Pull Request #23 · google/grumpy

paulsmith · 2017-01-04T23:55:28Z

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

trotterdylan · 2017-01-05T00:05:14Z

compiler/stmt.py

    self.block = block_
    self.writer = util.Writer()
    self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer)
+    self.parser_flags = 0


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.

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.

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

Yep -- that is what I came up with. I should have an update shortly.

trotterdylan · 2017-01-05T00:07:17Z

compiler/stmt.py

+      for alias in node.names:
+        name = alias.name
+        if name in future_features:
+          (flag, implemented) = future_features[name]


Style nit: kill unnecessary parens:

flag, implemented = future_features[name]

trotterdylan · 2017-01-05T00:09:54Z

compiler/stmt.py

+        elif name in redundant_future_features:
+          # no-op
+          pass
+        else:


Kill the no-op elif and replace it with:

elif name not in redundant_future_features:
... raise ParseError

trotterdylan · 2017-01-05T00:10:53Z

compiler/stmt_test.py

        print '123'
        print 'foo', 'bar'""")))

+  def testFutureFeaturePrintFunction(self):


Mind adding a test against one of the unimplemented future features?

trotterdylan · 2017-01-05T00:13:02Z

runtime/builtin_types.go

+			}
+			end = kwend.Value()
+		case "file":
+			// TODO: need to map Python sys.stdout, sys.stderr etc. to


Please wrap comments at 80 chars.

trotterdylan · 2017-01-05T00:15:41Z

runtime/builtin_types.go

+			// to recover to the file descriptor probably
+		}
+	}
+	for i, arg := range args {


Maybe generalize grumpy.Print() instead of duplicating the logic here.

trotterdylan · 2017-01-05T00:17:35Z

runtime/builtin_types.go

 	return NewInt(result).ToObject(), nil
 }

+func builtinPrint(f *Frame, args Args, kwargs KWArgs) (*Object, *BaseException) {


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

trotterdylan

This is awesome, thanks for taking this on!

kmod · 2017-01-05T01:06:06Z

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.

trotterdylan

This all looks great. One last suggestion.

trotterdylan · 2017-01-05T18:15:25Z

compiler/stmt.py

    self.block = block_
    self.writer = util.Writer()
    self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer)
+    self.parser_flags = 0


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

trotterdylan

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.

paulsmith · 2017-01-06T04:10:56Z

@trotterdylan I cleaned up some pylint errors from the code I added, but when I run make precommit, there are a number of errors for other parts of the code base I didn't edit. I'm not sure if there is something in my local environment that's causing pylint to not be configured correctly or something, but I'm not sure I can make the precommit pass cleanly as is.

trotterdylan · 2017-01-06T17:47:05Z

compiler/stmt.py

+
+  def visit(self, node):
+    root = node
+    # If this is the module node, do an initial pass through the module body's


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.

Sure, makes sense. I'll put something together.

trotterdylan · 2017-01-06T17:48:03Z

@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 · 2017-01-06T19:04:20Z

@trotterdylan Re lint errors: https://gist.github.com/paulsmith/683cc5b5fda04a556f07a5ef6a4042f2

trotterdylan · 2017-01-06T21:49:49Z

@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.

paulsmith · 2017-01-07T22:38:22Z

@trotterdylan PTAL -- I refactored the future pass logic into a visit_future function, which returns an object that contains relevant future info and is passed to the StatementVisitor via ModuleBlock.

trotterdylan

Awesome. This is coming along great. A few more suggestions.

trotterdylan · 2017-01-07T23:23:40Z

compiler/stmt.py

+
+
+def import_from_future(node):
+  """processes a future import statement, returning set of flags it defines."""


Capitalize "processes..."

trotterdylan · 2017-01-07T23:29:18Z

compiler/stmt.py


  def __init__(self, block_):
    self.block = block_
+    self.future_features = self.block.future_features or FutureFeatures()


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?

trotterdylan · 2017-01-07T23:36:33Z

compiler/stmt.py

+    self.future_features = self.block.future_features or FutureFeatures()
    self.writer = util.Writer()
    self.expr_visitor = expr_visitor.ExprVisitor(self.block, self.writer)
+    self.parser_flags = getattr(block_, 'parser_flags', 0)


This no longer seems necessary since everyone accesses parser_flags from the future_features attr?

trotterdylan · 2017-01-07T23:45:05Z

runtime/core.go

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


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.

paulsmith · 2017-01-08T18:22:01Z

@trotterdylan That did turn out to be the problem, re a Py3k version of pylint. Hard-coding to a 2.7 version made precommit clean.

trotterdylan · 2017-01-09T05:11:10Z

This is great! Thanks for taking the time to push this through!

trotterdylan reviewed Jan 5, 2017

View reviewed changes

trotterdylan suggested changes Jan 5, 2017

View reviewed changes

trotterdylan reviewed Jan 5, 2017

View reviewed changes

kofalt mentioned this pull request Jan 5, 2017

Make Grumpy compatible with standard python projects #43

Open

5 tasks

paulsmith added 3 commits January 5, 2017 21:32

Implement print_function future feature

51d6697

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.

Changes based on comments on PR

0c6b174

Do initial pass through module statements to enforce future import rules

f761416

paulsmith force-pushed the print_function branch from 45a5d9a to f761416 Compare January 6, 2017 03:48

pylint fixes

335c7b6

trotterdylan reviewed Jan 6, 2017

View reviewed changes

paulsmith added 2 commits January 7, 2017 16:06

Refactor visit_future to standalone function

e5f4310

make pylint happy

2174462

trotterdylan suggested changes Jan 7, 2017

View reviewed changes

Tweaks per PR comments

6238a39

Remove errant log statement

709186c

trotterdylan approved these changes Jan 9, 2017

View reviewed changes

trotterdylan merged commit 7e86708 into google:master Jan 9, 2017

trotterdylan mentioned this pull request Jan 10, 2017

from __future__ import print_function #22

Closed

paulsmith deleted the print_function branch October 1, 2017 21:09



		def import_from_future(node):
		"""processes a future import statement, returning set of flags it defines."""

Conversation

paulsmith commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

kmod commented Jan 5, 2017

Uh oh!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

paulsmith commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotterdylan commented Jan 6, 2017

Uh oh!

paulsmith commented Jan 6, 2017

Uh oh!

trotterdylan commented Jan 6, 2017

Uh oh!

paulsmith commented Jan 7, 2017

Uh oh!

trotterdylan left a comment

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!

paulsmith commented Jan 8, 2017

Uh oh!

trotterdylan commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulsmith commented Jan 4, 2017 •

edited

Loading