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

merge-pyi crashes when input .py file contains the word 'print' (in rare cases). #1156

Closed
mrolle45 opened this issue Mar 21, 2022 · 2 comments
Labels
bug cat: tools pytype-based tools (merge-pyi, annotate-ast, etc.)

Comments

@mrolle45
Copy link

mrolle45 commented Mar 21, 2022

Note: Since I am unfamiliar with pull requests and testing, would someone please test and update the release code with the proposed two changes given below. Thanks.

Edit: there are actually two bugs.

Parsing the input file uses the Python 2 grammar even though the input file is a Python 3 file.
The problem is with a print() call. The word print is a keyword which must be at the start of a statement.
In most situations this is not a problem, because print(x, y, z) appears as a complete expression statement. The parser interprets this as printing with (x, y, z) as a tuple to be printed.
However, merge_pyi crashes when print appears in some other way, including, for example:

  1. print(x, file=f) (this happened to me)
  2. lambda x: print(x)
  3. func(print)

Cause of the bug
The parse is done by the RefactoringTool.refactor_string() method in lib2to3.refactor. This object is constructed with possible options, including "print_function". In order to treat the string in Python 3 syntax, it is necessary for this option to have a true value instead of the default False. pytype.tools.merge_pyi.main() does not do this.

Proposed fix
Simply pass the needed option to RefactoringTool.refactor_string(). No need to worry about a Python 2.7 file being parsed, because pytype -V2.7 is not supported.
N.B.: You might want to be sure that the documentation is clear that Python 3 files are required for all of the tools, and the Python version must be an acceptable value for pytype -V.
Change merge_pyi.py line 923 to read:

  tool = StandaloneRefactoringTool(options={"print_function": True})

instead of

  tool = StandaloneRefactoringTool(options={})

Note that the code does not need changing for:

class StandaloneRefactoringTool(refactor.RefactoringTool):
  """Modified RefactoringTool for running outside the standard 2to3 install."""

  def __init__(self, options):
    self._fixer = None
    super().__init__([], options=options)
    ...
  ...

Second bug
The first bug (above) applies only to the .py input. The second bug applies similarly to the .pyi input.
In my case, I had a class which has a print() method. The parser didn't parse the def print() -> None statement, as it's expecting Python2 syntax.

Proposed fix
Change merge_pyi.py line 102 from

  driver = driver.Driver(pygram.python_grammar, convert=pytree.convert)

to

  driver = driver.Driver(pygram.python_grammar_no_print_statement, convert=pytree.convert)

I made this change in my own installed package, and the tool works correctly now with the cases where it had failed.

@mrolle45
Copy link
Author

I made this proposed fix in my own installed copy of the code, and merge-pyi runs correctly on my files that used to crash.

@rchen152 rchen152 added bug cat: callables functions and decorators labels Mar 21, 2022
@rchen152
Copy link
Contributor

Thanks for providing fixes for this issue and #1153. The proposed fixes look reasonable to me; I'll apply them.

@rchen152 rchen152 added cat: tools pytype-based tools (merge-pyi, annotate-ast, etc.) and removed cat: callables functions and decorators labels Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cat: tools pytype-based tools (merge-pyi, annotate-ast, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants