classification
Title: lineno and col_offset are wrong on function definitions with decorators
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6, Python 3.5, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Ethan Smith, eric.smith, gforcada, levkivskyi, nitishch, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2018-04-03 05:13 by gforcada, last changed 2018-04-25 00:13 by eric.smith.

Pull Requests
URL Status Linked Edit
PR 6410 closed Ethan Smith, 2018-04-07 12:18
PR 6460 open Ethan Smith, 2018-04-12 22:33
Messages (6)
msg314861 - (view) Author: Gil Forcada (gforcada) Date: 2018-04-03 05:13
Given the following code:

class MyClass(object):

    @property
    def my_function(self): pass

Parsing it with ast module, the lineno and col_offset of the ast.FunctionDef is reported to be where the decorator starts (i.e. line 3 column 4) rather than where the actual def statement is (i.e. line 4 column 4).

This is due to the decorator that is part of the ast.FunctionDef, but as  there can be multiple decorators (which they don't provide their own lineno and col_offset arguments) and they can span across multiple lines, there is no reliable way to actually know where the actual def statement starts physically.

See this bug report on flake8-builtins plugin (I'm the author) reported by @dhood user on github:

https://github.com/gforcada/flake8-builtins/issues/22#issuecomment-377961642
msg314864 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-04-03 07:50
See how similar issue was solved in Pyflakes: https://github.com/PyCQA/pyflakes/pull/273.

Python tests also needed special workaround, see Lib/test/test_sys_settrace.py and issue17288.
msg314906 - (view) Author: Ethan Smith (Ethan Smith) * Date: 2018-04-03 22:35
There is also a relevant mypy bug report https://github.com/python/mypy/issues/3871. This seems like a common problem for tools working on the AST. The relevant code seems to be https://github.com/python/cpython/blob/master/Python/ast.c#L1695. 

Would a possible solution be adding a decorated_lineno attribute to decorated ast nodes?
msg315008 - (view) Author: Ethan Smith (Ethan Smith) * Date: 2018-04-05 23:41
I have a branch with an implementation of my suggestion here: https://github.com/ethanhs/cpython/tree/decorlineno

I was hoping to see if this was seen as a reasonable patch that might be accepted.


Also, while I think it would be nice, I take it a patch for this would be unlikely to be backported, right?
msg315046 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-04-07 00:18
> I was hoping to see if this was seen as a reasonable patch that might be accepted.

I didn't look carefully but superficially it looks reasonable, so it is worth trying.

> Also, while I think it would be nice, I take it a patch for this would be unlikely to be backported, right?

I think this is unlikely because it affects some public APIs.
msg315074 - (view) Author: Ethan Smith (Ethan Smith) * Date: 2018-04-07 21:29
In my PR, I added `def_lineno` and `class_lineno` as fields in the ASDL, instead of attributes (since constructors cannot have attributes, only types can). This means they show up in `ast.dump` which is probably not the desired behavior, as it makes the dumped ast whitespace/line offset sensitive.

Therefore I propose we change the line number of the nodes to be the one of the def/class statement. It seems based on [this commit](https://github.com/python/cpython/commit/09aaa88328a5083469b2682230c7f3c62942afab) that the change was done to fix inspect.getsource (so that it started on the first decorator), but I think it is much more logical for inspect to handle decorated items instead of having the ast lie.

One other option could be for a modified ast to have a decorated node, which holds the decorator list, and the class/function. This has the possible downside of being a not-insignificant change to the ast.
History
Date User Action Args
2018-04-25 00:13:19eric.smithsetnosy: + eric.smith
2018-04-12 22:33:05Ethan Smithsetpull_requests: + pull_request6154
2018-04-07 21:29:28Ethan Smithsetmessages: + msg315074
2018-04-07 12:18:10Ethan Smithsetkeywords: + patch
stage: patch review
pull_requests: + pull_request6115
2018-04-07 00:18:20levkivskyisetmessages: + msg315046
2018-04-05 23:41:50Ethan Smithsetmessages: + msg315008
2018-04-05 09:07:50levkivskyisetnosy: + levkivskyi
2018-04-05 07:22:57nitishchsetnosy: + nitishch
2018-04-03 22:35:18Ethan Smithsetnosy: + Ethan Smith
messages: + msg314906
2018-04-03 07:50:28serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg314864
2018-04-03 05:13:29gforcadacreate