bpo-38870: Implement Simple Preceding to AST Unparser

[isidentical]

cc: @pablogsal

https://bugs.python.org/issue38870

[isidentical]

Do we need news entry for this fix?

[isidentical]

I am not sure why this exists and what it does but it seems wrong to me, at least with checking everything in ast.

cpython/Lib/test/test_ast.py

Lines 326 to 331 in ded8888

	def test_field_attr_existence(self):
	for name, item in ast.__dict__.items():
	if isinstance(item, type) and name != 'AST' and name[0].isupper():
	x = item()
	if isinstance(x, ast.AST):
	self.assertEqual(type(x._fields), tuple)

It was failing because of both _Levels and IntEnum. I first thinked an extra condition about if name not starts with underscore but it doesnt worked because of IntEnum. So I moved instance of AST check upper.

[isidentical]

@pablogsal is it worth to mention there will be an extra field in certain nodes called _precedence_level so after unparsing the tree will be mutated. Maybe we can strip out that field by traversing tree with ast.walk after generating result? (but that would definitely slow down)

[pablogsal]

I still didn't have time to go to the full PR but some general comments:

• I find the diff very difficult to read. You rearranged many functions and change signatures so the diff is bigger than it should be and that makes it complicated to read the changes and how the feature is implemented. Please, try to change only the relevant things for this issue in particular.

• The class should not modify the ast object that you pass into or mutate it in any way, so please, do not attach attributes to the nodes (like _precedence_level) or do any mutation over the tree.

• Bringing enum into the ast module will likely introduce another performance regression (please, measure).

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[isidentical]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

[pablogsal]

I'm currently handling some urgent regressions on buildbots, so it will take me some time to come back to this :(

[isidentical]

Thanks for reviews @vstinner, can I get an DO NOT MERGE label until the other PR I'm going to open is merged? It is going to look messy until then.

[vstinner]

Thanks for reviews @vstinner, can I get an DO NOT MERGE label until the other PR I'm going to open is merged? It is going to look messy until then.

You can add [WIP] to the PR title.

[isidentical]

@pablogsal @vstinner #17612 is merged and I rewrote this preceding patch with basing that delimiting changes. The diff is much more minimal right now. This patch tries to implement the preceding system that was also used in ast_unparse.c and aims to remove unnecesary parens arround the expressions.

[pablogsal]

This looks good to me with the latest corrections!

Great job, @isidentical! 🎉

[vstinner]

Well done @isidentical. I'm happy to see that the merged change now uses the enum module :-)