Function line number information is not always correct

[ethanhs]

Currently, the line number of the function definition is not correct leading to issues such as #3869.

In the Python ast (as well as type-ast) the FunctionDef node we use to, for example, report missing return annotations gives a line number of the beginning of the first decorator (if any). Therefore it can incorrectly report the line number information we need to report errors.

Our currently logic increments the line number of any decorators, however, we cannot depend on this to be consistent. Consider this problematic (but completely valid!) example:

@decorate(
        "multi",
        "line"
        )
def foo(
        a: int,
        b: str,
        ):

        pass

The FunctionDef.lineno would be 1, so mypy will report test.py:2: error: Function is missing a return type annotation. However, the return should be on line 8! Note that blanks space between the body and end of the function definition node means we cannot decrement based on the first item in the body of the function.

As a first possible solution, we could modify typed_ast to give the line number of the returns if the function is not typed (currently it just puts the attribute as None), so that the information is known, but it clearly indicates that the node is empty. This is certainly not an ideal solution.

Corresponding issue opened in python/typed_ast#50

[gvanrossum]

IMO asking for the line number where the return annotation should go (i.e. where the '):' is) is overkill, but asking for the line number for the 'def' is reasonable.

[ethanhs]

Hm, that is probably true now that I think about it more. That would also have the added bonus of better error messages if a function name is redefined. I was thinking that since on larger functions it is common to split arguments over multiple lines, it would be nice to give an exact line number of where to put the annotation, but now that I think about it, if we point to the name, it shouldn't be too bad for the user to modify the return of that function.

Perhaps we could add a def_lineno attribute to FunctionDef nodes in typed_ast? Then refactor mypy to use that of course.

[gvanrossum]

That sounds reasonable.

[ilevkivskyi]

I think we should do the same for classes (i.e. add class_lineno), mypy currently provides special support to only one class decorator @runtime, but IIRC @JelleZijlstra already got confused once when I put a # type: ignore on the class decorator line.

[JelleZijlstra]

I think I would have been confused too if that ignore comment had not been on the decorator line. :)

[ethanhs]

I agree with @ilevkivskyi adding a class_lineno is also probably a good idea.

If nothing else, it will correctly report name re-definitions better.

If there are no objections, I will start on a patch to typed_ast.

I got busy with a PEP, so this is up for grabs

[ethanhs]

I should mention there is an open bpo issue about this exact problem. https://bugs.python.org/issue33211

[ethanhs]

So if we do this, it will require a change from:

@deco  # type: ignore
def f(a: int, b: str) -> None:
    ...

to

@deco
def f(a: int, b: str) -> None:  # type: ignore
    ...

everywhere, which is semantically more correct, but shows up a fair amount in typeshed (cc @JelleZijlstra, I'm happy to work on changing the occurrences of this).

That also means pytype will need to make a similar patch. @matthiaskramm, since Google already patches their ast, I hope it wouldn't hurt to patch it a bit more? The totality of changes to the ast are a handful of lines (see ethanhs/typed_ast@51b8d3e and note only the first file is the patch you'd need).

[gvanrossum]

For pytype, Matthias is no longer on that project, @rchen152 is his replacement.

[rchen152]

So if I understand correctly, the problem is that in typeshed, "# type: ignore" comments are going to move from the first line of the first decorator to the line on which "def" appears, so pytype will need to change its pyi parsing to handle this?

If that's it, then everything is fine! Pytype ignores "# type: ignore" in a pyi file.

[ethanhs]

For pytype, Matthias is no longer on that project, rchen152 is his replacement.

Ah, I didn't know that! Apologies.

So if I understand correctly, the problem is that in typeshed, "# type: ignore" comments are going to move from the first line of the first decorator to the line on which "def" appears, so pytype will need to change its pyi parsing to handle this?

Yes exactly.

If that's it, then everything is fine! Pytype ignores "# type: ignore" in a pyi file.

Excellent! I will work on the changes tomorrow then.

[gvanrossum]

Should we do this or not? I worry that just changing the lineno attribute of FunctionDef and ClassDef nodes will break tons of users' code since it changes where # type: ignore comments would have to go. So maybe let sleeping dogs lie?

OTOH in Python 3.8 the ast module moves the lineno attributes of these nodes, so in the long run we may have no choice (and to do it we would have to update typed_ast first, see python/typed_ast#50).

[brandtbucher]

I recently had a PR merged (#6648) that recognizes # type: ignores on any line of a multi-line expression in Python 3.8; it would probably be fairly straightforward to use the same approach here for classes and functions. It would allow the correct lines to be reported, while also respecting ignores occurring on any line in the scope (such as between the first decorator and the final colon def/class line).

If this seems like a viable solution, I'd be happy to write a PR.

[gvanrossum]

But that will only affect 3.8, right? Maybe that’s okay and we should just keep the existing behavior for older versions—and not modify typed-ast.

[brandtbucher]

Correct, 3.8 only. It would probably make any planned transition smoother too, since 3.8 users will have the option of the "correct" line (i.e. in new 3.8 code) or the "compatibility" line (i.e. in typeshed).

[gvanrossum]

Yeah, this sounds like a good thing to have.

[brandtbucher]

Awesome, I'll begin work on this.