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
ast: provide more useful range information #54978
Comments
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 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". :) Thanks and best regards, |
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." |
2010/12/24 Raymond Hettinger <report@bugs.python.org>:
I agree for the most part. It's also nearly impossible to preserve all
If someone can comment on what that would look like... |
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. |
Hi, well, but you have to agree that there is no point in setting a correct column offset for bar in Best regards, |
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? |
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, |
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():
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. Removing those two lines doesn't seem to break anything and sets ranges correctly. Best regards, |
2010/12/24 Sven Brauch <report@bugs.python.org>:
The ranges are correct. They just aren't what you want. The attribute |
Hi, I agree that the current behavior is not wrong or such. It's just entirely useless. 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, |
2010/12/25 Sven Brauch <report@bugs.python.org>:
Because those can take any Python expression. Attributes only ever take a name.
Expr is just a container node. It's not supposed to store extra information.
That's not the case here: x = foo.blah |
Well, weather it's supposed to or not, it *does* contain the line number information: 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, |
2010/12/25 Sven Brauch <report@bugs.python.org>:
What if it's like this, though? x = ( foo).blah
It wouldn't be any more useful than it is now. I don't think it's |
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, |
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]. |
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.000000000 +0200
+++ python-ast-fix/Python/ast.c 2010-12-26 13:25:48.000000000 +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>] ... and after the patch: [1, 0, <_ast.Attribute>] It would really be great if that could be applied. Best regards, |
I suggest you mail python-dev or python-ideas. I find it more consistent as it stands now. |
Okay, thank you, I'm going to do that. :) Bye, |
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: After: foo[bar] = 4 With the update, are there still concerns? |
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? |
Hi, Mailing list thread: https://mail.python.org/pipermail/python-dev/2012-December/123320.html Greetings, |
OK, so that patch was committed. Does that mean this one can be close? (I'm not familiar with the code in question.) |
Yes, this issue can be closed. |
Thanks. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: