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

Make ast.dump() not output optional default fields #80468

Closed
serhiy-storchaka opened this issue Mar 14, 2019 · 17 comments
Closed

Make ast.dump() not output optional default fields #80468

serhiy-storchaka opened this issue Mar 14, 2019 · 17 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 36287
Nosy @benjaminp, @serhiy-storchaka, @1st1, @ilevkivskyi, @eamanu, @isidentical
PRs
  • [WIP] bpo-36287: Ignoring optional field. #12328
  • bpo-36287: Make ast.dump() not output optional fields and attributes. #18843
  • Dependencies
  • bpo-39981: Default values for AST Nodes
  • Files
  • ast.patch
  • 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 = None
    created_at = <Date 2019-03-14.09:48:05.485>
    labels = ['type-feature', 'library', '3.9']
    title = 'Make ast.dump() not output optional default fields'
    updated_at = <Date 2020-04-08.14:28:27.407>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2020-04-08.14:28:27.407>
    actor = 'BTaskaya'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2019-03-14.09:48:05.485>
    creator = 'serhiy.storchaka'
    dependencies = ['39981']
    files = ['48975']
    hgrepos = []
    issue_num = 36287
    keywords = ['patch']
    message_count = 16.0
    messages = ['337907', '337922', '337955', '338030', '338033', '338055', '356925', '356935', '357012', '363640', '363641', '363642', '363775', '364305', '364326', '365987']
    nosy_count = 6.0
    nosy_names = ['benjamin.peterson', 'serhiy.storchaka', 'yselivanov', 'levkivskyi', 'eamanu', 'BTaskaya']
    pr_nums = ['12328', '18843']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue36287'
    versions = ['Python 3.9']

    @serhiy-storchaka
    Copy link
    Member Author

    Currently ast.dump() outputs values for optional fields even if they are equal to defaults. This makes the output unnecessary verbose.

    For example (kind and type_comment are optional):

    >>> ast.dump(ast.parse('x = 1'))
    "Module(body=[Assign(targets=[Name(id='x', ctx=Store())], value=Constant(value=1, kind=None), type_comment=None)], type_ignores=[])"

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 14, 2019
    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Mar 14, 2019

    Hi @serhiy.storchaka,

    I send a patch to Github to review. Let me know if is necessary unittest.

    Regards

    @brettcannon
    Copy link
    Member

    @eamanu tests are basically always necessary. :)

    @ilevkivskyi
    Copy link
    Member

    We can probably also skip type_ignores list if it is empty (which will be the case in 99% situations).

    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Mar 15, 2019

    Maybe we can ignore None and [] ?

    @serhiy-storchaka
    Copy link
    Member Author

    None can not be ignored in Constant(value=None). [] can not be ignored in Tuple(elts=[]).

    There is also a problem with using ast.dump() with annotate_fields=False:

    >>> from ast import *
    >>> dump(Raise(cause=Name(id='B', ctx=Load())), annotate_fields=False)
    "Raise(Name('B', Load()))"
    >>> dump(Raise(Name('B', Load())))
    "Raise(exc=Name(id='B', ctx=Load()))"

    For Raise(cause=X) it outputs a string which is evaluated to Raise(exc=X).

    @isidentical
    Copy link
    Sponsor Member

    @eamanu are you still interested in this issue? (bumped to 3.9)

    @isidentical isidentical added the 3.9 only security fixes label Nov 18, 2019
    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Nov 19, 2019

    Hi,

    Sorry sincerely I forgot this issue, if there are not any objection I can continue it.

    @ilevkivskyi
    Copy link
    Member

    No objections from me assuming you are going forward along the way proposed by Serhiy (i.e. only skip values for certain fields if value is the default, not a blanket skip for all Nones and empty lists).

    @serhiy-storchaka
    Copy link
    Member Author

    PR 18843 solves this issue by setting None as class attributes for optional fields and attributes (like "kind" or "end_col_offset").

    It is not so easy for fields like "type_ignores". They are mutable lists, so this approach cannot be applied to them.

    I want to get rid also from "ctx=Load()", but it is more complex change.

    @serhiy-storchaka serhiy-storchaka removed the 3.8 only security fixes label Mar 8, 2020
    @isidentical
    Copy link
    Sponsor Member

    It is not so easy for fields like "type_ignores". They are mutable lists, so this approach cannot be applied to them.

    What about keeping ASDL signatures in the nodes (). If we know the type of a field (can be parsed in runtime) we can infer the default value of a field. For type_ignores, it is a sequence so if it is empty we can just crop that part.

    @isidentical
    Copy link
    Sponsor Member

    Related issue: bpo-39638

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset b7e9525 by Serhiy Storchaka in branch 'master':
    bpo-36287: Make ast.dump() not output optional fields and attributes with default values. (GH-18843)
    b7e9525

    @isidentical
    Copy link
    Sponsor Member

    @serhiy.storchaka, with these ASDL signatures, I have a patch that would omit default values for both Nones and [] in case they are redundant. But this is a bit different than your approach so I wanted to ask what's your opinion about adding an extra argument called omit_defaults, and only omit defaults it present. I'm adding this because unlike your first patch, these aren't actually defaults when creating the objects (like ast.Module(body=[x]) != ast.Module(body=[x], type_ignores=[])) so doing anything other than looking to that representation would be different than the actual result. What're your opinions about this? (I'm submitting the initial version of the patch before doing a PR)

    @isidentical
    Copy link
    Sponsor Member

    Actually I have a better solution for this (which I hope to share really soon if it works.) I think we can do default value initialization for both Nones (with your patch) and lists, it requires a bit of extra work but I think I can do it.

    @isidentical
    Copy link
    Sponsor Member

    Adding bpo-39981 as a dependency.

    @furkanonder
    Copy link
    Sponsor Contributor

    @serhiy-storchaka The issue seems to have been solved. I think we can close the problem.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants