classification
Title: Patch: some changes to AST to make it more useful for static language analysis
Type: Stage: resolved
Components: None Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: benjamin.peterson, brett.cannon, eric.snow, ezio.melotti, meador.inge, merwok, python-dev, scummos
Priority: normal Keywords: patch

Created on 2012-12-27 21:44 by scummos, last changed 2015-02-02 15:53 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
python.diff scummos, 2012-12-27 21:44
python2.diff scummos, 2013-01-06 20:31 review
readable.diff scummos, 2013-01-10 22:07 review
full.diff scummos, 2013-01-10 22:07 review
full2.diff scummos, 2013-01-10 23:36 review
full3.diff scummos, 2013-01-19 21:00 review
81299-extend-asdl.diff scummos, 2013-01-19 22:14 review
81300-change-var-kwargs.diff scummos, 2013-01-19 22:14
81301-change-attr-ranges.diff scummos, 2013-01-19 22:15
81300-change-var-kwargs-new.diff scummos, 2013-01-29 22:07
81302-adjust-unparser.diff scummos, 2013-03-18 17:17
Messages (44)
msg178339 - (view) Author: Sven Brauch (scummos) * Date: 2012-12-27 21:44
Here's a patch doing some adjustments to the AST to make it more useful for static language analysis, as discussed in http://mail.python.org/pipermail/python-dev/2012-December/123320.html.

Changes done:
 * the described fix to attribute ranges
 * add location information for var / kwargs and arguments

Interestingly, this even fixes a bug; compare the locations of the error in the following situation:

>>> l = [1, 2, 3]
>>> l[
... 
... 2
... 
... ].Foo

Old error message:
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
AttributeError: 'int' object has no attribute 'Foo'

New error message:
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
AttributeError: 'int' object has no attribute 'Foo'

The new message is obviously more accurate (one could even go as far as saying that the first one does not make any sense at all -- what does the expression in the slice have to do with the error?).
The same thing happens in similar situations, e.g. with line continuation characters, function calls, ... anything multi-line with an error related to attribute access.

I hope the patch is okay, if not please let me know what to change. I also hope I managed to include all important changes into the patch ;)
msg179094 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-04 23:54
It would be good if
a) the patch was against hg default
b) it had tests
msg179150 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-05 23:24
While writing tests, I noticed that the additional fields (lineno, col_offset for vararg, kwarg, and other arguments) currently are mandatory. Is that a problem?
It doesn't seem trivial to change that, since apparently only attributes (not fields) can be optional, but those are not allowed by the syntax of python.asdl at this point.
In case the fields need to be mandatory, what would be the correct approach to achieve that?

Thanks.
msg179151 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-05 23:30
A question mark after the type name in the AST makes it optional.
msg179154 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-05 23:58
Thanks. I had seen and tried this before, but the "ast" module in python, which is used in the tests, still requires the additional arguments. Probably this is only valid for the C API?
msg179220 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-06 20:31
Here's a new proposal, I adjusted the AST tests and fixed some small problems I encountered during that. It contains all the diffs for generated files, should I remove those for easier review?
A remaining problem is that AST_Tests::_assertTrueorder now fails, I think because the condition it checks simply is not met any more (by design of the change). What's the correct way to deal with that?
msg179600 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-10 22:07
Here's another version now which I think could be used like this. All tests have been adjusted. I'll append two patches, one just containing the changes to the parser for ease of review, and one full diff which also contains changes to the generated files and the test adjustments.

Please point out any remaining problems you see with this so I can fix them.
msg179601 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-10 22:07
Attached is the full diff this time.
msg179604 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-10 23:36
The patch review tool currently throws errors on submitting any form (http://pastie.org/pastes/5665048/text) so please forgive me for answering here once more. I'll copy this information (patch + message) to the review as soon as the website is working again.

> In ast.c, use the LINENO macro for n_lineno.
Done.

> http://bugs.python.org/review/16795/diff/7080/Lib/test/test_ast.py#newcode183
> Lib/test/test_ast.py:183: def _assertTrueorder(self, ast_node,
parent_pos, reverse_check = False):
> Wrap everything here by 80 chars.
Done.

> http://bugs.python.org/review/16795/diff/7080/Lib/test/test_ast.py#newcode198
> Lib/test/test_ast.py:198: self.assertTrue(node_pos <= parent_pos if
> reverse_check else node_pos >= parent_pos)
> Lift the condition out of the assert call.
Done.

> http://bugs.python.org/review/16795/diff/7080/Lib/test/test_ast.py#newcode467
> Lib/test/test_ast.py:467: self.maxDiff = None
> A comment explaining what this is for would be nice.
Sorry, this was for testing purposes only, and I forgot to remove it.

> http://bugs.python.org/review/16795/diff/7080/Lib/test/test_ast.py#newcode589
> Lib/test/test_ast.py:589: 0, 0, 0, 0)
> These extra parameters are optional now, right? They needn't be passed
> then.
Unfortunately not: Altough the question mark in the asdl file is present and I made fairly sure to regenerate all the derived files, the parameters are still mandatory.

URL of the patch review: http://bugs.python.org/review/16795/
msg179605 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-10 23:51
Could you post an example of the error, please?
msg179606 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-11 00:01
The above post has an example for trying to add a patch, here's what happens when I try to post a reply: http://pastie.org/pastes/5665144/text
I also tried with another web browser, so it's unlikely that it's the browser's fault (but maybe the user's? ;)
msg179607 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-11 00:02
Ah, sorry, I was talking about the failure of optional arguments.
msg179608 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-11 00:04
Ah, whops, I misunderstood that.

The error is rather generic:
Traceback (most recent call last):
  File "./Lib/test/test_ast.py", line 796, in test_lambda
    self._check_arguments(fac, self.expr)
  File "./Lib/test/test_ast.py", line 596, in _check_arguments
    check(arguments(args=args), "must have Load context")
  File "./Lib/test/test_ast.py", line 593, in arguments
    kwarg, kwargannotation, defaults, kw_defaults)
TypeError: arguments constructor takes either 0 or 12 positional arguments

It's very generic in C too:
Python/ast.c:1571:42: error: macro "arguments" requires 13 arguments, but only 9 given
msg179621 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-11 03:52
Ah, yes. This is part of the annoying inconsistency in our asdl framework. Here's what I think should happen:
- on arguments, vararg and kwarg should get the "arg" type, killing some of the numerous fields on arguments
- asdl needs to be hacked, so "arg" can have a lineno and col_offset attributes like the "expr" type.

Sorry this is getting so painful and involved.
msg179737 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-11 22:26
Not an issue, having this thing resolved upstream would save a huge lot of pain elsewhere. ;)

So, to make sure... I'll go to the asdl file, make arguments have two arg attributes which store the data for the var and kwarg which they can contain, then I adjust ast.c to reflect that new structure. Then I go to asdl.py and hack it so we can have attributes ( ... ) on arguments. Does that sound correct?
msg179748 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-12 00:24
Yes. Feel free to do that in separate patches as needed.
msg180116 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-16 23:43
I think I got it mostly working now (it was quite simple in fact), but there's one issue which I can't seem to solve. This fails:

>>> compile(ast.parse("def fun(): pass"), "<file>", "exec")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: required field "arg" missing from arg

However, this succeeds:
>>> compile(ast.parse("def fun(*va, **kwa): pass"), "<file>", "exec")
<code object <module> at 0x7fb390323780, file "<file>", line 1>

The reason is quite simple: vararg and kwarg are optional in arguments, but they're of type arg, and arg has mandatory attributes ("arg", the name of the argument). Still, when calling ast.parse(), Python creates attributes called vararg, kwarg on the "attributes" object, which are set to None:

>>> ast.parse('def fun(): pass').body[0].args.vararg.__repr__()
'None'

Thus, when in compile(), the code in Python_ast.c does "if ( _PyObject_HasAttrId(obj, &PyId_vararg) ) { ... }" this check says "yes there's a vararg" altough there really is none, which leads to the above error message.

I checked the asdl file, and in fact I think this is a general issue, which was not noticed so far, since only things without mandatory attributes are used in conjunction with the question mark "?" operator there (expr and the integral types identifier, int...). Is this correct?

An easy way to solve this problem would be to check whether the attribute is None in Python_ast.c, but I'm everything but sure this is the correct way to fix this. Alternatively, one could not create the attributes on the ast objects when they're not present in the parsed code (i.e. leave the "vararg" attribute nonexistent instead of setting it to none). What should I do about this?
msg180163 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-17 23:50
I think "None" should be treated as meaning not present for an optional argument.

By the way, it would be good if we could get you to sign a contributor agreement. http://www.python.org/psf/contrib/
msg180261 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-19 21:00
Here's the next version which I hope to be somewhat complete now.

vararg and kwarg are now of type arg, and I did all the changes which are required to make this possible. The ast tests pass.

Do you prefer to have this as one large patch all together, or would you rather like to review (and apply) 3..4 patches split into the individual features I implemented?
msg180262 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-19 21:01
Individual patches would be great.
msg180263 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-19 21:08
Alright, I'll be back with those shortly (as soon as I found out how to do this best with hg -- I'm used to git ;). I'll also sign the contributor agreement, that's no problem of course.
msg180266 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-19 22:14
Okay, here they are. I'm not sure how to make hg include a commit message in the patch...

81299-extend-asdl.diff: Changes required to the ASDL framework, in order to allow attributes ( ... ) on a product
81300-change-var-kwargs.diff: Makes var/kwarg be instances of arg, and adds the lineno / col_offset attributes to arg.
81301-change-attr-ranges.diff: Changes power ranges as described in the first post of this report.

All three patches include the corresponding changes to the unit tests, and hopefully the correct set of changes to the generated (Python-ast.h/.c) files.
msg180267 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-19 22:14
second patch file
msg180268 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-19 22:15
third patch file

(... is there a better way to upload three files?)
msg180537 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-24 18:09
I have signed the contributor agreement and sent a scan to the specified mail address (received no reply so far, but I guess that's okay).

Did anyone happen to find the time to look at the patches yet?

Greetings,
Sven
msg180542 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-01-24 19:14
Thanks for signing the agreement. I'll try to look at the patches by the end of this weekend. Sorry for the delay.
msg180611 - (view) Author: √Čric Araujo (merwok) * (Python committer) Date: 2013-01-25 19:25
> I'm not sure how to make hg include a commit message in the patch...
See hg help export.

(In Mercurial, the only objects are changesets; hg log trawls through commit messages (with options to see short text, full text, diff), hg diff only shows diff, and hg export is the command to output full changesets.)
msg180612 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-01-25 19:30
It's not necessary to include the commit message and/or use "hg export" though, since we don't import patches directly and we write the message ourself when we commit.

> (... is there a better way to upload three files?)

You need to upload them individually and "submit changes" 3 times, but it's not necessary to write a comment every time.  Also, unless the 3 patches are independent, it's usually better to upload a single diff that includes all the necessary changes.
msg180953 - (view) Author: Sven Brauch (scummos) * Date: 2013-01-29 22:07
Hm, I'm still getting the same error messages from the review tool which I described earlier; I can neither comment nor add patches.

So, I'll have to abuse the bug report again:
Thanks for the review. Is it possible you selected the wrong patch file for the second patch (Patch Set 5)? It seems to include all changes instead of just those from the second of the three patches I submitted. Also I had already removed the traceback.print_stack() call.

I fixed the other two issues and I will attach a corrected version of the second patch for review. I hope I got everything right ;)

Please have an extra close look at the changes to symtable.c and compile.c (since I'm not very familiar with that code), in order to avoid that we break stuff with this.

Cheers,
Sven
msg181609 - (view) Author: Sven Brauch (scummos) * Date: 2013-02-07 14:22
Any news on this yet? ;)

Unfortunately, I'm still having no luck in adding the patch to the review tool (same error message).
msg181612 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-02-07 14:43
Are you attaching files directly on Rietveld or on this issue?
msg181613 - (view) Author: Sven Brauch (scummos) * Date: 2013-02-07 14:51
Attaching files to this bug report here works fine (see corrected patch above), but when I add the file to http://bugs.python.org/review/16795/ under "Add another patchset", I get the error message I described. I tried with firefox, chromium and konqueror.
msg181614 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-02-07 14:53
Yeah, I think that's broken. It's best just to attach them here.
msg181615 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-02-07 14:55
It's just not supported -- the "Add another patchset" link should be removed from rietveld.
msg181616 - (view) Author: Sven Brauch (scummos) * Date: 2013-02-07 15:00
Oh, alright, that explains things. In this case, the file I attached on Jan 29 (http://bugs.python.org/file28905/81300-change-var-kwargs-new.diff) should contain all the requested changes.

Greetings
msg182127 - (view) Author: Sven Brauch (scummos) * Date: 2013-02-15 01:37
I don't want to push anything, but did you find time to review this yet? It would be great to have it in the next release.
msg182128 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-02-15 01:55
Python 3.4.0a1 isn't due until August so you have no worries about missing the next release. =)
msg184472 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-18 16:56
Hi Sven, I was about to apply this (sorry for the delay), and I realized there's one more thing. We have an example AST unparser in Tools/parser that needs to be updated for AST changes. You can run it's test suite by running test_tools in the main test suite.
msg184475 - (view) Author: Sven Brauch (scummos) * Date: 2013-03-18 17:17
Hi Benjamin,

the delay is not a problem -- as long as this is in time for Python 3.4, everything is fine.

Attached is a patch which adjusts the unparser to the changes. Acoording to the tests, this is all that needs to be updated.

Cheers,
Sven
msg184480 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-18 17:50
New changeset 7c5c678e4164 by Benjamin Peterson in branch 'default':
unify some ast.argument's attrs; change Attribute column offset (closes #16795)
http://hg.python.org/cpython/rev/7c5c678e4164
msg184481 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-18 17:53
New changeset 219c997b880b by Benjamin Peterson in branch 'default':
add Sven Brauch for his #16795 contribution
http://hg.python.org/cpython/rev/219c997b880b
msg184485 - (view) Author: Sven Brauch (scummos) * Date: 2013-03-18 18:32
Thanks for reviewing this, and thanks for guiding me through the implementation process. ;)
msg235267 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-02 15:53
New changeset 7d1c32ddc432 by Benjamin Peterson in branch '3.4':
revert lineno and col_offset changes from #16795 (closes #21295)
https://hg.python.org/cpython/rev/7d1c32ddc432
msg235268 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-02-02 15:53
People pointed out in #21295 that this made some things that were possible before impossible, so the lineno and col_offset changes of this have been reverted.
History
Date User Action Args
2015-02-02 15:53:56benjamin.petersonsetmessages: + msg235268
2015-02-02 15:53:31python-devsetmessages: + msg235267
2013-03-18 18:32:15scummossetmessages: + msg184485
2013-03-18 17:53:53python-devsetmessages: + msg184481
2013-03-18 17:50:17python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg184480

resolution: fixed
stage: resolved
2013-03-18 17:17:27scummossetfiles: + 81302-adjust-unparser.diff

messages: + msg184475
2013-03-18 16:56:23benjamin.petersonsetmessages: + msg184472
2013-02-15 01:55:42brett.cannonsetmessages: + msg182128
2013-02-15 01:37:25scummossetmessages: + msg182127
2013-02-08 04:19:05benjamin.petersonsetassignee: benjamin.peterson
2013-02-07 15:00:10scummossetmessages: + msg181616
2013-02-07 14:55:18ezio.melottisetmessages: + msg181615
2013-02-07 14:53:08benjamin.petersonsetmessages: + msg181614
2013-02-07 14:51:57scummossetmessages: + msg181613
2013-02-07 14:43:30benjamin.petersonsetmessages: + msg181612
2013-02-07 14:22:48scummossetmessages: + msg181609
2013-01-29 22:07:27scummossetfiles: + 81300-change-var-kwargs-new.diff

messages: + msg180953
2013-01-25 19:30:20ezio.melottisetnosy: + ezio.melotti
messages: + msg180612
2013-01-25 19:25:00merwoksetnosy: + merwok
messages: + msg180611
2013-01-24 19:14:18benjamin.petersonsetmessages: + msg180542
2013-01-24 18:09:22scummossetmessages: + msg180537
2013-01-19 22:15:09scummossetfiles: + 81301-change-attr-ranges.diff

messages: + msg180268
2013-01-19 22:14:52scummossetfiles: + 81300-change-var-kwargs.diff

messages: + msg180267
2013-01-19 22:14:36scummossetfiles: + 81299-extend-asdl.diff

messages: + msg180266
2013-01-19 21:08:53scummossetmessages: + msg180263
2013-01-19 21:01:58benjamin.petersonsetmessages: + msg180262
2013-01-19 21:00:51scummossetfiles: + full3.diff

messages: + msg180261
2013-01-17 23:50:39benjamin.petersonsetmessages: + msg180163
2013-01-16 23:43:25scummossetmessages: + msg180116
2013-01-12 00:24:48benjamin.petersonsetmessages: + msg179748
2013-01-11 22:26:10scummossetmessages: + msg179737
2013-01-11 03:52:54benjamin.petersonsetmessages: + msg179621
2013-01-11 00:04:22scummossetmessages: + msg179608
2013-01-11 00:02:20benjamin.petersonsetmessages: + msg179607
2013-01-11 00:01:36scummossetmessages: + msg179606
2013-01-10 23:51:23benjamin.petersonsetmessages: + msg179605
2013-01-10 23:36:09scummossetfiles: + full2.diff

messages: + msg179604
2013-01-10 22:07:27scummossetfiles: + full.diff

messages: + msg179601
2013-01-10 22:07:04scummossetfiles: + readable.diff

messages: + msg179600
2013-01-06 20:31:33scummossetfiles: + python2.diff

messages: + msg179220
2013-01-05 23:58:06scummossetmessages: + msg179154
2013-01-05 23:30:22benjamin.petersonsetmessages: + msg179151
2013-01-05 23:24:10scummossetmessages: + msg179150
2013-01-04 23:54:11benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg179094
2012-12-29 02:36:57meador.ingesetnosy: + meador.inge
2012-12-28 19:52:04brett.cannonsetnosy: + brett.cannon
2012-12-28 05:31:19eric.snowsetnosy: + eric.snow
2012-12-27 21:44:43scummoscreate