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

gh-60191: Implement ast.compare #19211

Merged
merged 38 commits into from
May 22, 2024
Merged

Conversation

isidentical
Copy link
Member

@isidentical isidentical commented Mar 28, 2020

Lib/ast.py Outdated Show resolved Hide resolved
Lib/ast.py Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
Copy link

cpython-cla-bot bot commented May 11, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@AlexWaygood AlexWaygood changed the title bpo-15987: Implement ast.compare gh-60191: Implement ast.compare May 11, 2024
@AlexWaygood AlexWaygood requested a review from Eclips4 May 11, 2024 20:27
Use separate helper functions to compare _fields and _attributes, because
_attributes are always simple strings.

Move the type comparison test to the point where it is relevant,
rather than making unnecessary type tests.

Add comments to explain the logic in more detail.
Copy link
Contributor

@jeremyhylton jeremyhylton left a comment

Choose a reason for hiding this comment

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

I added a few comments, but realized I wanted to try a slightly revised implementation to answer the questions I had. I'll share the revision in a moment.

Doc/library/ast.rst Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
The basic functionality here is to compare asts recursively. If
compare_fields is False, then the comparison is not recursive. It just
compares that the top-level AST objects are the same and have the same
attributes. This option doesn't seem interesting enough to offer as a
feature.

Revise the docstring for compare() to explain the options in more detail.
Doc/library/ast.rst Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
jeremyhylton and others added 3 commits May 21, 2024 11:58
In practice the primary effect of this change was for Constant() nodes
where the value of two constants were equal without being the same
literal type, e.g. ast.compare(Constant(1), Constant(True),
compare_types=True) was True.

This behavior doesn't seem like it has a very strong motivation, and
adds some complexity. We can add it back later if there's a clear need
for it.
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
jeremyhylton and others added 3 commits May 21, 2024 14:53
The comparison fundamentally depends on _fields and _attributes, which
could be modified by the user. It's not clear that such modifications
are sensible or supported by the API, but we can at least make sure
comparison doesn't silently ignore those comparisons.

Also pass a and b as arguments to helper methods instead of using them
from the enclosing scope.
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@isidentical
Copy link
Member Author

hey @jeremyhylton, i am currently a bit busy with non-oss stuff so if you'd like feel free to take over the PR (and close this one). whatever is the most convenient (sorry for the CLA being blocked, @ambv just made me aware).

Lib/test/test_ast.py Outdated Show resolved Hide resolved
Lib/test/test_ast.py Show resolved Hide resolved
jeremyhylton and others added 2 commits May 21, 2024 15:19
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
jeremyhylton and others added 4 commits May 21, 2024 15:55
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@jeremyhylton jeremyhylton enabled auto-merge (squash) May 21, 2024 20:02
@jeremyhylton jeremyhylton disabled auto-merge May 21, 2024 20:03
Doc/library/ast.rst Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@jeremyhylton jeremyhylton enabled auto-merge (squash) May 21, 2024 20:12
@jeremyhylton jeremyhylton merged commit d065edf into python:main May 22, 2024
36 checks passed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
* bpo-15987: Implement ast.compare

Add a compare() function that compares two ASTs for structural equality. There are two set of attributes on AST node objects, fields and attributes. The fields are always compared, since they represent the actual structure of the code. The attributes can be optionally be included in the comparison. Attributes capture things like line numbers of column offsets, so comparing them involves test whether the layout of the program text is the same. Since whitespace seems inessential for comparing ASTs, the default is to compare fields but not attributes.

ASTs are just Python objects that can be modified in arbitrary ways. The API for ASTs is under-specified in the presence of user modifications to objects. The comparison respects modifications to fields and attributes, and to _fields and _attributes attributes. A user could create obviously malformed objects, and the code will probably fail with an AttributeError when that happens. (For example, adding "spam" to _fields but not adding a "spam" attribute to the object.) 

Co-authored-by: Jeremy Hylton <jeremy@alum.mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants