Line range not indenting correctly

[hayd]

Promoting this to an issue.

$ /Users/myint/projects/autopep8/autopep8.py --max-line-length=79 --ignore= /opt/local/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/inspect.py --aggressive --aggressive --aggressive --aggressive --range 550 610 -d

--- original//opt/local/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/inspect.py
+++ fixed//opt/local/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/inspect.py
@@ -547,56 +547,63 @@
     filename = os.path.basename(path)
     suffixes = [(-len(suffix), suffix, mode, mtype)
                     for suffix, mode, mtype in imp.get_suffixes()]
-    suffixes.sort() # try longest suffixes first, in case they overlap
+    suffixes.sort()  # try longest suffixes first, in case they overlap
     for neglen, suffix, mode, mtype in suffixes:
         if filename[neglen:] == suffix:
             return ModuleInfo(filename[:neglen], suffix, mode, mtype)
+

 def getmodulename(path):
     """Return the module name for a given file, or None."""
     fname = os.path.basename(path)
     # Check for paths that look like an actual module file
-    suffixes = [(-len(suffix), suffix)
-                    for suffix in importlib.machinery.all_suffixes()]
-    suffixes.sort() # try longest suffixes first, in case they overlap
+    suffixes = sorted([(-len(suffix), suffix)
+                       for suffix in importlib.machinery.all_suffixes()])
     for neglen, suffix in suffixes:
         if fname.endswith(suffix):
             return fname[:neglen]
     return None

+
 def getsourcefile(object):
     """Return the filename that can be used to locate an object's source.
     Return None if no way can be identified to get the source.
-    """
-    filename = getfile(object)
+        """
+        filename = getfile(object)
     all_bytecode_suffixes = importlib.machinery.DEBUG_BYTECODE_SUFFIXES[:]
     all_bytecode_suffixes += importlib.machinery.OPTIMIZED_BYTECODE_SUFFIXES[:]
     if any(filename.endswith(s) for s in all_bytecode_suffixes):
         filename = (os.path.splitext(filename)[0] +
                     importlib.machinery.SOURCE_SUFFIXES[0])
-    elif any(filename.endswith(s) for s in
+        elif any(filename.endswith(s) for s in
                  importlib.machinery.EXTENSION_SUFFIXES):
-        return None
+            return None
     if os.path.exists(filename):
         return filename
     # only return a non-existent filename if the module has a PEP 302 loader
-    if getattr(getmodule(object, filename), '__loader__', None) is not None:
+        if getattr(
+                getmodule(
+                    object,
+                    filename),
+                '__loader__',
+                None) is not None:
         return filename
     # or it is in the linecache
-    if filename in linecache.cache:
+        if filename in linecache.cache:
         return filename
+

 def getabsfile(object, _filename=None):
     """Return an absolute path to the source or compiled file for an object.

     The idea is for each object to have a unique origin, so this routine
-    normalizes the result as much as possible."""
-    if _filename is None:
-        _filename = getsourcefile(object) or getfile(object)
+        normalizes the result as much as possible."""
+        if _filename is None:
+            _filename = getsourcefile(object) or getfile(object)
     return os.path.normcase(os.path.abspath(_filename))
-
-modulesbyfile = {}
+    lesbyfile = {}
 _filesbymodname = {}
+

 def getmodule(object, _filename=None):
     """Return the module an object was defined in, or None if not found."""

[myint]

The sorted change is due to the lib2to3 based fixer, which is valid. But in any case, it looks like the --range code fails possibly due to the three-line sort()-related lines being changed into two lines. Maybe the code does not handle cases where the number of lines gets reduced?

I'm disabling these buggy cases for now. Please feel free to revisit this with a pull request that fixes the issue and re-enables the --range code for the inspect.py case.

[hayd]

I put it up as a gist.

So you can test with:

from autopep8 import fix_code, parse_args
import requests
code = requests.get("https://gist.githubusercontent.com/hayd/2d4a22e1854683c9c72d/raw/1641824237cba91fa6b6232786f77b7a98a93c6c/inspect.py").text
args = parse_args(['']) # I'm not sure what you're suppose to here, I often get SystemError
args.line_range=[550, 610]
fix_code(code, args)

#you can see the diff with
from pep8radius.diff import get_diff, print_diff
args.line_range=[550, 610]
print_diff(get_diff(a, autopep8.fix_code(a, args), "inspect"))

---

Aside: I thought this used to work:

In [11]: autopep8.parse_args(['--range 1 10'])
Out[11]: Namespace(aggressive=0, diff=False, exclude=[], experimental=False, files=[u'--range 1 10'], global_config=u'/Users/andy/.config/pep8', ignore=set([u'E24']), ignore_local_config=False, in_place=False, indent_size=4, jobs=1, line_range=None, list_fixes=False, max_line_length=79, pep8_passes=-1, recursive=False, select=u'', verbose=0)

[myint]

I think the first argument to parse_args() (sys.argv) would be the program name. I think it needs a last argument, which is the filename.

But in any case, I prefer the command line:

$ ./autopep8.py --range 550 610 test/inspect_example.py -d -a

[hayd]

Should things like the following reduce the number of lines?

autopep8.fix_code("""
A = [1, 2,

3, 4]""")

I struggle to find examples where the lines are reduced.

I'm kinda surprised that this would be an issue as the lines are being iterated in reverse (IIRC), and that there isn't a smaller example to show this error. I'm not sure a good way to debug this right now.

The len()==.. does not solve it as you can get an edge case where these would be equal (where some lines are also added). That is not a fix.

[myint]

Here is a smaller example:

a = []
a.sort()
if True:
    a = []
    a.sort()
else:
    pass

Without my workaround:

$ ./autopep8.py -d foo.py -a --range 1 5

--- original/foo.py
+++ fixed/foo.py
@@ -1,7 +1,6 @@
-a = []
-a.sort()
+a = sorted([])
 if True:
     a = []
     a.sort()
-else:
+    :
     pass

Note the loss of the else. (This works fine without the --range.)

I don't quite follow about the edge case you refer to. Could you expand? (My reasoning with the length check is that something about the code that comes afterwards goes haywire if those two length are not equal. I'm thinking that the actual contents don't actually matter. I may very well could be wrong though as I didn't write the code.)

Thanks

[hayd]

Excellent! Thanks for the example, will check it out! (Hmmm weirdly I can't reproduce this!!)

RE the line numbers if you have something like:

if False:
    pass
a = []
a.sort()
if True:
    # 2343
    a = []
    a.sort()
else:
    # 2343
    'something really really really really really really really long' + 'something or other'
    pass

That is, the returned subsource and original subsource will be the same length (even though it hits the criteria we think may trigger this). Like I say, I can't reproduce! :(

[myint]

What version of autopep8 are you using? And are you sure, you don't have my workaround enabled (grep 'Avoid buggy case' autopep8.py)?

Also, you might want to double check the range numbers you are using as I updated my comment a few minutes after initially submitting it.

[myint]

By the way, my workaround works fine for the case you show above.

By the way, your example works fine with or without my workaround:

$  ./autopep8.py -d bar.py -a --range 1 11

I don't get any loss like I do when the numbers of lines goes down. So I think the workaround is correct in checking for equality of number of lines and ignoring content.

[hayd]

Just running verbose I see it has an E112 (comment is not indented correctly)... but there are no comments!! I don't see how but it looks like fix_e112 removes the left space which then bubbles up to deindenting (as the deindent code assumes the indent to be the same - which it should be at that stage!!!).

 % ./autopep8.py --diff ~/autopep8/foo.py -aaaa --range 1 5 -vvv
[file:/Users/andy/autopep8/foo.py]
--->  Applying local fix for E265
--->  Applying local fix for W602
--->  Applying local fix for E265
--->  Applying local fix for W602
--->  1 issue(s) to fix {'E112': set([6])}
--- original//Users/andy/autopep8/foo.py
+++ fixed//Users/andy/autopep8/foo.py
@@ -1,7 +1,6 @@
-a = []
-a.sort()
+a = sorted([])
 if True:
     a = []
     a.sort()
-else:
+    :
     pass

I haven't a fix yet... but hopefully I can get one.

It's very finicky, the fact you can add a couple of lines at the end and it completely changes the behaviour (no e112 is found) is weird. Just because I can't come up with an example doesn't mean there isn;t one though!!

[myint]

Memo to myself: I should run ./acid.py --random-range -a against any proposed fix before removing the workaround permanently.

[hayd]

@myint do you know what function/where this sorted replacement is happening? An alternative soln, which would fix this particular case, would be to replace it with a blank:

['', 'sorted([])\n', ...]

(so the length of source remains the same with the replacement).

[myint]

I don't think that would be a good solution. The sorted() fix is happening in lib2to3. And there are fixers in autopep8 whose whole purpose is to remove lines.

[myint]

I actually found an example that breaks even without aggressive:

$ python2.7 test/acid.py --line-range 289 925 test/vectors_example.py

[hayd]

I'm pretty sure this is a logic bug from assuming there are no line changes when doing lib2to3, what I think you can do is immediately end the pass if the line changes after lib2to3 (rather than completely abort).... alternatively it may be possible to fix up the borked numbers which cause this.

[myint]

It seems to fail even if all fixers are disabled.

$ python2.7 test/acid.py --line-range 289 925 test/vectors_example.py -vvvv --ignore=E,W
--->  0 issue(s) to fix {}
autopep8 broke /Users/myint/projects/autopep8/test/vectors_example.py
unexpected indent (<string>, line 818)

It looks like it is indenting things for some reason:

--- original//Users/myint/projects/autopep8/test/vectors_example.py
+++ fixed//Users/myint/projects/autopep8/test/vectors_example.py
@@ -297,8 +297,8 @@

     def magnitude(self, value=None):
         '''Find the magnitude (spatial length) of this vector.
-        With a value, return a vector with same direction but of given length.
-        '''
+            With a value, return a vector with same direction but of given length.
+            '''
         mag = self.__mag()
         if value is None: return mag
         if zero(mag):
@@ -459,9 +459,9 @@
             sx,sy,sz,sw = self._v
             ox,oy,oz,ow = other._v
             self._v = [ sw*ox + sx*ow + sy*oz - sz*oy,
-                        sw*oy + sy*ow + sz*ox - sx*oz,
-                        sw*oz + sz*ow + sx*oy - sy*ox,
-                        sw*ow - sx*ox - sy*oy - sz*oz ]
+                            sw*oy + sy*ow + sz*ox - sx*oz,
+                            sw*oz + sz*ow + sx*oy - sy*ox,
+                            sw*ow - sx*ox - sy*oy - sz*oz ]
         else:
             V.__imul__(self, other)
         return self
@@ -483,10 +483,10 @@

     def __init__(self, *args):
         '''Constructs a new 4x4 matrix.
-        If no arguments are given, an identity matrix is constructed.
-        Any combination of V vectors, tuples, lists or scalars may be given,
-        but taken together in order, they must have 16 number values total.
-        '''
+            If no arguments are given, an identity matrix is constructed.
+            Any combination of V vectors, tuples, lists or scalars may be given,
+            but taken together in order, they must have 16 number values total.
+            '''
         # no args gives identity matrix
         # 16 scalars collapsed from any combination of lists, tuples, vectors
         if not args:
@@ -509,21 +509,21 @@
             if axis in ('x','X'):
                 c = math.cos(theta) ; s = math.sin(theta)
                 return cls( [  1,  0,  0,  0,
-                               0,  c, -s,  0,
-                               0,  s,  c,  0,
-                               0,  0,  0,  1 ] )
+                                   0,  c, -s,  0,
+                                   0,  s,  c,  0,
+                                   0,  0,  0,  1 ] )
             if axis in ('y','Y'):
                 c = math.cos(theta) ; s = math.sin(theta)
                 return cls( [  c,  0,  s,  0,
-                               0,  1,  0,  0,
-                              -s,  0,  c,  0,
-                               0,  0,  0,  1 ] )
+                                   0,  1,  0,  0,
+                                  -s,  0,  c,  0,
+                                   0,  0,  0,  1 ] )
             if axis in ('z','Z'):
                 c = math.cos(theta) ; s = math.sin(theta)
                 return cls( [  c, -s,  0,  0,
-                               s,  c,  0,  0,
-                               0,  0,  1,  0,
-                               0,  0,  0,  1 ] )
+                                   s,  c,  0,  0,
+                                   0,  0,  1,  0,
+                                   0,  0,  0,  1 ] )
         if isinstance(axis, V):
             axis = Q.rotate(axis, theta)
         if isinstance(axis, Q):
@@ -541,9 +541,9 @@
         e =     2*(xy + zw) ; f = 1 - 2*(xx + zz) ; g =     2*(yz - xw)
         i =     2*(xz - yw) ; j =     2*(yz + xw) ; k = 1 - 2*(xx + yy)
         return cls( [ a, b, c, 0,
-                      e, f, g, 0,
-                      i, j, k, 0,
-                      0, 0, 0, 1 ] )
+                          e, f, g, 0,
+                          i, j, k, 0,
+                          0, 0, 0, 1 ] )

     @classmethod
     def scale(cls, factor):
@@ -568,9 +568,9 @@
         (x,y,z,w) = normal.normalize()._v
         (n2x,n2y,n2z) = (-2*x, -2*y, -2*z)
         return cls( [ 1+n2x*x,   n2x*y,   n2x*z, 0,
-                        n2y*x, 1+n2y*y,   n2y*z, 0,
-                        n2z*x,   n2z*y, 1+n2z*z, 0,
-                          d*x,     d*y,     d*y, 1  ] )
+                            n2y*x, 1+n2y*y,   n2y*z, 0,
+                            n2z*x,   n2z*y, 1+n2z*z, 0,
+                              d*x,     d*y,     d*y, 1  ] )

     @classmethod
     def shear(cls, amount):
@@ -584,9 +584,9 @@
     def frustrum(cls, l, r, b, t, n, f):
         rl = 1/(r-l) ; tb = 1/(t-b) ; fn = 1/(f-n)
         return cls( [  2*n*rl,  0,       (r+l)*rl,  0,
-                       0,       2*n*tb,  0,         0,
-                       0,       0,      -(f+n)*fn, -2*f*n*fn,
-                       0,       0,      -1,         0         ] )
+                           0,       2*n*tb,  0,         0,
+                           0,       0,      -(f+n)*fn, -2*f*n*fn,
+                           0,       0,      -1,         0         ] )

     @classmethod
     def perspective(cls, yfov, aspect, n, f):
@@ -609,17 +609,17 @@

     def __getitem__(self, rc):
         '''Returns a single element of the matrix.
-        May index 0-15, or with tuples of (row,column) 0-3 each.
-        Indexing goes across first, so m[3] is m[0,3] and m[7] is m[1,3].
-        '''
+            May index 0-15, or with tuples of (row,column) 0-3 each.
+            Indexing goes across first, so m[3] is m[0,3] and m[7] is m[1,3].
+            '''
         if not isinstance(rc, tuple): return V.__getitem__(self, rc)
         return self._v[rc[0]*4+rc[1]]

     def __setitem__(self, rc, value):
         '''Injects a single element into the matrix.
-        May index 0-15, or with tuples of (row,column) 0-3 each.
-        Indexing goes across first, so m[3] is m[0,3] and m[7] is m[1,3].
-        '''
+            May index 0-15, or with tuples of (row,column) 0-3 each.
+            Indexing goes across first, so m[3] is m[0,3] and m[7] is m[1,3].
+            '''
         if not isinstance(rc, tuple): return V.__getitem__(self, rc)
         self._v[rc[0]*4+rc[1]] = float(value)

@@ -628,8 +628,8 @@

     def row(self, r, v=None):
         '''Returns or replaces a vector representing a row of the matrix.
-        Rows are counted 0-3. If given, new vector must be four numbers.
-        '''
+            Rows are counted 0-3. If given, new vector must be four numbers.
+            '''
         if r < 0 or r > 3: raise IndexError, 'row index out of range'
         if v is None: return V(self._v[r*4:(r+1)*4])
         e = v
@@ -640,8 +640,8 @@

     def col(self, c, v=None):
         '''Returns or replaces a vector representing a column of the matrix.
-        Columns are counted 0-3. If given, new vector must be four numbers.
-        '''
+            Columns are counted 0-3. If given, new vector must be four numbers.
+            '''
         if c < 0 or c > 3: raise IndexError, 'column index out of range'
         if v is None: return V([ self._v[c+4*i] for i in range(4) ])
         e = v
@@ -653,17 +653,17 @@
     def translation(self):
         '''Extracts the translation component from this matrix.'''
         (a,b,c,d,
-         e,f,g,h,
-         i,j,k,l,
-         m,n,o,p) = self._v
+             e,f,g,h,
+             i,j,k,l,
+             m,n,o,p) = self._v
         return V(m,n,o)

     def rotation(self):
         '''Extracts Euler angles of rotation from this matrix.
-        This attempts to find alternate rotations in case of gimbal lock,
-        but all of the usual problems with Euler angles apply here.
-        All Euler angles are in radians.
-        '''
+            This attempts to find alternate rotations in case of gimbal lock,
+            but all of the usual problems with Euler angles apply here.
+            All Euler angles are in radians.
+            '''
         (a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p) = self._v
         rotY = D = math.asin(c)
         C = math.cos(rotY)
@@ -724,15 +724,15 @@
         dr = 1.0 / (a*d00 - b*d01 + c*d02 - d*d03)
         # inverse
         return self.__class__( [  d00*dr, -d10*dr,  d20*dr, -d30*dr,
-                                 -d01*dr,  d11*dr, -d21*dr,  d31*dr,
-                                  d02*dr, -d12*dr,  d22*dr, -d32*dr,
-                                 -d03*dr,  d13*dr, -d23*dr,  d33*dr ] )
+                                     -d01*dr,  d11*dr, -d21*dr,  d31*dr,
+                                      d02*dr, -d12*dr,  d22*dr, -d32*dr,
+                                     -d03*dr,  d13*dr, -d23*dr,  d33*dr ] )

     def transpose(self):
         return M( [ self._v[i] for i in [ 0, 4,  8, 12,
-                                          1, 5,  9, 13,
-                                          2, 6, 10, 14,
-                                          3, 7, 11, 15 ] ] )
+                                              1, 5,  9, 13,
+                                              2, 6, 10, 14,
+                                              3, 7, 11, 15 ] ] )

     def __mul__(self, other):
         # called in case of m *m, m * v, m * s
@@ -744,9 +744,9 @@
                 a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p = self._v
                 X,Y,Z,W = other._v
                 return V( a*X + b*Y + c*Z + d*W,
-                          e*X + f*Y + g*Z + h*W,
-                          i*X + j*Y + k*Z + l*W,
-                          m*X + n*Y + o*Z + p*W )
+                              e*X + f*Y + g*Z + h*W,
+                              i*X + j*Y + k*Z + l*W,
+                              m*X + n*Y + o*Z + p*W )
         return self.__class__(self).__imul__(other)

     def __rmul__(self, other):
@@ -759,9 +759,9 @@
                 A,B,C,D,E,F,G,H,I,J,K,L,M,N,O,P = self._v
                 x,y,z,w = other._v
                 return V( x*A + y*E + z*I + w*M,
-                          x*B + y*F + z*J + w*N,
-                          x*C + y*G + z*K + w*O,
-                          x*D + y*H + z*L + w*P )
+                              x*B + y*F + z*J + w*N,
+                              x*C + y*G + z*K + w*O,
+                              x*D + y*H + z*L + w*P )
         return self.__class__(self).__imul__(other)

     def __imul__(self, other):
@@ -774,21 +774,21 @@
             s0,s1,s2,s3,s4,s5,s6,s7,s8,s9,sA,sB,sC,sD,sE,sF = self._v
             o0,o1,o2,o3,o4,o5,o6,o7,o8,o9,oA,oB,oC,oD,oE,oF = other._v
             self._v = [ o0*s0 + o1*s4 + o2*s8 + o3*sC, #
-                        o0*s1 + o1*s5 + o2*s9 + o3*sD,
-                        o0*s2 + o1*s6 + o2*sA + o3*sE,
-                        o0*s3 + o1*s7 + o2*sB + o3*sF,
-                        o4*s0 + o5*s4 + o6*s8 + o7*sC, #
-                        o4*s1 + o5*s5 + o6*s9 + o7*sD,
-                        o4*s2 + o5*s6 + o6*sA + o7*sE,
-                        o4*s3 + o5*s7 + o6*sB + o7*sF,
-                        o8*s0 + o9*s4 + oA*s8 + oB*sC, #
-                        o8*s1 + o9*s5 + oA*s9 + oB*sD,
-                        o8*s2 + o9*s6 + oA*sA + oB*sE,
-                        o8*s3 + o9*s7 + oA*sB + oB*sF,
-                        oC*s0 + oD*s4 + oE*s8 + oF*sC, #
-                        oC*s1 + oD*s5 + oE*s9 + oF*sD,
-                        oC*s2 + oD*s6 + oE*sA + oF*sE,
-                        oC*s3 + oD*s7 + oE*sB + oF*sF ]
+                            o0*s1 + o1*s5 + o2*s9 + o3*sD,
+                            o0*s2 + o1*s6 + o2*sA + o3*sE,
+                            o0*s3 + o1*s7 + o2*sB + o3*sF,
+                            o4*s0 + o5*s4 + o6*s8 + o7*sC, #
+                            o4*s1 + o5*s5 + o6*s9 + o7*sD,
+                            o4*s2 + o5*s6 + o6*sA + o7*sE,
+                            o4*s3 + o5*s7 + o6*sB + o7*sF,
+                            o8*s0 + o9*s4 + oA*s8 + oB*sC, #
+                            o8*s1 + o9*s5 + oA*s9 + oB*sD,
+                            o8*s2 + o9*s6 + oA*sA + oB*sE,
+                            o8*s3 + o9*s7 + oA*sB + oB*sF,
+                            oC*s0 + oD*s4 + oE*s8 + oF*sC, #
+                            oC*s1 + oD*s5 + oE*s9 + oF*sD,
+                            oC*s2 + oD*s6 + oE*sA + oF*sE,
+                            oC*s3 + oD*s7 + oE*sB + oF*sF ]
         else:
             raise ValueError, 'multiply by 4d matrix or 4d vector or scalar'
         return self
@@ -815,7 +815,7 @@
     t = math.atan2(v[1], v[0])
     if t < 0:
    return t + 2.*math.pi
-    return t
+      return t

 def quangle(a, b):
     '''Find a quaternion that rotates one 3d vector to parallel another.'''

[hayd]

@myint Why's this closed and the tests deleted?? I'm still hoping to fix this up before autopep8 release!

[myint]

Whoops, I thought activity on this had died. (And I didn't like the low coverage :).) t should have pinged. Feel free to undo it in the fix pull request.

Thanks