bpo-32892: Support subclasses of base types in isinstance checks for AST constants. by serhiy-storchaka · Pull Request #9934 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

…AST constants.

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

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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({}).

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

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).

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

(maybe instanciate the ast object only once)

Choose a reason for hiding this comment

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

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.