Patch: some changes to AST to make it more useful for static language analysis

@brettcannon

BPO	16795
Nosy	@brettcannon, @benjaminp, @ezio-melotti, @merwok, @meadori, @ericsnowcurrently
Files	python.diff python2.diff readable.diff full.diff full2.diff full3.diff 81299-extend-asdl.diff 81300-change-var-kwargs.diff 81301-change-attr-ranges.diff 81300-change-var-kwargs-new.diff 81302-adjust-unparser.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/benjaminp'
closed_at = <Date 2013-03-18.17:50:17.629>
created_at = <Date 2012-12-27.21:44:43.601>
labels = []
title = 'Patch: some changes to AST to make it more useful for static language analysis'
updated_at = <Date 2015-02-02.15:53:56.247>
user = 'https://bugs.python.org/scummos'

bugs.python.org fields:

activity = <Date 2015-02-02.15:53:56.247>
actor = 'benjamin.peterson'
assignee = 'benjamin.peterson'
closed = True
closed_date = <Date 2013-03-18.17:50:17.629>
closer = 'python-dev'
components = ['None']
creation = <Date 2012-12-27.21:44:43.601>
creator = 'scummos'
dependencies = []
files = ['28462', '28596', '28679', '28680', '28681', '28786', '28787', '28788', '28789', '28905', '29445']
hgrepos = []
issue_num = 16795
keywords = ['patch']
message_count = 44.0
messages = ['178339', '179094', '179150', '179151', '179154', '179220', '179600', '179601', '179604', '179605', '179606', '179607', '179608', '179621', '179737', '179748', '180116', '180163', '180261', '180262', '180263', '180266', '180267', '180268', '180537', '180542', '180611', '180612', '180953', '181609', '181612', '181613', '181614', '181615', '181616', '182127', '182128', '184472', '184475', '184480', '184481', '184485', '235267', '235268']
nosy_count = 8.0
nosy_names = ['brett.cannon', 'benjamin.peterson', 'ezio.melotti', 'eric.araujo', 'meador.inge', 'scummos', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue16795'
versions = ['Python 3.4']

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 ;)

It would be good if
a) the patch was against hg default
b) it had tests

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.

A question mark after the type name in the AST makes it optional.

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?

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?

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.

Attached is the full diff this time.

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/

Could you post an example of the error, please?

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? ;)

Ah, sorry, I was talking about the failure of optional arguments.

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

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.

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?

Yeah, I think that's broken. It's best just to attach them here.

It's just not supported -- the "Add another patchset" link should be removed from rietveld.

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

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.

Python 3.4.0a1 isn't due until August so you have no worries about missing the next release. =)

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.

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

New changeset 7c5c678e4164 by Benjamin Peterson in branch 'default':
unify some ast.argument's attrs; change Attribute column offset (closes bpo-16795)
http://hg.python.org/cpython/rev/7c5c678e4164

New changeset 219c997b880b by Benjamin Peterson in branch 'default':
add Sven Brauch for his bpo-16795 contribution
http://hg.python.org/cpython/rev/219c997b880b

Thanks for reviewing this, and thanks for guiding me through the implementation process. ;)

New changeset 7d1c32ddc432 by Benjamin Peterson in branch '3.4':
revert lineno and col_offset changes from bpo-16795 (closes bpo-21295)
https://hg.python.org/cpython/rev/7d1c32ddc432

People pointed out in bpo-21295 that this made some things that were possible before impossible, so the lineno and col_offset changes of this have been reverted.

Navigation Menu

Repository navigation

Patch: some changes to AST to make it more useful for static language analysis #60999

17 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Patch: some changes to AST to make it more useful for static language analysis #60999

Description

Activity

scummos commented on Dec 27, 2012

benjaminp commented on Jan 4, 2013

scummos commented on Jan 5, 2013

benjaminp commented on Jan 5, 2013

scummos commented on Jan 5, 2013

scummos commented on Jan 6, 2013

scummos commented on Jan 10, 2013

scummos commented on Jan 10, 2013

scummos commented on Jan 10, 2013

benjaminp commented on Jan 10, 2013

scummos commented on Jan 11, 2013

benjaminp commented on Jan 11, 2013

scummos commented on Jan 11, 2013

benjaminp commented on Jan 11, 2013

scummos commented on Jan 11, 2013

17 remaining items

benjaminp commented on Feb 7, 2013

ezio-melotti commented on Feb 7, 2013

scummos commented on Feb 7, 2013

scummos commented on Feb 15, 2013

brettcannon commented on Feb 15, 2013

benjaminp commented on Mar 18, 2013

scummos commented on Mar 18, 2013

python-dev commented on Mar 18, 2013

python-dev commented on Mar 18, 2013

scummos commented on Mar 18, 2013

python-dev commented on Feb 2, 2015

benjaminp commented on Feb 2, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions