Skip to content

Conversation

@vstinner
Copy link
Member

@vstinnervstinner 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
MemberAuthor

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

@corona10corona10 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 :)

@vstinner
Copy link
MemberAuthor

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

@vstinnervstinner closed this Jul 3, 2020
@vstinnervstinner reopened this Jul 3, 2020
@vstinner
Copy link
MemberAuthor

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

@corona10
Copy link
Member

I did same thing on #21294 :(

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

re-open :)

@vstinnervstinner merged commit b1cc6ba into python:masterJul 3, 2020
@vstinnervstinner deleted the ast_module_state branch July 3, 2020 18:01
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner@corona10@encukou@the-knights-who-say-ni@bedevere-bot