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

PEP 511: Add ast.Constant to allow AST optimizer to emit constants #70334

Closed
vstinner opened this issue Jan 18, 2016 · 16 comments
Closed

PEP 511: Add ast.Constant to allow AST optimizer to emit constants #70334

vstinner opened this issue Jan 18, 2016 · 16 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 26146
Nosy @brettcannon, @vstinner, @benjaminp, @serhiy-storchaka
Dependencies
  • bpo-25843: code_richcompare() don't use constant type when comparing code constants
  • Files
  • constant.patch
  • constant-2.patch
  • constant-3.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 = <Date 2016-01-26.00:19:15.674>
    created_at = <Date 2016-01-18.17:04:51.468>
    labels = ['interpreter-core', 'type-feature']
    title = 'PEP 511: Add ast.Constant to allow AST optimizer to emit constants'
    updated_at = <Date 2016-01-27.11:20:23.012>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-01-27.11:20:23.012>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-01-26.00:19:15.674>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2016-01-18.17:04:51.468>
    creator = 'vstinner'
    dependencies = ['25843']
    files = ['41647', '41689', '41708']
    hgrepos = []
    issue_num = 26146
    keywords = ['patch']
    message_count = 16.0
    messages = ['258528', '258531', '258536', '258543', '258621', '258759', '258799', '258865', '258869', '258870', '258901', '258936', '258937', '258982', '258983', '259017']
    nosy_count = 5.0
    nosy_names = ['brett.cannon', 'vstinner', 'benjamin.peterson', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26146'
    versions = ['Python 3.6']

    @vstinner
    Copy link
    Member Author

    Currently, the Python parser emits ast.Tuple AST nodes which are compiled to multiple LOAD_xxx instructions followed a final BUILD_TUPLE instruction. The Python peephole optimizer detect LOAD_CONST x n + BUILD_TUPLE instructions to replace it directly with a tuple constant.

    IHMO it's better to implement this optimization early at the AST level in an AST optimizer. The PEP-511 proposes to add a new ast.Constant AST node for that.

    With this new AST node, the AST optimizer can emit tuple constants, but also any kind of constant like frozenset. For example, it's also possible to optimize "x in {1, 2}" to "x in frozenset({1, 2})" where frozenset({1, 2}) is a constant (don't call frozenset type at runtime). (Again, this optimization is already implemented in the peephole optimizer, it's just an example.)

    Attached patch adds the new ast.Constant type but update also code consuming AST to handle this new kind of node:

    I didn't change the compiler to emit directly ast.Constant nodes to reduce the impact on the backward compatibility. This change can be done. An AST optimizer is responsible to convert NameConstant (False, True, None), Num (int, float, complex), Str, Bytes, Tuple to Constant. Example with fatoptimizer:
    https://github.com/haypo/fatoptimizer/blob/2d794f511fe23ccde320725c6d12ce5ce8ffbdfe/fatoptimizer/convert_const.py

    ast.Constant also simplifies AST optimizers: no need to check each time if a node is constant or not.

    Adding a new kind of node was already proposed in the old issue bpo-11549: the patch adds ast.Lit (it was proposed to rename it to ast.Literal).

    @vstinner vstinner added the type-feature A feature request or enhancement label Jan 18, 2016
    @vstinner
    Copy link
    Member Author

    To support emiting constants from ast.Constant, we will also need the fix for the issue bpo-25843. Currently, the compile merges constants (0, 0) and (0.0, 0.0) because they are equal, but item types are different.

    @brettcannon
    Copy link
    Member

    Would it make sense to tag the type of the constant in the node somehow?

    We also make no backwards-compatibility guarantees about the AST, so if it simplifies things to switch entirely to Constant from Num, etc. then I said do it.

    @vstinner
    Copy link
    Member Author

    Brett Cannon: "Would it make sense to tag the type of the constant in the node somehow?"

    It's easy to get the type of a constant: type(node.value). I like using isinstance().

    Brett Cannon: "We also make no backwards-compatibility guarantees about the AST, so if it simplifies things to switch entirely to Constant from Num, etc. then I said do it."

    Oh, I forgot an important point from the PEP: "[ast.Constant] does not contain line number and column offset informations on tuple or frozenset items." I don't know if it's an issue or not.

    I prefer to move step by step. As I wrote, we can decide to use directly ast.Constant later, even before the Python 3.6 release.

    @vstinner
    Copy link
    Member Author

    First I also wanted to add ast.Literal to literals: list, set, dict, etc. But it doesn't work, we loose the item order: set and dict are unordered. An optimizer must not change the order in which items are created. At least, not by default.

    I'm talking about an hypothetical ast.Literal type which would take a Python object (list, set, etc.) Current ast.Set contains an ordered list of items, ast.Dict uses two ordered lists for keys and values.

    @vstinner
    Copy link
    Member Author

    We also make no backwards-compatibility guarantees about the AST, so if it simplifies things to switch entirely to Constant from Num, etc. then I said do it.

    Replacing Num/Str/... with Constant causes bug in markerlib (used by pip, it breaks indirectly the venv module of the stdlib) and Chameleon benchmark. I only found these projects by mistake. I'm quite sure that much more code rely on the AST "API" even if it's unstable.

    I prefer to not break the API for free :-)

    @vstinner
    Copy link
    Member Author

    Patch version 2:

    • rework the whole patch
    • add unit tests
    • fix AST validation: the code was completly wrong in patch 1, I don't understand how it worked :-p Validate also correctly nested tuple and nested frozenset.
    • add a comment to explain why obj2ast_constant() doesn't have to Py_INCREF() singletons
    • handle also Ellipsis
    • revert changes on set_context(). It seems like set_context() is only called with code emited directly by the compiler (not by compile(custom_ast_tree, ...)). If someone finds a way to call set_context() with an ast.Constant, we can support this case later.

    @vstinner
    Copy link
    Member Author

    @serhiy: Would you mind reviewing constant-2.patch? I reviewed my own patch and added some comments, I found a refleak ;-)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 23, 2016

    New changeset 5de1bd759f3b by Victor Stinner in branch 'default':
    Issue bpo-26146: marshal.loads() now uses the empty frozenset singleton
    https://hg.python.org/cpython/rev/5de1bd759f3b

    @vstinner
    Copy link
    Member Author

    New changeset 5de1bd759f3b by Victor Stinner in branch 'default':
    Issue bpo-26146: marshal.loads() now uses the empty frozenset singleton
    https://hg.python.org/cpython/rev/5de1bd759f3b

    This change is not directly related to this issue.

    It's required by test_singleton_empty_frozenset() of test_set when an AST transformer replaces frozenset(), frozenset([]), etc. calls with an empty frozenset constant.

    @vstinner
    Copy link
    Member Author

    Patch version 3: fix a reference leak in validate_constant().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 26, 2016

    New changeset 27e5437f442c by Victor Stinner in branch 'default':
    Add ast.Constant
    https://hg.python.org/cpython/rev/27e5437f442c

    @vstinner
    Copy link
    Member Author

    I pushed an enhanced version of constant-3.patch:

    • fix the compiler: don't emit LOAD_CONST for ast.Constant if the value is a string (str) or a number (int, float, complex), as done previously for ast.Str and ast.Num

    • add unit tests on: the compiler (ensure that LOAD_CONST is emitted for constants), ast.get_docstring() and ast.literal_eval()

    @vstinner vstinner added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jan 26, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 26, 2016

    New changeset c5df914e73ad by Victor Stinner in branch 'default':
    Fix a refleak in validate_constant()
    https://hg.python.org/cpython/rev/c5df914e73ad

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 26, 2016

    New changeset 04444d0d0a05 by Victor Stinner in branch 'default':
    Issue bpo-26146: remove useless code
    https://hg.python.org/cpython/rev/04444d0d0a05

    New changeset 58086f0a953a by Victor Stinner in branch 'default':
    Issue bpo-26146: enhance ast.Constant error message
    https://hg.python.org/cpython/rev/58086f0a953a

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 27, 2016

    New changeset 16f60cd918e0 by Victor Stinner in branch 'default':
    PEP-511
    https://hg.python.org/peps/rev/16f60cd918e0

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants