Menu

#9 Fix _trim_arity swallowing user TypeErrors

Unstable (example)
closed
5
2016-08-04
2016-01-16
No

_trim_arity() is notorious for masking TypeErrors in user parse actions (see eg [#48] and [#79]). The first, straightforward patch (arity.patch) uses the inspect module to determine the proper arity.

_trim_arity() now operates in three steps:

  1. (unchanged) if the parse action is a "single-argument builtin", call it with tokens only;

  2. if the function has variable positional arguments, use it directly;

  3. count the number n of positional arguments and use min(n, maxargs) to choose how many arguments to use. Ie args being a tuple (string, location, tokens), if arity is:

    • 0: use args[3:] == []
    • 1: use args[2:] == [tokens]
    • 2: use args[1:] == [location, tokens]
    • 3 or more: use args[0:] == [string, location, tokens]

Capping to maxargs=3 means that if the parse action takes more than 3 arguments, the user will get a helpful exception stating that his callback takes too many arguments.

A unittest script has been attached and may be run with PYTHONPATH=pyparsing/src pythonX test-arity.py. The script has two test cases: one for regular cases, and one which features:

  • a TypeError raised by the user's parse action;
  • a TypeError raised when the user's parse action has invalid arity.

Note that this second test case fails with the current version of pyparsing: the resulting errors do not correspond to what (I believe) should be displayed.

The code has been tested with Python 2.7.9 and Python 3.4.2.

Note that inspect.getargspec is deprecated since Python 3.0; Python 2, however, does not feature the recommended replacement.

pyparsing already seems to branch on occasions depending the major version (if PY_3:). I figured it could not hurt to provide a second, somewhat longer-winded patch (arity-deprecated.patch) with a Python 3-specific function.

If I made any mishap submitting this patch (forget to run a test suite, wrong diff flags, etc), I will gladly fix them. Thank you for pyparsing.

3 Attachments

Related

Bugs: #48
Bugs: #79

Discussion

  • Kévin Le Gouguec

    And of course, the second after I send this patch, I finally see the repository's unit tests, run them, and find that now they are failing.

    I will investigate these errors soon and see if I can do something about it.

    EDIT: My approach was indeed quite naive. inspect.getargspec and inspect.signature have trouble handling things like types, arbitrary objects with a __call__ method, and functools.partial objects.

    I think all of those edge cases can be solved, but I do not know enough about Python's introspection features to be sure (yet). I get the feeling that if a comprehensive solution to the "how many arguments does this thing take?" problem exists, it will need distinct logic for Python 2 and Python 3.

    I will work on improving this patch. Hopefully, even if I cannot make it comprehensive enough to have it included in pyparsing, I will have learned some things along the way.

    In the meantime, sorry for the noise.

     

    Last edit: Kévin Le Gouguec 2016-01-16
  • Paul McGuire

    Paul McGuire - 2016-01-18

    Kevin - thanks for taking a run at this annoying aspect of pyparsing - I finally got to looking at it again myself, and I think I have a reasonable solution that handles the edge cases. I've checked this into SVN, feel free to give it a try.

    Thanks,
    -- Paul

     
    • Kévin Le Gouguec

      Paul,

      Thank you for this fix! It does successfully let user exceptions through.

      I made some progress on the "how many arguments does this thing take?" problem, but I would not call my current iteration "comprehensive" yet. I am attaching a patch (against 312) for posterity, but at this point I do not think it is worth applying it and the issue can be closed, as far as I am concerned.

      Some highlights from this patch:

      • no regression according to unitTests.py!
      • no support for functools.partial yet (trivial to add for Python 3, not so for Python 2);
      • consequently, pyparsing.replaceWith has been re-implemented with a lambda.

      I would suggest that maybe a more context-specific and human-readable token (eg PYPARSING-ARITY-TESTING-TOKEN) would be both more legible and less collision-prone than some pseudo-random punctuation, but that is a very minor nitpick. Again, thank you for this fix.

       
  • Paul McGuire

    Paul McGuire - 2016-08-04
    • status: open --> closed
    • assigned_to: Paul McGuire
     
  • Paul McGuire

    Paul McGuire - 2016-08-04

    Closed with submitter's blessing.

     

Log in to post a comment.