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

louisom · 2017-05-01T14:47:37Z

This will not compare with attributes, but fields and types.

https://bugs.python.org/issue15987

serhiy-storchaka

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

serhiy-storchaka · 2017-05-01T20:03:35Z

Lib/ast.py

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


return NotImplemented

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

serhiy-storchaka · 2017-05-01T20:04:51Z

Lib/ast.py

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


Isn't this recursive?

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

serhiy-storchaka · 2017-05-01T20:05:31Z

Lib/ast.py

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


Why not getattr?

Thanks for point out.

serhiy-storchaka · 2017-05-01T20:06:05Z

Lib/ast.py

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


Actually this isn't needed. See below.

serhiy-storchaka · 2017-05-01T20:07:18Z

Lib/ast.py

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


Fail fast.

if i != j: return False

But stop, why not compare just lists?

if af != bf: return False

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.

Thus we will need to recursively to compare the items.

serhiy-storchaka · 2017-05-01T20:09:48Z

Lib/ast.py

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


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

serhiy-storchaka · 2017-05-01T20:10:14Z

Lib/ast.py

+
+
+for n in _NODES:
+    n.__eq__ = ast_eq_compare


Why not patch just the base class?

@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

serhiy-storchaka · 2017-05-01T20:12:01Z

Lib/ast.py


+# 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))]


'ASTPyCF_ONLY_AST' is the single literal.

Change to ('AST', 'PyCF_ONLY_AST')

serhiy-storchaka · 2017-05-01T20:14:06Z

Lib/ast.py



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


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

serhiy-storchaka · 2017-05-01T20:18:11Z

Lib/ast.py



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


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

louisom · 2017-05-02T06:45:00Z

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

louisom · 2017-05-02T09:27:28Z

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

tonybaloney · 2019-05-06T15:39:09Z

hi, this PR has been open for some time and the discussion in https://bugs.python.org/issue15987 is inconclusive.
Please can the team review this BPO

vstinner · 2019-05-10T01:57:45Z

cc @methane

vstinner · 2019-05-10T01:58:16Z

@louisom: Can you rebase this PR on master?

tonybaloney · 2019-05-10T02:06:18Z

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

vstinner · 2019-05-10T02:39:07Z

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

https://github.com/louisom user still exists. I understand that he deleted its fork of CPython.

It's possible to download this PR as a patch using the URL:
https://patch-diff.githubusercontent.com/raw/python/cpython/pull/1368.patch

Is there someone interested to convert this old PR into a newer PR?

flavianh · 2019-07-20T10:25:19Z

@vstinner I can try!

flavianh · 2019-07-27T09:42:51Z

@vstinner Recreated at #14970

csabella · 2020-01-14T12:06:35Z

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

bpo-15987: Add AST nodes eq & ne compare methods

f3457b4

the-knights-who-say-ni added the CLA signed label May 1, 2017

Skip complex ast compare when get unicode decode error

84241a3

serhiy-storchaka reviewed May 1, 2017

View reviewed changes

brettcannon added the type-feature A feature request or enhancement label May 1, 2017

louisom added 3 commits May 2, 2017 12:22

Add base type richcompare methods

b47c6fd

Add comments

f2c3655

Add corner case and fix richcompare

aa92d32

louisom added 6 commits May 2, 2017 15:32

Add more descriptive assertEqual for debugging

886fc47

Debugging with travis

5ec28b2

Debugging test_ast

213111c

Remove debugging

8a36d80

Move code to asdl_c.py

b42207c

Revert change for Python-ast.c

033f245

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

louisom added 2 commits May 2, 2017 17:38

Fix style and add comment

b85ff63

Add re-generated Python-ast.c

acd7ab7

taion mentioned this pull request Jan 4, 2018

Re-enable AST_COMPARE prettier/plugin-python#17

Merged

brettcannon added the awaiting review label Feb 2, 2018

flavianh mentioned this pull request Jul 26, 2019

bpo-15987: Add ast.AST class richcompare methods #14970

Closed

csabella closed this Jan 14, 2020

Julian mannequin mentioned this pull request Apr 10, 2022

Provide a way to compare AST nodes for equality recursively #60191

Open



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

Uh oh!

Comments

Conversation

louisom commented May 1, 2017 • edited by vstinner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisom May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisom commented May 2, 2017

Uh oh!

louisom commented May 2, 2017

Uh oh!

tonybaloney commented May 6, 2019

Uh oh!

vstinner commented May 10, 2019

Uh oh!

vstinner commented May 10, 2019

Uh oh!

tonybaloney commented May 10, 2019

Uh oh!

vstinner commented May 10, 2019

Uh oh!

flavianh commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flavianh commented Jul 27, 2019

Uh oh!

csabella commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

louisom commented May 1, 2017 •

edited by vstinner

Loading

louisom May 2, 2017 •

edited

Loading

flavianh commented Jul 20, 2019 •

edited

Loading