Convert _is_abstract to module function, address misc review comments

Co-authored-by: Bénédikt Tran <[email protected]>

Author: brianschubert

Lib/test/test_ast/test_ast.py

@@ -1,4 +1,5 @@
 import _ast_unparse
+import _ast
 import ast
 import builtins
 import contextlib
@@ -111,9 +112,15 @@ def cleanup():
 
         msg = "type object 'ast.AST' has no attribute '_fields'"
         # Both examples used to crash:
-        with self.assertRaisesRegex(AttributeError, msg), self.assertWarns(DeprecationWarning):
+        with (
+            self.assertRaisesRegex(AttributeError, msg),
+            self.assertWarns(DeprecationWarning),
+        ):
             ast.AST(arg1=123)
-        with self.assertRaisesRegex(AttributeError, msg), self.assertWarns(DeprecationWarning):
+        with (
+            self.assertRaisesRegex(AttributeError, msg),
+            self.assertWarns(DeprecationWarning),
+        ):
             ast.AST()
 
     def test_node_garbage_collection(self):
@@ -429,12 +436,15 @@ def _construct_ast_class(self, cls):
             elif typ is object:
                 kwargs[name] = b'capybara'
             elif isinstance(typ, type) and issubclass(typ, ast.AST):
-                if typ._is_abstract():
+                if _ast._is_abstract(typ):
                     # Use an arbitrary concrete subclass
-                    concrete = next(sub for sub in typ.__subclasses__() if not sub._is_abstract())
-                    kwargs[name] = self._construct_ast_class(concrete)
+                    concrete = next((sub for sub in typ.__subclasses__()
+                                     if not _ast._is_abstract(sub)), None)
+                    msg = f"abstract node class {typ} has no concrete subclasses"
+                    self.assertIsNotNone(concrete, msg)
                 else:
-                    kwargs[name] = self._construct_ast_class(typ)
+                    concrete = typ
+                kwargs[name] = self._construct_ast_class(concrete)
         return cls(**kwargs)
 
     def test_arguments(self):
@@ -581,8 +591,8 @@ def test_nodeclasses(self):
         self.assertEqual(x.foobarbaz, 42)
 
         # Directly instantiating abstract node types is allowed (but deprecated)
-        with self.assertWarns(DeprecationWarning):
-            ast.stmt()
+        self.assertWarns(DeprecationWarning, ast.stmt)
+        self.assertWarns(DeprecationWarning, ast.expr_context)
 
     def test_no_fields(self):
         # this used to fail because Sub._fields was None
@@ -593,7 +603,8 @@ def test_invalid_sum(self):
         pos = dict(lineno=2, col_offset=3)
         with self.assertWarns(DeprecationWarning):
             # Creating instances of ast.expr is deprecated
-            m = ast.Module([ast.Expr(ast.expr(**pos), **pos)], [])
+            e = ast.expr(**pos)
+        m = ast.Module([ast.Expr(e, **pos)], [])
         with self.assertRaises(TypeError) as cm:
             compile(m, "<test>", "exec")
         self.assertIn("but got expr()", str(cm.exception))
@@ -1145,16 +1156,18 @@ class CopyTests(unittest.TestCase):
     def iter_ast_classes():
         """Iterate over the (native) subclasses of ast.AST recursively.
 
-        This excludes the special class ast.Index since its constructor
-        returns an integer.
+        This excludes:
+          * abstract AST nodes
+          * the special class ast.Index, since its constructor returns
+            an integer.
         """
         def do(cls):
             if cls.__module__ != 'ast':
                 return
             if cls is ast.Index:
                 return
             # Don't attempt to create instances of abstract AST nodes
-            if cls._is_abstract():
+            if _ast._is_abstract(cls):
                 return
 
             yield cls

Parser/asdl_c.py

@@ -887,11 +887,10 @@ def visitModule(self, mod):
     }
     else if (contains == 1) {
         if (PyErr_WarnFormat(
-            PyExc_DeprecationWarning, 1,
-            "Instantiating abstract AST node class %T is deprecated. "
-            "This will become an error in Python 3.20",
-            self
-        ) < 0) {
+                PyExc_DeprecationWarning, 1,
+                "Instantiating abstract AST node class %T is deprecated. "
+                "This will become an error in Python 3.20", self) < 0)
+        {
             return -1;
         }
     }
@@ -1458,23 +1457,6 @@ def visitModule(self, mod):
     return result;
 }
 
-/* Helper for checking if a node class is abstract in the tests. */
-static PyObject *
-ast_is_abstract(PyObject *cls, void *Py_UNUSED(ignored)) {
-    struct ast_state *state = get_ast_state();
-    if (state == NULL) {
-        return NULL;
-    }
-    int contains = PySet_Contains(state->abstract_types, cls);
-    if (contains == -1) {
-        return NULL;
-    }
-    else if (contains == 1) {
-        Py_RETURN_TRUE;
-    }
-    Py_RETURN_FALSE;
-}
-
 static PyMemberDef ast_type_members[] = {
     {"__dictoffset__", Py_T_PYSSIZET, offsetof(AST_object, dict), Py_READONLY},
     {NULL}  /* Sentinel */
@@ -1486,7 +1468,6 @@ def visitModule(self, mod):
      PyDoc_STR("__replace__($self, /, **fields)\\n--\\n\\n"
                "Return a copy of the AST node with new values "
                "for the specified fields.")},
-    {"_is_abstract", _PyCFunction_CAST(ast_is_abstract), METH_CLASS | METH_NOARGS, NULL},
     {NULL}
 };
 
@@ -2001,6 +1982,30 @@ def emit_defaults(self, name, fields, depth):
 class ASTModuleVisitor(PickleVisitor):
 
     def visitModule(self, mod):
+        self.emit("""
+/* Helper for checking if a node class is abstract in the tests. */
+static PyObject *
+ast_is_abstract(PyObject *Py_UNUSED(module), PyObject *cls) {
+    struct ast_state *state = get_ast_state();
+    if (state == NULL) {
+        return NULL;
+    }
+    int contains = PySet_Contains(state->abstract_types, cls);
+    if (contains == -1) {
+        return NULL;
+    }
+    else if (contains == 1) {
+        Py_RETURN_TRUE;
+    }
+    Py_RETURN_FALSE;
+}
+
+static struct PyMethodDef astmodule_methods[] = {
+    {"_is_abstract", ast_is_abstract, METH_O, NULL},
+    {NULL}  /* Sentinel */
+};
+""".strip(), 0, reflow=False)
+        self.emit("", 0)
         self.emit("static int", 0)
         self.emit("astmodule_exec(PyObject *m)", 0)
         self.emit("{", 0)
@@ -2041,7 +2046,8 @@ def visitModule(self, mod):
     .m_name = "_ast",
     // The _ast module uses a per-interpreter state (PyInterpreterState.ast)
     .m_size = 0,
-    .m_slots = astmodule_slots,
+    .m_methods = astmodule_methods,
+    .m_slots = astmodule_slots
 };
 
 PyMODINIT_FUNC