Application crash when parsing multiline non-forward-slashed parenthesized expressions, fail gracefully instead about flask-ext-migrate

[keyan]

Yeah this looks like a baron bug, but I agree with Markus that the failure should be handled better. I am going to change the issue name to reflect the new focus of this ticket.

[untitaker]

We would need an example of a source file that fails with that error.

[ye]

@untitaker thanks for the response. the source is proprietary but I will see what I can do to reproduce a slim-down version of the source that trigger the same error.

And this may very well be a bug on PyCQA/baron

[keyan]

Hey @ye thanks for opening this. Any update on potentially forwarding the troublesome source? Would hate for this issue to be in the wild for others.

[ye]

@keyan my apologies. I will try to get back to you today.

[ye]

Ok, I was able to nail it down to the the fact that PyCQA/baron is unable to parse expressions in multiple lines wrapped in a pair of parenthesis without using forward slash (\) as the line continuation.

Here is the example code that would trigger the same error (note, it's not the exact same GroupingError but close). As you can see below, line 7-8 is the culprit that tripped up PyCQA/baron but Python CLI just runs it fine (the code has been running in product for 2 years so far no complaints). And I can confirm that by commenting out the multiple line expression wrapped in parenthesis, flask_ext_migrate is able to run though the file without a hitch.

$ less test.py 
      1 class A(object):
      2     prop2 = 'This should work OK!'
      3 
      4 class B(object):
      5     prop1 = {'prop': A()}
      6 
      7 x = (B().prop1
      8     ['prop'].prop2)
      9
     10 y =x
     11 
     12 print(y)
$ python3 test.py 
This should work OK!
$ flask_ext_migrate test.py 
Traceback (most recent call last):
  File "/Users/ye/.envs/py35/bin/flask_ext_migrate", line 11, in <module>
    sys.exit(startup())
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/flask_ext_migrate/startup.py", line 21, in startup
    fix(filepath)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/flask_ext_migrate/__init__.py", line 140, in fix
    ast = read_source(input_file)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/flask_ext_migrate/__init__.py", line 7, in read_source
    red = RedBaron(source_code.read())
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/redbaron/redbaron.py", line 36, in __init__
    self.node_list = base_nodes.NodeList.from_fst(baron.parse(source_code), parent=self, on_attribute="root")
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/baron/baron.py", line 49, in parse
    tokens = tokenize(source_code, False)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/baron/baron.py", line 70, in tokenize
    return mark_indentation(inner_group(space_group(_tokenize(group(split(pouet)), print_function))))
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/baron/inner_formatting_grouper.py", line 112, in group
    return list(group_generator(sequence))
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/baron/inner_formatting_grouper.py", line 165, in group_generator
    fail_on_bad_token(iterator.show_next(), debug_file_content, in_grouping_mode)
  File "/Users/ye/.envs/py35/lib/python3.5/site-packages/baron/inner_formatting_grouper.py", line 125, in fail_on_bad_token
    raise GroupingError("Fail to group formatting tokens, here:\n%s <----\n\n'%s' should have been in: %s\n\nCurrent value of 'in_grouping_mode': %s" % (debug_file_content, token, ', '.join(sorted(GROUP_ON)), in_grouping_mode))
baron.inner_formatting_grouper.GroupingError: Fail to group formatting tokens, here:
   4 class B(object):
   5     prop1 = {'prop': A()}
   6 
   7 x = (B().prop1
   8     'prop'].prop2)
   9 
  10 y = x
  11  <----

'('ENDMARKER', '')' should have been in: AMPER, AMPER_EQUAL, AND, AS, BINARY_RAW_STRING, BINARY_STRING, CIRCUMFLEX, CIRCUMFLEX_EQUAL, COLON, COLON, COMMA, DOT, DOUBLE_SLASH, DOUBLE_SLASH_EQUAL, DOUBLE_STAR, DOUBLE_STAR_EQUAL, ELSE, ENDL, EQUAL, EQUAL_EQUAL, FOR, GREATER, GREATER_EQUAL, IF, IMPORT, IN, IS, LEFT_BRACKET, LEFT_PARENTHESIS, LEFT_SHIFT, LEFT_SHIFT_EQUAL, LEFT_SQUARE_BRACKET, LEFT_SQUARE_BRACKET, LESS, LESS_EQUAL, MINUS, MINUS_EQUAL, NOT, NOT_EQUAL, OR, PERCENT, PERCENT_EQUAL, PLUS, PLUS_EQUAL, RAW_STRING, RAW_STRING, RIGHT_BRACKET, RIGHT_PARENTHESIS, RIGHT_SHIFT, RIGHT_SHIFT_EQUAL, RIGHT_SQUARE_BRACKET, SLASH, SLASH_EQUAL, STAR, STAR_EQUAL, STRING, UNICODE_RAW_STRING, UNICODE_RAW_STRING, UNICODE_STRING, UNICODE_STRING, VBAR, VBAR_EQUAL

Current value of 'in_grouping_mode': -1

$ cat test.py | sed 's/\.prop1/.prop1\\/' > test_with_slash.py
$ flask_ext_migrate test_with_slash.py
$ python3 test_with_slash.py
This should work OK!
$ diff test_with_slash.py test.py 
7c7
< x = (B().prop1\
---
> x = (B().prop1

I think I should file a report to PyCQA/baron since this is a bug in their parsing grouping token logic?

[untitaker]

I do think so though ext-migrate could just skip those files, generally fail more gracefully.

[ye]

@untitaker @keyan is there a guideline for contributing code to this repo? If you guys can point me the right directions on exactly what behavior you want this "graceful" skip to be, then I can take a stab at it. Thanks.

PS: I've tweaked the example trigger code to generate the exact Exception that I originally encountered and updated them accordingly.

[keyan]

Ye, thanks for offering to contribute! To answer your questions:

is there a guideline for contributing code to this repo?

There is no explicit contribution guide. In general, try to come to an agreement about how the patch should roughly look before starting to code. And if possible include a test that fails without your fix commit.

exactly what behavior you want this "graceful" skip to be

I don't have strong feelings on this so I'm happy to hear some ideas you have for what this should look like.

My initial thoughts are that because the failure is happening within Baron parsing and Baron parsing is mostly a blackbox for us, perhaps we should:

1. handle exceptions from that function call and return a result which indicates an error e.g. a return code/exception tuple -- (1, exception).
2. check the return value from the read_source() call here and make sure we didn't get an error
3. If there was an error then we'd stop processing the file and return the error back (the call to fix() happens here).

Eventually we could take the info passed back from the fix() call and format some nice output that tells you which files couldn't be converted and maybe even which lines where the issue (it seems like Baron returns that info in the exception). This would happen after we call fix. But we wouldn't have to include that in the first patch.