bpo-32892: Support subclasses of base types in isinstance checks for AST constants.

[serhiy-storchaka]

Some projects (e.g. Chameleon) create ast.Str containing an instance of the str subclass.

https://bugs.python.org/issue32892

[nascheme]

It looks like making this isinstance will allow type sub-classes for all the different constant node types. I guess this only affects 'ast' module users that are using the old class names? Just wondering if it is safe to allow any subtype in there. The constants need to get marshaled don't they?

[vstinner]

I don't think that serialization has to be considered in isinstance().

It looks like making this isinstance will allow type sub-classes for all the different constant node types.

I don't understand. The second argument is _const_types[cls], not _const_types. ast.Str only check for str and str subclasses.

[nascheme]

I mean the change affects all the constant node types (Str, Num, Bytes, etc). I was wondering aloud if that was okay. I looked at the new ast.py code again today and tested older versions of Python. I think this change is fine. It only affects code that uses the backwards compatibility node types. The _ABC.__instancecheck__ is not enough to make perfect backwards compatibility. Old versions of Python didn't do any type checking for ast node values so you could do something like:

    x = ast.Str({})
    assert isinstance(x, ast.Str)

That will fail with the new ast.py module, even after this PR. Maybe the following would be better. Make Constant "remember" what kind of constant node it is when created. Then have __instancecheck__ use that as the first test.

--- a/Lib/ast.py
+++ b/Lib/ast.py
@@ -338,6 +338,9 @@ Constant.s = property(_getter, _setter)
 class _ABC(type):
 
     def __instancecheck__(cls, inst):
+        const_type = getattr(inst, '_const_type', None)
+        if const_type and const_type is cls:
+            return True
         if not isinstance(inst, Constant):
             return False
         if cls in _const_types:
@@ -351,7 +354,9 @@ class _ABC(type):
 
 def _new(cls, *args, **kwargs):
     if cls in _const_types:
-        return Constant(*args, **kwargs)
+        n = Constant(*args, **kwargs)
+        n._const_type = cls
+        return n
     return Constant.__new__(cls, *args, **kwargs)
 
 class Num(Constant, metaclass=_ABC):

[serhiy-storchaka]

This can be done simpler if Str.__new__ return Str instead of Constant. But I think that since old classes will be removed eventually, it is better to return the pure Constant.

I don't think we should support ast.Str({}).

[nascheme]

I checked ast.Num in older versions, it doesn't allow subclasses of int.

>>> class Int(int):
...   pass
... 
>>> n = ast.Expression(ast.Num(Int(1), lineno=1, col_offset=1))
>>> n = ast.fix_missing_locations(n)
>>> co = compile(n, 'none', 'eval')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: non-numeric type in Num

I think with your change we would get the same behavior, i.e. you can create the AST node with the subclass but then compile() will reject it with an error.

I tried with Str and got a similar type error. So, I wonder what Chameleon is trying to do. You can create the AST tree with the string subtypes in it but you can't compile it. So, what's the point?

>>> class String(str):
...   pass
... 
>>> n = ast.Expression(ast.Str(String('foo'), lineno=1, col_offset=1))
>>> co = compile(n, 'none', 'eval')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: AST string must be of type str

[serhiy-storchaka]

You can create AST nodes with arbitrary payload and use it on the Python side. But when pass a tree to compile() you will got an error if types don't match exactly.

Seems Chameleon doesn't pass these nodes to the compiler. Instead it generates a Python source from the AST.

This PR affects only isinstance checks. isinstance(ast.Str(String('foo')), ast.Str) will return true. In 3.8 ast.Str is a "pseudo-type", kept for backward compatibility (and it will be kept for long time).

[nascheme]

Okay, LGTO.

[malthe]

Chameleon has a code generation step that serializes the AST tree into a Python module which is then compiled as usual.

[vstinner]

Can you please add this test?
self.assertFalse(isinstance(ast.Constant(S('42')), ast.Num))

[vstinner]

(maybe instanciate the ast object only once)

[serhiy-storchaka]

I think it looks clearer if the ast object is instantiated at the same line. In most tests different ast objects are tested in sequential lines.