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

bpo-41194: Convert _ast extension to PEP 489 #21293

Merged
merged 2 commits into from
Jul 3, 2020
Merged

bpo-41194: Convert _ast extension to PEP 489 #21293

merged 2 commits into from
Jul 3, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 3, 2020

Convert the _ast extension module to PEP 489 "Multiphase
initialization". Replace the global _ast state with a module state.

https://bugs.python.org/issue41194

Convert the _ast extension module to PEP 489 "Multiphase
initialization". Replace the global _ast state with a module state.
@vstinner
Copy link
Member Author

vstinner commented Jul 3, 2020

With this PR, get_global_ast_state() is still used by multiple functions. It can introduce minor inconsistencies when the _ast extension module is loaded more than once in the same interpreter. But this PR better isolates the _ast module in subinterpreters.

The PyType_GetModule() (PEP 573) cannot be used in ast_type_init() (tp_init slot).

Even if all AST_Type methods and slots are fixed to use PyType_GetModule(), there are still PyAST_mod2obj() and PyAST_obj2mod() functions which still use get_global_ast_state().

Even if get_global_ast_state() has drawbacks, IMHO this PR is worth it.

@corona10: Would you mind to review it?

cc @DinoV

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with a simple question :)

Parser/asdl_c.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

vstinner commented Jul 3, 2020

continuous-integration/travis-ci Expected — Waiting for status to be reported

But https://travis-ci.org/github/python/cpython/builds/704696576 is the build and it completed. I don't get it :-(

  • Ran for 18 min 13 sec
  • Total time 27 min 27 sec
  • 9 minutes ago

@vstinner vstinner closed this Jul 3, 2020
@vstinner vstinner reopened this Jul 3, 2020
@vstinner
Copy link
Member Author

vstinner commented Jul 3, 2020

I close/reopen my issue to force a new Travis CI build :-(

@corona10
Copy link
Member

corona10 commented Jul 3, 2020

I did same thing on #21294 :(

@corona10 corona10 closed this Jul 3, 2020
@corona10 corona10 reopened this Jul 3, 2020
@corona10
Copy link
Member

corona10 commented Jul 3, 2020

re-open :)

@vstinner vstinner merged commit b1cc6ba into python:master Jul 3, 2020
@vstinner vstinner deleted the ast_module_state branch July 3, 2020 18:01
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
Convert the _ast extension module to PEP 489 "Multiphase
initialization". Replace the global _ast state with a module state.
vstinner added a commit that referenced this pull request Aug 10, 2020
* bpo-41194: Convert _ast extension to PEP 489 (GH-21293)

Convert the _ast extension module to PEP 489 "Multiphase
initialization". Replace the global _ast state with a module state.

(cherry picked from commit b1cc6ba)

* bpo-41204: Fix compiler warning in ast_type_init() (GH-21307)

(cherry picked from commit 1f76453)
if (name == NULL) {
return NULL;
}
PyObject *module = PyImport_GetModule(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in https://bugs.python.org/issue41631, this is incorrect :(
PyImport_GetModule looks in sys.modules, which is modifiable by the user, and so it's not guaranteed to return _ast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants