Skip to content

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

Closed
Closed
graminit.h defines very generic names like 'stmt' or 'test'#79378
Labels
3.8 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)

Description

@vstinner
vstinner
opened on 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']

    Activity

    1. vstinner commented on Nov 9, 2018

      @vstinner
      vstinner
      on Nov 9, 2018
      MemberAuthor

      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.

    2. vstinner
      added
      3.8 (EOL)end of life
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      on Nov 9, 2018

    3. vstinner commented on Nov 9, 2018

      @vstinner
      vstinner
      on Nov 9, 2018
      MemberAuthor

      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,
      ...
      };

    4. vstinner commented on Nov 9, 2018

      @vstinner
      vstinner
      on Nov 9, 2018
      MemberAuthor

      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.

    5. serhiy-storchaka commented on Nov 9, 2018

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 9, 2018
      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.

    6. serhiy-storchaka commented on Nov 9, 2018

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 9, 2018
      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.

    7. vstinner commented on Nov 9, 2018

      @vstinner
      vstinner
      on Nov 9, 2018
      MemberAuthor

      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?

    8. pablogsal commented on Nov 9, 2018

      @pablogsal
      pablogsal
      on Nov 9, 2018
      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

    9. serhiy-storchaka commented on Nov 9, 2018

      @serhiy-storchaka
      serhiy-storchaka
      on Nov 9, 2018
      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.

    10. vstinner commented on Nov 9, 2018

      @vstinner
      vstinner
      on Nov 9, 2018
      MemberAuthor

      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.

    11. vstinner commented on May 14, 2019

      @vstinner
      vstinner
      on May 14, 2019
      MemberAuthor

      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'

    12. vstinner commented on May 26, 2021

      @vstinner
      vstinner
      on May 26, 2021
      MemberAuthor

      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.

    13. vstinner
      closed this as completedon May 26, 2021

    14. vstinner commented on May 26, 2021

      @vstinner
      vstinner
      on May 26, 2021
      MemberAuthor

      Moreover, graminit.h was removed in Python 3.10.

    15. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.8 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions