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

graminit.h defines very generic names like 'stmt' or 'test' #79378

Closed
vstinner opened this issue Nov 9, 2018 · 12 comments
Closed

graminit.h defines very generic names like 'stmt' or 'test' #79378

vstinner opened this issue Nov 9, 2018 · 12 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Nov 9, 2018

BPO 35197
Nosy @vstinner, @serhiy-storchaka, @pablogsal
Superseder
  • bpo-43244: Move PyArena C API to the internal C API
  • Files
  • include-graminit-h.diff
  • 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 2021-05-26.22:43:50.488>
    created_at = <Date 2018-11-09.14:06:44.804>
    labels = ['interpreter-core', '3.8']
    title = "graminit.h defines very generic names like 'stmt' or 'test'"
    updated_at = <Date 2021-05-26.22:44:32.369>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-05-26.22:44:32.369>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-26.22:43:50.488>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2018-11-09.14:06:44.804>
    creator = 'vstinner'
    dependencies = []
    files = ['47916']
    hgrepos = []
    issue_num = 35197
    keywords = ['patch']
    message_count = 12.0
    messages = ['329517', '329518', '329519', '329524', '329526', '329528', '329533', '329542', '329571', '342523', '394488', '394490']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'serhiy.storchaka', 'pablogsal']
    pr_nums = []
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '43244'
    type = None
    url = 'https://bugs.python.org/issue35197'
    versions = ['Python 3.8']

    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 9, 2018

    graminit.h is an header file associating Grammar entities to their values, example:

    #define stmt 269

    Problem: defines symbols have no prefix and cause compilation issues on regular C code depending where graminit.h is included. Example with ast.c:

    static int
    validate_stmt(stmt_ty stmt)
    { ... }

    "#define stmt 269" causes a compilation issue, the preprocessor replaces the code with:

    static int
    validate_stmt(stmt_ty 269)
    { ... }

    ... which is invalid.

    Another example:

        return validate_expr(exp->v.IfExp.test, Load) &&
            validate_expr(exp->v.IfExp.body, Load) &&
            validate_expr(exp->v.IfExp.orelse, Load);
    

    The preprocessor replaces "exp->v.IfExp.test" with "exp->v.IfExp.305" which is invalid...

    The compile.h header file works around the issue by redefining 3 constants but using "Py_" prefix:

    /* These definitions must match corresponding definitions in graminit.h.
       There's code in compile.c that checks that they are the same. */
    #define Py_single_input 256
    #define Py_file_input 257
    #define Py_eval_input 258

    For comparison, graminit.h uses:

    #define single_input 256
    #define file_input 257
    #define eval_input 258

    There are different solutions:

    • Do nothing: require to include graminit.h at the right place
    • Add a prefix to all symbols, ex: test => Py_test, grammar_test, etc.
    • Rename problematic names like 'test' and 'stmt'
    • Fix C code to avoid problematic name: 'test' is an an attribute name of a node struct...

    Note: "static const int single_input = 305;" cause a complation error on "case single_input": "case label does not reduce to an integer constant".

    IMHO adding a prefix would be a nice enhancement and a good compromise. It only impacts the four C files which include graminit.h: future.c, ast.c, parsetok.c and parsermodule.c.

    @vstinner vstinner added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 9, 2018
    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 9, 2018

    Note: "static const int single_input = 305;" cause a complation error on "case single_input": "case label does not reduce to an integer constant".

    Ah! A friend gave me another solution, define an enum. Example:

    enum {
    single_input = 256,
    ...
    };

    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 9, 2018

    Current workaround in ast.c:

    /* This is done here, so defines like "test" don't interfere with AST use above. */
    #include "grammar.h"
    #include "parsetok.h"
    #include "graminit.h"

    This code is in the "middle" of ast.c.

    @serhiy-storchaka
    Copy link
    Member

    I think it would be better to exclude graminit.h from the set of distributed headers. It is internal generated file, it should not be used by any user code.

    Some constants are duplicated as a part of public API in compile.h.

    @serhiy-storchaka
    Copy link
    Member

    graminit.h is included in few files, and not always its inclusion is necessary. Here is a patch that minimizes its inclusion. It is not ready for merging, it is just a demo.

    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 9, 2018

    I think it would be better to exclude graminit.h from the set of distributed headers. It is internal generated file, it should not be used by any user code.

    I'm fine with moving it to Include/internal/. Do you know if it's used outside CPython?

    @pablogsal
    Copy link
    Member

    Do you know if it's used outside CPython?

    Although not a complete view, all of the mentions I can find in GitHub is in copies of CPython/forks/vendoring of Cpython:

    https://github.com/search?l=C&p=13&q=graminit.h&type=Code

    @serhiy-storchaka
    Copy link
    Member

    It can be used by the code that works directly with CST instead of AST: these constants are used as values of the n_type field of the node structure.

    @vstinner
    Copy link
    Member Author

    vstinner commented Nov 9, 2018

    It can be used by the code that works directly with CST instead of AST: (...)

    Sorry, I don't know this part of CPython. Is the CST exposed in any way outside CPython internals? If no, I see no reason to expose graminit.h in the public C API.

    @vstinner
    Copy link
    Member Author

    Another common issue with Python-ast.h, I just saw it on Windows:

    compile.c
    c:\program files (x86)\windows kits\10\include\10.0.17134.0\um\winbase.h(102): warning C4005: 'Yield': macro redefinition [C:\vstinner\python\master\PCbuild\pythoncore
    .vcxproj]
    c:\vstinner\python\master\include\python-ast.h(627): note: see previous definition of 'Yield'

    @vstinner
    Copy link
    Member Author

    Issue fixed in bpo-43244.

    I moved the evil public Python-ast.h header file into the internal C API as Include/internal/pycore_ast.h. It doesn't define "Yield" anymore: "_PyAST_Yield" must be used instead.

    @vstinner
    Copy link
    Member Author

    Moreover, graminit.h was removed in Python 3.10.

    @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.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants