I think that it should only accept fields known to the AST class.

I think that it should only accept fields known to the AST class.

I can live with that assumption but I need to update the docs then. However, what about constructions that add parent's back references. I would expect such nodes to be able to copy themselves but only change one single attribute (like a sibling) so that's why I only allowed the fields in__dict__ to be mutated.

By the way, is there something I should have taken care of because of singletons such asast.Load? Now that I see my tests, I'm not sure if a special treatement is needed.

AFAIK, all other__replace__ implementations only allow to change the fixed set of fields, not arbitrary instance attributes.

Actually, I assumed that AST nodes were somewhat similar to simple namespaces, which allows for arbitrary addition I think:

But if you think we should restrict to fields known to the class, I can easily remove the code.

I feel AST nodes are more like dataclasses. For dataclasses, unrecognized arguments are rejected bycopy.replace and non-field attributes on the original object are lost.

>>> from dataclasses import dataclass>>> >>> @dataclass... class A:...     a: int...     >>> from copy import replace>>> a = A(1)>>> a.b = 42>>> a2 = replace(a, a=2)>>> a2A(a=2)>>> a2.bTraceback (most recent call last):  File "<python-input-8>", line 1, in <module>    a2.bAttributeError: 'A' object has no attribute 'b'>>> a.b42>>> replace(a, b=5)Traceback (most recent call last):  File "<python-input-10>", line 1, in <module>    replace(a, b=5)    ~~~~~~~^^^^^^^^  File "/Users/jelle/py/cpython/Lib/copy.py", line 294, in replace    return func(obj, **changes)  File "/Users/jelle/py/cpython/Lib/dataclasses.py", line 1630, in _replace    return self.__class__(**changes)           ~~~~~~~~~~~~~~^^^^^^^^^^^TypeError: A.__init__() got an unexpected keyword argument 'b'

Just after I sent my comment, I actually thought that maybe I should reject them since we anyway reject arbitrary keyword arguments in the constructor, maybe it's indeed closer to dataclasses.

I convinced myself as well and since we reached a consensus, I'll remove this. I'll extend thevalidate function (good thing I did this).

SimpleNamespace constructor accepts arbitrary keyword arguments, so__replace__() does the same. All names are valid forSimpleNamespace, but other objects are more picky.

I think that the implementation can be simpler.

Yes, that's what I decided after our discussion from yesterday. Currently the construction only warns about extra keyword arguments. For now, I'll leave that warning but this also means that the fieldsare replaced, even though the behaviour would disappear. In particular, in 3.15 and later, the tests should be changed, so I added some comments but is there a more rigourous way?

Ah the bot did not catch the fact that I said that I was done with the changes.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@serhiy-storchaka,@JelleZijlstra: please review the changes made to this pull request.

🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commitbddcb97 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

(I hope I didn't leak things. The previous implementation didn't but maybe this one does). I'll be leaving for today, my defense is tomorrow so I'll patch if afterwards if needed.

Just running the refleak bots because it's good practice to do that when there's a big pile of new C code.

Good luck with your defense!