bpo-44896: drop lineno requirement on nodes for ast.unparse #27770
Conversation
|
@pablogsal do you think we should backport this to 3.9? I don't see this as a bug, so not sure if we should document this as a change on |
|
I'm +1 with backport, I see it more as a bigfix but I trust your decision :) |
|
As the original issue creator I hope I can way in to suggest we backport it, as I don’t think this is a feature, it’s more about fixing the unexpected behavior of the module. |
Lib/ast.py
Outdated
| lineno = getattr(node, 'lineno', None) | ||
| if lineno is None: | ||
| return None | ||
|
|
||
| comment = self._type_ignores.get(lineno) or node.type_comment |
There was a problem hiding this comment.
I'm not so sure about this. The crash before at least made the user call fix_missing_locations which solves the problem. Now you're suggesting to change the behavior in possibly invalid ways:
- if there is a
node.type_commentset but line number isn't, now it will be silently not returned; - if there is a matching type ignore in
self._type_ignore, it won't be found because the location information is missing and no type comment will be returned.
Since the new behavior makes the code silently miss returning type comments without any indication as to why, the programmer might have trouble finding why this is.
6016538 to
ba29911
Compare
There was a problem hiding this comment.
I think we should just let this go and instead advertise that if you put new synthetic nodes in a tree, you have to either provide lineno and col_offset yourself, or use fix_missing_locations() to fill out the information for you.
The problem with the logic that I commented on twice now is one reason for my opinion. Another is bigger though:
Suppose user code is replacing nodes or just inserting new ones. This operation not only creates nodes without location attributes. It also in all likelihood moves locations of type comments and those aren't updated (for example, _type_ignores[123] should now really be _type_ignores[125] to still point at the same tokens). So user code shouldn't be naive when it comes to missing locations, and we definitely shouldn't paper over missing locations.
| comment = None | ||
| if node.type_comment is not None: | ||
| comment = node.type_comment | ||
| elif (lineno := getattr(node, 'lineno', None)) is not None: | ||
| comment = self._type_ignores.get(lineno) | ||
|
|
There was a problem hiding this comment.
Let's back up a little.
Assuming node.lineno exists, the old code behaved like this:
- if there is a type ignore for the current line, use it
- otherwise, use whatever is in
node.type_comment
With your new code, node.type_comment is always used if it exists, reversing the priority.
There was a problem hiding this comment.
Yes, and this was intentional. If there is a type_comment attached, it should take the priority since type_ignores is a property of the module but on the other hand type_comment directly belongs to the node itself, so a change in it probably reflect / overwrite the specified type_ignores. Not sure if this deserves another PR with only this change (with still forcing for lineno), but I feel like closer to the node == better reflectiveness should hold up in this case.
|
When you're done making the requested changes, leave the comment: |
https://bugs.python.org/issue44896