facebookincubator / Bowler

Safe code refactoring for modern Python.
https://pybowler.io/
MIT License
1.53k stars 89 forks source link

Crash on functions with certain typehinting #129

Open kmantel opened 4 years ago

kmantel commented 4 years ago

Simple typehints seem to be supported, but others cause bowler to crash. For example,

foo.py:

import typing

def foo(bar: typing.List[str]):
    pass

Running

bowler.Query('foo.py').select_function('foo').add_argument('baz', 1).diff()

Produces

Skipping foo.py: failed to transform because 'Node' object has no attribute 'value'
Traceback (most recent call last):
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/tool.py", line 209, in refactor_queue
    hunks = self.refactor_file(filename)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/tool.py", line 171, in refactor_file
    tree = self.refactor_string(input, filename)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/fissix/refactor.py", line 377, in refactor_string
    self.refactor_tree(tree, name)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/fissix/refactor.py", line 417, in refactor_tree
    self.traverse_by(self.bmi_post_order_heads, tree.post_order())
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/fissix/refactor.py", line 493, in traverse_by
    new = fixer.transform(node, results)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/query.py", line 962, in transform
    returned_node = callback(node, capture, filename)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/query.py", line 746, in add_argument_transform
    spec = FunctionSpec.build(node, capture)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/imr.py", line 214, in build
    arguments = FunctionArgument.build_list(args, is_def)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/imr.py", line 89, in build_list
    arg = cls.build(leaf, is_def)
File "~/.pyenv/versions/pnl368/lib/python3.6/site-packages/bowler/imr.py", line 50, in build
    kwargs["annotation"] = leaf.children[-1].value
AttributeError: 'Node' object has no attribute 'value'
AttributeError: 'Node' object has no attribute 'value'
<bowler.query.Query object at 0x7fe8de0d8860>

While replacing typing.List[str] with just str produces the expected result

turbaszek commented 3 years ago

With @olchas we encountered the same issue. How can we help to solve this bug?

thatch commented 3 years ago

@jreese is just checking whether leaf.children[-1] is a Leaf (->store .value) or Node (store the whole thing?) enough of a fix here, or are there downstream dependencies on this that would break. I suspect any type checking we might have is disabled by using kwargs.

thatch commented 3 years ago
  1. Store the entire node in kwargs["annotation"] seems the way to go. There may be corresponding changes where that's pulled back out
  2. Add a test case near https://github.com/facebookincubator/Bowler/blob/master/bowler/tests/query.py#L297 that includes a type hint of typing.List[str] in the first post
  3. Make sure the tests pass
  4. Submit a PR.

Some getting started docs are at https://github.com/facebookincubator/Bowler/blob/master/CONTRIBUTING.md

MatthewMarroquin commented 3 years ago

@thatch Taking a look at this; first time working with unit tests any documentation on this?