bpo-15987: Add ast.AST class richcompare methods by louisom · Pull Request #1368

bpo-15987: Add ast.AST class richcompare methods by louisom · Pull Request #1368 · python/cpython

Choose a reason for hiding this comment

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

I don't know whether it is good idea to implement the comparison of AST nodes, but added technical comments to the implementation.


		def ast_eq_compare(a, b):
		if type(a) != type(b):
		return False

Choose a reason for hiding this comment

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

This will allow other classes implement the comparison with AST nodes.

		if type(a) != type(b):
		return False
		if type(a) not in _NODES:
		return a == b

Choose a reason for hiding this comment

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

Isn't this recursive?

Choose a reason for hiding this comment

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

We may recursive to a builtin-type value, then can return compare fast.


		ret = True
		for field in a._fields:
		af = vars(a)[field]

Choose a reason for hiding this comment

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

Why not getattr?

Choose a reason for hiding this comment

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

Thanks for point out.

		if type(a) not in _NODES:
		return a == b

		ret = True

Choose a reason for hiding this comment

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

Actually this isn't needed. See below.

		if len(af) != len(bf):
		return False
		for i, j in zip(af, bf):
		ret &= ast_eq_compare(i, j)

Choose a reason for hiding this comment

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

Fail fast.

But stop, why not compare just lists?

if af != bf:
    return False

Choose a reason for hiding this comment

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

No, you can't simply compare these, _ast.KEYWORDS's fields value will be builtin-type or [_ast.KEYWORDS] (e.g. [<_ast.Expr object at 0x7f06659c8468>]), since the compare method doesn't hook up yet, you can't simply compare a list with _ast.KEYWORDS.

Choose a reason for hiding this comment

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

Thus we will need to recursively to compare the items.


		for n in _NODES:
		n.__eq__ = ast_eq_compare
		n.__ne__ = ast_ne_compare

Choose a reason for hiding this comment

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

This isn't needed. The default implementation falls back to __eq__



		for n in _NODES:
		n.__eq__ = ast_eq_compare

Choose a reason for hiding this comment

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

Why not patch just the base class?

Choose a reason for hiding this comment

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

@serhiy-storchaka You are right, I thought is was difficult or not possible when implementing a top-down richcompare, but I'm wrong.

re-implement the richcompare inside the AST_type


		# Add EQ and NE compare to _ast Nodes
		_NODES = [vars(_ast)[k] for k in filter(
		lambda x: not x.startswith('_') and x not in ('AST' 'PyCF_ONLY_AST'), vars(_ast))]

Choose a reason for hiding this comment

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

'ASTPyCF_ONLY_AST' is the single literal.

Choose a reason for hiding this comment

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

Change to ('AST', 'PyCF_ONLY_AST')



		# Add EQ and NE compare to _ast Nodes
		_NODES = [vars(_ast)[k] for k in filter(

Choose a reason for hiding this comment

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

Don't use filter and lambda. Use just if in the list comprehension.



		# Add EQ and NE compare to _ast Nodes
		_NODES = [vars(_ast)[k] for k in filter(

Choose a reason for hiding this comment

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

Rather than calling vars() multiple times just iterate vars(_ast).items().

I'm not sure why travis failed on clang and gcc, my local build run pass the test.

@Haypo point out that I shouldn't change Python/Python-ast.c, it should be generated by asdl_c.py

louisom changed the title ~~bpo-15987: Add AST nodes eq & ne compare methods~~ bpo-15987: Add ast.AST class richcompare methods

May 2, 2017

taion mentioned this pull request

Jan 4, 2018

@louisom: Can you rebase this PR on master?

@vstinner the original branch no longer exists, GH shows it as "unknown repository", but it means the branch or the user no longer exist.

The only way to merge this would be to recreate the patch elsewhere.

Closing this pull request as it has been recreated under a different pull request.

Julian mannequin mentioned this pull request

Apr 10, 2022