Navigation Menu

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

Jump to bottom

Remove Suite node from AST #83820

Closed
isidentical opened this issue Feb 15, 2020 · 5 comments
Closed

Remove Suite node from AST #83820

isidentical opened this issue Feb 15, 2020 · 5 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir

Comments

@isidentical
Copy link
Sponsor Member

BPO 39639
Nosy @brettcannon, @vstinner, @benjaminp, @jeff5, @1st1, @pablogsal, @isidentical, @shihai1991
PRs
  • bpo-39639: remove suite node #18513
    		•

    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 2020-03-04.16:17:30.198>
created_at = <Date 2020-02-15.11:46:49.387>
labels = ['library', '3.9']
title = 'Remove Suite node from AST'
updated_at = <Date 2020-03-04.16:17:30.198>
user = 'https://github.com/isidentical'

    bugs.python.org fields:

    activity = <Date 2020-03-04.16:17:30.198>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2020-03-04.16:17:30.198>
closer = 'pablogsal'
components = ['Library (Lib)']
creation = <Date 2020-02-15.11:46:49.387>
creator = 'BTaskaya'
dependencies = []
files = []
hgrepos = []
issue_num = 39639
keywords = ['patch']
message_count = 5.0
messages = ['362014', '362018', '362477', '363304', '363358']
nosy_count = 8.0
nosy_names = ['brett.cannon', 'vstinner', 'benjamin.peterson', 'jeff.allen', 'yselivanov', 'pablogsal', 'BTaskaya', 'shihai1991']
pr_nums = ['18513']
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue39639'
versions = ['Python 3.9']
    @isidentical
    Copy link
    Sponsor Member Author

    AST is containing a node from past that is explained as "not really an actual node but useful in Jython's typesystem.". There is no usage of it anywhere in the CPython repo, just some code in ast_optimizer, symbol table and compiler to forbid it from running. If there is not any specific reason to keep it, we can just remove and clean some code.

    @isidentical isidentical added 3.9 only security fixes stdlib Python modules in the Lib dir labels Feb 15, 2020
    @isidentical isidentical changed the title Remote Suite node from AST Remove Suite node from AST Feb 15, 2020
    @isidentical isidentical changed the title Remote Suite node from AST Remove Suite node from AST Feb 15, 2020
    @shihai1991
    Copy link
    Member

    I am not sure there have any relation with jython's python.asdl?

    https://github.com/jythontools/jython/blob/master/ast/Python.asdl#L10

    @jeff5
    Copy link
    Mannequin

    jeff5 mannequin commented Feb 22, 2020

    Jython uses the reference grammar and ASDL as a way to ensure it is Python we approximate, not some subtly different language. The presence of Suite here gives rise to a class (https://github.com/jythontools/jython/blob/v2.7.2b3/src/org/python/antlr/ast/Suite.java) and we actually use instances of it in the compiler (https://github.com/jythontools/jython/blob/v2.7.2b3/src/org/python/compiler/CodeCompiler.java#L2389).

    It is a bit of a wart, to have a Jython-specific type here: somewhat defeating the object of using the same source. I expect there was a good reason: perhaps there was no better way to express the commonality between Interactive and Module. It was all before my involvement.

    I would try to avoid needing it in Jython 3, and if we can't, it doesn't look hard to manage the variation our copy. It's not like we copy these files mechanically from from CPython during a build.

    +1 on removing it.

    @vstinner
    Copy link
    Member

    vstinner commented Mar 3, 2020

    Jeff Allen: thanks for the useful feedback on how it's used in Jython!

    @pablogsal
    Copy link
    Member

    New changeset d82e469 by Batuhan Taşkaya in branch 'master':
    bpo-39639: Remove the AST "Suite" node and associated code (GH-18513)
    d82e469

    @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
    Assignees
    No one assigned
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @shihai1991 @pablogsal @isidentical