facebookincubator / Bowler

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

Module name in decorator isn't renamed #90

Open orsinium opened 5 years ago

orsinium commented 5 years ago

Input file:

import attr

@attr.s()
class A:
  ...

Code:

import bowler
bowler.Query('tmp.py').select_module('attr').rename('testme').execute(write=True, silent=True)

Output file:

import testme

@attr.s()
class A:
  ...

Attr was renamed in the import statement, but not in the decorator.

thatch commented 4 years ago

A test for this should go near https://github.com/facebookincubator/Bowler/blob/master/bowler/tests/query.py#L187

To see what the lib2to3 pattern would look like, you can use python -m bowler dump [--selector-pattern] <filename>.

The bug is probably a missing piece of the pattern in https://github.com/facebookincubator/Bowler/blob/master/bowler/query.py#L120 that involves {dotted_name}

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

markrofail commented 4 years ago

would like to contribute to this. Can I be assigned?

markrofail commented 3 years ago

I don't think the problem is in {dotted_name}

import testbefore

@testbefore()
class A:
    pass

also fails.

amyreese commented 3 years ago

The selector will need to get more complicated/thorough to catch cases in decorators, since decorators (pre 3.9) have a restricted syntax and use different pieces of the grammar. You can see this by dumping the full parsed CST from your example:

(.venv) jreese@garbodor ~/workspace/bowler master± » cat foo.py
import testbefore

@testbefore()
class A:
    pass

@testbefore.foo()
def func():
    pass
(.venv) jreese@garbodor ~/workspace/bowler master± » bowler dump foo.py
foo.py
[file_input] ''
.  [simple_stmt] ''
.  .  [import_name] ''
.  .  .  [NAME] '' 'import'
.  .  .  [NAME] ' ' 'testbefore'
.  .  [NEWLINE] '' '\n'
.  [decorated] '\n'
.  .  [decorator] '\n'
.  .  .  [AT] '\n' '@'
.  .  .  [NAME] '' 'testbefore'
.  .  .  [LPAR] '' '('
.  .  .  [RPAR] '' ')'
.  .  .  [NEWLINE] '' '\n'
.  .  [classdef] ''
.  .  .  [NAME] '' 'class'
.  .  .  [NAME] ' ' 'A'
.  .  .  [COLON] '' ':'
.  .  .  [suite] ''
.  .  .  .  [NEWLINE] '' '\n'
.  .  .  .  [INDENT] '' '    '
.  .  .  .  [simple_stmt] ''
.  .  .  .  .  [NAME] '' 'pass'
.  .  .  .  .  [NEWLINE] '' '\n'
.  .  .  .  [DEDENT] '\n' ''
.  [decorated] ''
.  .  [decorator] ''
.  .  .  [AT] '' '@'
.  .  .  [dotted_name] ''
.  .  .  .  [NAME] '' 'testbefore'
.  .  .  .  [DOT] '' '.'
.  .  .  .  [NAME] '' 'foo'
.  .  .  [LPAR] '' '('
.  .  .  [RPAR] '' ')'
.  .  .  [NEWLINE] '' '\n'
.  .  [funcdef] ''
.  .  .  [NAME] '' 'def'
.  .  .  [NAME] ' ' 'func'
.  .  .  [parameters] ''
.  .  .  .  [LPAR] '' '('
.  .  .  .  [RPAR] '' ')'
.  .  .  [COLON] '' ':'
.  .  .  [suite] ''
.  .  .  .  [NEWLINE] '' '\n'
.  .  .  .  [INDENT] '' '    '
.  .  .  .  [simple_stmt] ''
.  .  .  .  .  [NAME] '' 'pass'
.  .  .  .  .  [NEWLINE] '' '\n'
.  .  .  .  [DEDENT] '' ''
.  [ENDMARKER] '' ''

Of note is that the selector will need to be expanded to cover use of both the dotted_name and decorator symbols with a single NAME leaf matching the target module. Python grammar is complicated. 😔

amyreese commented 3 years ago

This is also a more general problem with a number of selectors in Bowler where they don't catch 100% of uses of the name, or where being 100% thorough is potentially catching cases where a local could be shadowing the global module name and Bowler doesn't actually know that.