[issue10769] ast: provide more useful range information

[R. David Murray]

R. David Murray added the comment:

Thanks.

--
stage: needs patch -> resolved

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch added the comment:

Yes, this issue can be closed.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[R. David Murray]

R. David Murray added the comment:

OK, so that patch was committed.  Does that mean this one can be close?  (I'm 
not familiar with the code in question.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch added the comment:

Hi,

Mailing list thread: 
https://mail.python.org/pipermail/python-dev/2012-December/123320.html
Discussion on the patch: http://bugs.python.org/issue16795

Greetings,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[R. David Murray]

R. David Murray added the comment:

If there was a python-ideas or python-dev thread that resulted in consensus 
approval of this change, can someone post a link to it?

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Ethan Furman]

Changes by Ethan Furman :


--
nosy: +ethan.furman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Michael]

Michael  added the comment:

Hi,

Attached is the updated patch by Sven Brauch from the original mailing list 
thread bringing column offset reporting for attributes in line with everything 
else.

The offsets for bar before the patch:
foo[bar] = 4
foo(bar) = 4
foo.bar = 0

After:

foo[bar] = 4
foo(bar) = 4
foo.bar = 4

With the update, are there still concerns?

--
keywords: +patch
nosy: +kensington
Added file: http://bugs.python.org/file25275/fix-attr-ranges.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Okay, thank you, I'm going to do that. :)

Bye,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Benjamin Peterson]

Benjamin Peterson  added the comment:

I suggest you mail python-dev or python-ideas. I find it more consistent as it 
stands now.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Hi,

yeah Terry, that's exactly what most people whom I talked about this said (me 
too).

Anyway, here's the patch which -- in my opinion -- fixes this behavior:

--- python-orig/Python/ast.c 2010-10-19 03:22:07.0 +0200
+++ python-ast-fix/Python/ast.c 2010-12-26 13:25:48.0 +0100
@@ -1742,8 +1742,6 @@
 tmp = ast_for_trailer(c, ch, e);
 if (!tmp)
 return NULL;
-tmp->lineno = e->lineno;
-tmp->col_offset = e->col_offset;
 e = tmp;
 }
 if (TYPE(CHILD(n, NCH(n) - 1)) == factor) {

The offsets for "foo.bar.baz" before the patch:

[1, 0, <_ast.Attribute>]
[1, 0, <_ast.Attribute>, 'baz']
[1, 0, <_ast.Name>, 'bar']
[1, 0, 'foo']

... and after the patch:

[1, 0, <_ast.Attribute>]
[1, 7, <_ast.Attribute>, 'baz']
[1, 3, <_ast.Name>, 'bar']
[1, 0, 'foo']

It would really be great if that could be applied.

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Terry J. Reedy]

Terry J. Reedy  added the comment:

FWIW, I find the current behavior for attributes to be surprising, to the point 
where at first glance it almost looks like a bug. Which is to say, I would have 
expected 'col' to point to the first non-whitespace column after the '.'. If 
there are multiple attributes, a.b.c.d, then to me, each node should point to 
its appropriate place, just as with a[b][c].

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Then you'd get the point where foo starts instead of the location of the 
opening brace. Sounds good for me.

Also, I already said that, with the change I proposed, I do not see any case 
where relevant information would be lost.

Your argument that it would not be any more useful than now is simply invalid, 
because I got an application here which would work with, but not without the 
change, and I didn't yet see one for the other side.

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2010/12/25 Sven Brauch :
>
> Sven Brauch  added the comment:
>
> Well, weather it's supposed to or not, it *does* contain the line number 
> information:
> 
> For your example, the AST for "foo" tells you the offset for foo. If you want 
> to know the offset (well, "offset") for blah, why not look at foo? Currently, 
> the information is just copied from foo to blah.

What if it's like this, though?

x = (foo).blah

>
> Anyway, I'd like to get away from this abstract discussion which is not 
> likely to yield a result... is there any reason not to do it like I suggested 
> other than the philosophical "it's wrong"? Or don't you agree that the 
> information given this way would be more useful?

It wouldn't be any more useful than it is now. I don't think it's
reasonably possible to preserve every last obscure formatting in ast.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Well, weather it's supposed to or not, it *does* contain the line number 
information:

For your example, the AST for "foo" tells you the offset for foo. If you want 
to know the offset (well, "offset") for blah, why not look at foo? Currently, 
the information is just copied from foo to blah.

Anyway, I'd like to get away from this abstract discussion which is not likely 
to yield a result... is there any reason not to do it like I suggested other 
than the philosophical "it's wrong"? Or don't you agree that the information 
given this way would be more useful?

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2010/12/25 Sven Brauch :
>
> Sven Brauch  added the comment:
>
> Hi,
>
> I agree that the current behavior is not wrong or such. It's just entirely 
> useless.
> For all the other types of subscripts, such as a[b][c][d] or a(b)(c)(d), the 
> ranges of the actual subscript ASTs are also nulled, but there's an 
> additional Name (or whatever) AST created for b, c, and d which contains 
> their ranges. Why isn't it like this for attributes?

Because those can take any Python expression. Attributes only ever take a name.

>
> I also don't really see a reason why those ranges shouldn't be changed for 
> all possible cases (subscript / call / attribute). It will contain more 
> information while not taking any more memory or such, and it will be more 
> intuitive. You can still get the information on where the expression starts 
> by going up the chain to the next Expression AST node, which seems much more 
> logical to me than storing this -- rarely useful -- information redundantly 
> in maybe dozens of nodes. After all, the ast module is meant for "end users" 
> to base applications on it, isn't it?

Expr is just a container node. It's not supposed to store extra information.

>
> Otherwise, you should at least be consistent and remove that range 
> information from the tree completely, because there's absolutely *no* case in 
> which it would ever be useful (at least I cannot think of one).

That's not the case here:

x = foo.blah

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Hi,

I agree that the current behavior is not wrong or such. It's just entirely 
useless.
For all the other types of subscripts, such as a[b][c][d] or a(b)(c)(d), the 
ranges of the actual subscript ASTs are also nulled, but there's an additional 
Name (or whatever) AST created for b, c, and d which contains their ranges. Why 
isn't it like this for attributes?

I also don't really see a reason why those ranges shouldn't be changed for all 
possible cases (subscript / call / attribute). It will contain more information 
while not taking any more memory or such, and it will be more intuitive. You 
can still get the information on where the expression starts by going up the 
chain to the next Expression AST node, which seems much more logical to me than 
storing this -- rarely useful -- information redundantly in maybe dozens of 
nodes. After all, the ast module is meant for "end users" to base applications 
on it, isn't it?

Otherwise, you should at least be consistent and remove that range information 
from the tree completely, because there's absolutely *no* case in which it 
would ever be useful (at least I cannot think of one).

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2010/12/24 Sven Brauch :
>
> Sven Brauch  added the comment:
>
> Hi,
>
> I found the reason for this behavior in the code now, it's in Python/ast.c, 
> lines 1745 and 1746 in ast_for_power():
>
>        tmp->lineno = e->lineno;
>        tmp->col_offset = e->col_offset;
>
> Here, the range information for the individual attributes (which is correctly 
> set before!) is being discarded and replaced by the useless information from 
> the expression ast.
> I don't see any reason for this, is there one? :)
>
> Removing those two lines doesn't seem to break anything and sets ranges 
> correctly.

The ranges are correct. They just aren't what you want. The attribute
starts at the beginning of the power, not the ".".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Hi,

I found the reason for this behavior in the code now, it's in Python/ast.c, 
lines 1745 and 1746 in ast_for_power():

tmp->lineno = e->lineno;
tmp->col_offset = e->col_offset;

Here, the range information for the individual attributes (which is correctly 
set before!) is being discarded and replaced by the useless information from 
the expression ast.
I don't see any reason for this, is there one? :)

Removing those two lines doesn't seem to break anything and sets ranges 
correctly.

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Hi Terry,

well, the current behaviour is... logical in some way, as it says "the whole 
expression which accesses an attribute starts at column 0", i.e. it's easy to 
understand why it's done like this. It just turns out that this is pretty 
useless...

I'll try to find out what changes would be needed in the code to make the 
col_offset attribute useful in this case.

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Terry J. Reedy]

Terry J. Reedy  added the comment:

A request limited only to fixing the current field for attribute may get more 
traction than a request for a new field. Can you dig into to code to get any 
idea why the difference between attributes versus indexes and parameters?

--
nosy: +terry.reedy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

Sven Brauch  added the comment:

Hi,

well, but you have to agree that there is no point in setting a correct column 
offset for bar in 
foo[bar] (it's correctly set to 4 here!)
and
foo(bar)
but not for
foo.bar (there's no information provided here)
For me, this looks like it was just done the easiest way -- which is okay, of 
course, but is not a valid reason not to change it now.
It's also not that there's fifty things missing to make it fully functional for 
the purpose of highlighting / analyzing the code, this is about the only one. :)

Best regards,
Sven

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Raymond Hettinger]

Raymond Hettinger  added the comment:

If info fields are added, they need to be optional so that someone manipulating 
the tree (adding, rearranging, or removing nodes) doesn't have an additional 
burden of supplying this info before compiling into an executable code object.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2010/12/24 Raymond Hettinger :
>
> Raymond Hettinger  added the comment:
>
> ISTM the whole point of an Abstract Syntax Tree is to express semantics while 
> throwing away the syntax details.  The only reason any position information 
> is kept is to support tracebacks and debugging.

I agree for the most part. It's also nearly impossible to preserve all
the detail one would want in an *abstract* syntax tree.

>
> Perhaps the OP's request should changed to "add function to concrete parse 
> trees to show structure while preserving all the input detail returned by 
> tokenize."

If someone can comment on what that would look like...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Raymond Hettinger]

Raymond Hettinger  added the comment:

ISTM the whole point of an Abstract Syntax Tree is to express semantics while 
throwing away the syntax details.  The only reason any position information is 
kept is to support tracebacks and debugging.  

Perhaps the OP's request should changed to "add function to concrete parse 
trees to show structure while preserving all the input detail returned by 
tokenize."

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[R. David Murray]

Changes by R. David Murray :


--
nosy: +benjamin.peterson
stage:  -> needs patch
versions: +Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue10769] ast: provide more useful range information

[Sven Brauch]

New submission from Sven Brauch :

Hi,

I'm writing a python language support plugin for an IDE. I'm using the AST 
module to get information about the loaded source code, which works pretty 
well. However, in some cases, the information provided by the AST is simply not 
sufficient to do proper highlighting (or whatever); for example, 

class_instance.method(argument1.arg_attribute).attribute
class_instance.method(   argument1.arg_attribute). attribute
and
class_instance.method(   argument1. \
 arg_attribute)   \
   . attribute

produce exactly the same syntax tree, making it impossible to determine where 
e.g. "attribute" starts and ends. Although technically obviously correct, the 
information that the column offset for "attribute" is "0" is quite pointless.

It would really be great if there could be an additional attribute for 
Attribute Access nodes which tells me where the attribute access the node 
refers to *really* takes place, not just "column 0". :)
It would be even better if there was not only an offset, but also an "end" 
attribute providing information on where the node ends; that'd make this module 
way more useful, at least for me.

Thanks and best regards,
Sven

--
components: Library (Lib)
messages: 124596
nosy: georg.brandl, scummos
priority: normal
severity: normal
status: open
title: ast: provide more useful range information
type: feature request

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com