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

[vstinner]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

Moreover, graminit.h was removed in Python 3.10.