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

PEP-695: Potentially breaking changes made to __match_args__ attributes of AST nodes #104799

Closed
AlexWaygood opened this issue May 23, 2023 · 14 comments
Assignees
Labels
3.12 bugs and security fixes 3.13 new features, bugs and security fixes type-bug An unexpected behavior, bug, or error

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented May 23, 2023

Consider the following script:

import ast

def test(node):
    match node:
        case ast.FunctionDef("foo", ast.arguments(args=[ast.arg("bar")])):
            print('matched! :)')
        case _:
            print("Didn't match :(")


source = ast.parse("def foo(bar): pass")
node = source.body[0]
assert isinstance(node, ast.FunctionDef)
test(node)

Running this script on 3.11 gets you this output:

>python repro.py
matched! :)

Running this script on CPython main, however, gets you this output:

>python repro.py
Didn't match :(

The reason for this is that the implementation of PEP-695 (new in Python 3.12) added a number of new AST nodes to Python, and as a result, the __match_args__ attributes of ast.FunctionDef, ast.AsyncFunctionDef and ast.ClassDef are all different on 3.12 compared to what they were on 3.11.

`__match_args__` attributes on 3.11:
>>> import ast
>>> for node in ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef:
...     print(node.__match_args__)
...
('name', 'bases', 'keywords', 'body', 'decorator_list')
('name', 'args', 'body', 'decorator_list', 'returns', 'type_comment')
('name', 'args', 'body', 'decorator_list', 'returns', 'type_comment')
`__match_args__` attributes on 3.12:
>>> import ast
>>> for node in ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef:
...     print(node.__match_args__)
...
('name', 'type_params', 'bases', 'keywords', 'body', 'decorator_list')
('name', 'type_params', 'args', 'body', 'decorator_list', 'returns', 'type_comment')
('name', 'type_params', 'args', 'body', 'decorator_list', 'returns', 'type_comment')

This feels like it has the potential to be quite a breaking change for people using pattern-matching to parse ASTs. It would probably be okay if type_params had been added as the final item in the __match_args__ tuples, but at the moment it comes in second place.

Cc. @JelleZijlstra for PEP-695. Also curious if @brandtbucher has any thoughts (for pattern-matching expertise) or @isidentical (for ast-module expertise).

Linked PRs

@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error 3.12 bugs and security fixes 3.13 new features, bugs and security fixes labels May 23, 2023
@brandtbucher
Copy link
Member

Nice catch. Even though the AST has murky stability guarantees, I agree that the right thing to do here is to move the new arguments to the "end" (in the AST def, which generates both the constructor and __match_args__). That way, we can gracefully handle anybody matching on or building these nodes.

@JelleZijlstra
Copy link
Member

Submitted a PR. There is still a compatibility break when you create a FunctionDef node. Consider this code:

import ast
a = ast.arguments([], [], None, [], [], None, [])
f = ast.FunctionDef("hello", a, [ast.Pass()], [])
mod = ast.Module([f], [])
mod.lineno = mod.col_offset = 0
ast.fix_missing_locations(mod)
compile(mod, "x", "exec")

On 3.11 this works.

On main I get:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 8, in make
TypeError: required field "type_params" missing from FunctionDef

I'm not sure this is fixable without bigger changes to how we generate the AST classes.

@hmc-cs-mdrissi
Copy link
Contributor

hmc-cs-mdrissi commented May 24, 2023

I suspect of 2 breaking changes match args vs building ast nodes, the latter is more common. Especially as matching ast node I would expect to commonly use node visitor. It does produce a good number of hits

This is small fix to upgrade as I’d guess most of time empty type params is fix. And unsure AST typical compatibility expectation as seems like any new attribute would trigger this. Were there similar ast incompatibilities in last couple minor releases?

@JelleZijlstra
Copy link
Member

#104834 is a suggested way to restore compatibility for creation of AST nodes. It's more invasive, though, so not sure it's a good idea.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 24, 2023

Especially as matching ast node I would expect to commonly use node visitor.

@hmc-cs-mdrissi, pattern-matching works very well in combination with an ast.NodeVisitor subclass! I do this in typeshed-stats: https://github.com/AlexWaygood/typeshed-stats/blob/c01af7517d23678a1e1050640a1934dcc1b8284d/src/typeshed_stats/gather.py#L105-L132.

FWIW, while it would be ideal if we could avoid both, I think the pattern-matching breakage is worse than the direct-construction breakage. Calls to the constructors of these classes will immediately raise an exception on 3.12, so it will be immediately obvious what the cause of the breakage is, and how to fix it. But if you have a function like the one in my opening comment for this issue, it won't start raising an exception on 3.12 -- it will just silently start doing the wrong thing. That could make it hard for users to track down where the bug is, and what they need to do to fix it.

^EDIT: That rationale is all misguided; @JelleZijlstra corrected me below.

My preference would be:

@JelleZijlstra
Copy link
Member

Calls to the constructors of these classes will immediately raise an exception on 3.12

That's not true. The AST node constructors accept almost anything, you only get errors later when you use the node, and then only if you're lucky.

>>> import ast
>>> ast.FunctionDef(42)
<ast.FunctionDef object at 0x103197760>
>>> ast.dump(_)
'FunctionDef(name=42)'

@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 24, 2023

Oh, sorry. I misread/misunderstood your previous comment. My bad — that seems worse, in that case :)

But my opinion would still be "definitely do the easier thing, maybe also do the more invasive thing as well".

JelleZijlstra added a commit that referenced this issue May 26, 2023
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 26, 2023
…4828)

(cherry picked from commit ba73473)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@JelleZijlstra
Copy link
Member

JelleZijlstra commented May 26, 2023

Seems like the fix changes the ABI. Please comment on #104974 if you have insights into whether we should change the ABI here.

JelleZijlstra added a commit that referenced this issue May 30, 2023
…#104974)

gh-104799: Move location of type_params AST fields (GH-104828)
(cherry picked from commit ba73473)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
JelleZijlstra added a commit that referenced this issue Jun 2, 2023
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 2, 2023
…onGH-104834)

(cherry picked from commit 77d2579)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
JelleZijlstra added a commit that referenced this issue Jun 2, 2023
…104834) (#105213)

(cherry picked from commit 77d2579)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member Author

I think this is now fixed. Thanks @JelleZijlstra!

@DavidCEllis
Copy link
Contributor

Not sure if I should make a new issue for this as it seems related. While #104834 seems to have fixed the issue for compile and presumably match, ast.unparse at least still requires "type_params" in 3.12.0b2

The original version with compile now works, but using unparse you still get the error.

import ast
a = ast.arguments([], [], None, [], [], None, [])
f = ast.FunctionDef("hello", a, [ast.Pass()], [])
mod = ast.Module([f], [])
mod.lineno = mod.col_offset = 0
ast.fix_missing_locations(mod)

print(ast.unparse(mod))

3.11.4

def hello():
    pass

3.12.0b2 (3.12-dev from this morning also)

AttributeError: 'FunctionDef' object has no attribute 'type_params'

Adding type_params=[] to the functiondef prevents the error but it looked like that was supposed to be made the default with the patch?

@AlexWaygood AlexWaygood reopened this Jun 12, 2023
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this issue Jun 16, 2023
@JelleZijlstra
Copy link
Member

Thanks @DavidCEllis, fixing this in #105846.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 16, 2023
…onGH-105846)

(cherry picked from commit 957a974)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra added a commit that referenced this issue Jun 16, 2023
…105846) (#105862)

(cherry picked from commit 957a974)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@AlexWaygood
Copy link
Member Author

Should be fixed now following #105846@DavidCEllis let us know if anything else isn't working as you'd expect!

carljm added a commit to carljm/cpython that referenced this issue Jun 16, 2023
* main:
  pythongh-104799: PEP 695 backward compatibility for ast.unparse (python#105846)
  pythongh-105834: Add tests for calling `issubclass()` between two protocols (python#105835)
  CI: Remove docs build from Azure Pipelines (python#105823)
  pythongh-105844: Consistently use 'minor version' for X.Y versions (python#105851)
  Fix inaccuracies in "Assorted Topics" section of "Defining Extension Types" tutorial (python#104969)
  pythongh-105433: Add `pickle` tests for PEP695 (python#105443)
  bpo-44530: Document the change in MAKE_FUNCTION behavior (python#93189)
  pythonGH-103124: Multiline statement support for pdb (pythonGH-103125)
  pythonGH-105588: Add missing error checks to some obj2ast_* converters (pythonGH-105589)
@DavidCEllis
Copy link
Contributor

Should be fixed now following #105846@DavidCEllis let us know if anything else isn't working as you'd expect!

I ran into this as a bunch of tests that were fine against some of the alphas were failing on the first beta. Looking up the error message took me to the first patch. Waiting and retesting on 3.12.0b2 had most of them passing again apart from the ones that used unparse.

It looks like they all now pass against a new 3.12-dev build - Thanks @JelleZijlstra and @AlexWaygood !

@JelleZijlstra
Copy link
Member

Thanks! You might be interested in #105858 which will hopefully provide a more general solution; let me know on that issue if you have any feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 new features, bugs and security fixes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants