Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ast module doesn't support optional fields of Parser/Python.asdl #73808

Closed
vstinner opened this issue Feb 22, 2017 · 6 comments
Closed

ast module doesn't support optional fields of Parser/Python.asdl #73808

vstinner opened this issue Feb 22, 2017 · 6 comments
Labels
3.7 (EOL) end of life type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 29622
Nosy @vstinner, @methane, @takluyver, @Carreau
PRs
  • bpo-29622: make AST constructor accepts less than enough number of positional arguments #249
  • bpo-29637: clean docstring only if not None #267
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-02-23.17:48:48.543>
    created_at = <Date 2017-02-22.16:48:32.714>
    labels = ['type-feature', '3.7']
    title = "ast module doesn't support optional fields of Parser/Python.asdl"
    updated_at = <Date 2017-03-24.23:50:02.853>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-03-24.23:50:02.853>
    actor = 'methane'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-23.17:48:48.543>
    closer = 'methane'
    components = []
    creation = <Date 2017-02-22.16:48:32.714>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29622
    keywords = []
    message_count = 6.0
    messages = ['288372', '288375', '288414', '288442', '288443', '290420']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'methane', 'takluyver', 'mbussonn']
    pr_nums = ['249', '267']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29622'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    The _ast.AST constructor requires a tuple with the same number of items than the fields list.

    Example with Raise:

    ASDL: Raise(expr? exc, expr? cause)

    >>> import _ast
    >>> _ast.Raise()
    <_ast.Raise object at 0x7fc2ca7dee90>
    >>> _ast.Raise(1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: Raise constructor takes either 0 or 2 positional arguments
    >>> _ast.Raise(1, 2)
    <_ast.Raise object at 0x7fc2ca7def60>

    The cause field is optional in the ASDL, but required in the _ast module.

    A tradeoff would be to add the minimum and maximum number of fields to _ast.AST.

    This issue should prevent API breakage caused by the new docstring attribute added by issue bpo-29463.

    @vstinner vstinner added 3.7 (EOL) end of life type-feature A feature request or enhancement labels Feb 22, 2017
    @Carreau
    Copy link
    Mannequin

    Carreau mannequin commented Feb 22, 2017

    Duplicating my comment of 29463 who was in a race condition with this issue:

    ----
    thank you for your work on the AST, I know many developers are looking forward to improvement and stabilisation with the hope of having it stable (and documented) in the stdlib at some point.

    The recent change in PR 46 now change (at least) the constructor of ast.Module to take a second mandatory parameter (the docstring).

    I know the ast is autogenerated and "use at your own risk". But IPython for example, use mod = ast.Module([nodes]), with the second mandatory parameter added to Module that make it incompatible with current Python 3.7.
    Well it's long until it's released, and we can patch things, but I'm sure we are not the only one in this case, and we'd like older version of IPython to still be compatible with Python 3.7, so if Module()'s second parameter (the docstring) could be optional, that would be great.

    I would be happy if it raise a deprecation warning that it will be required in the future.

    I'm of course speaking about Module because that's the first error I encountered, but I'm guessing it applies to other changed AST nodes.

    Thanks.
    ---

    @methane
    Copy link
    Member

    methane commented Feb 23, 2017

    AST type doesn't have any information about optional fields. And I don't know it's worth enough to add it.
    When AST is instantiated by keyword arguments, there are no check about absence of some fields.
    I suppose we can just loosen positional arguments too.

    current:

        if (PyTuple_GET_SIZE(args) > 0) {
            if (numfields != PyTuple_GET_SIZE(args)) {
                PyErr_Format(PyExc_TypeError, "%.400s constructor takes %s"
                             "%zd positional argument%s",
                             Py_TYPE(self)->tp_name,
                             numfields == 0 ? "" : "either 0 or ",
                             numfields, numfields == 1 ? "" : "s");

    will be:

        if (PyTuple_GET_SIZE(args) > 0) {
            if (numfields > PyTuple_GET_SIZE(args)) {
                PyErr_Format(PyExc_TypeError, "%.400s constructor takes at most "
                             "%zd positional argument%s",
                             Py_TYPE(self)->tp_name,
                             numfields, numfields == 1 ? "" : "s");

    @methane
    Copy link
    Member

    methane commented Feb 23, 2017

    Now trailing optional fields are optional arguments of AST type.

    @methane
    Copy link
    Member

    methane commented Feb 23, 2017

    @mbussonn With PR 249, "import os" and "%timeit" works fine.

    @methane methane closed this as completed Feb 23, 2017
    @methane
    Copy link
    Member

    methane commented Mar 24, 2017

    New changeset 4c78c52 by INADA Naoki in branch 'master':
    bpo-29622: Make AST constructor to accept less than enough number of positional arguments (GH-249)
    4c78c52

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants