[DO NOT MERGE] Fix def line numbers for decorated nodes

[emmatyping]

Previously, due to requirements of inspect.getsource and getting the bytecode first line number correct, CPython had to patch the line number of decorated AST nodes. This changes that so for:

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

        pass

The line number for the foo FunctionDef is 5, not 1. Based on my work in python/cpython#6460

Fixes #50 and fixes python/mypy#3871

[ddfisher]

Thanks for the PR, @ethanhs! My preference would be to wait on merging this until the PR to cpython is merged, or maybe even until we pick it up naturally by tracking cpython ast (depending on how far off that will be). If we want to merge this before we get it naturally by tracking ast, we also need to update the development philosophy section of the readme (to mention that sometimes we backport things from upstream ast) and the module docstring of the ast3 module to document the specific change.

That said, if this is a change that mypy is anxious to have, we could potentially bend the rules a bit here. :)

[emmatyping]

Thanks for the PR, @ethanhs! My preference would be to wait on merging this until the PR to cpython is merged, or maybe even until we pick it up naturally by tracking cpython ast

Okay, that is fine. I thought I'd offer this while I was working on the cpython PR.

[emmatyping]

@ilevkivskyi I forgot I had opened this, so here you go :)

I modified it to be the same as Serhiy's changes, and added the modifications David requested.

[emmatyping]

Also, based on my tests this will cause quite a few tests to fail in mypy (since type ignores now have to go on the correct line). So this will require quite a few changes in typeshed and mypy's test suite.

[gvanrossum]

So this will require quite a few changes in typeshed and mypy's test

suite. Oh, that's too bad. I guess you could start with putting type-ignores on both lines? What about other type checkers? Would Pyre's tests also fail when they take on the new typeshed? Or pytype?

[emmatyping]

@gvanrossum in python/mypy#3871 Rebecca said that pytype doesn't look at type ignore in stub files, so they won't care about the change. I'm not sure about pyre. I will open a typeshed issue about this.

[ilevkivskyi]

Apart from two docs comments, this looks good to me!

[emmatyping]

Hm, with this patch it seems that typed_ast doesn't handle lines 73-77 correctly: https://github.com/python/typeshed/blob/master/stdlib/3/unittest/__init__.pyi#L73

It appears to complain that the function line number is 73. This is particularly confusing since the type ignore should be in the correct place to as of right now (why didn't cause problems before?). I will have to think a bit about what could cause this.

[ilevkivskyi]

Hm, this is a bit mysterious, what is special about this particular function as compared to other?

Can you just try "stock" Python parser on this? My guess is that mypy does some special for overloads to compensate for previous bug.

[emmatyping]

@ilevkivskyi the line number reported by typed_ast is 73, not just mypy (I was using https://github.com/asottile/astpretty, which will use typed_ast). I tested with a build of 3.8 and it correctly reported it as 74. Perhaps my environment is messed up? I will have to test this more.

[gvanrossum]

Perhaps look at L440 in fastparse.py:

func_def.set_line(lineno + len(n.decorator_list))

[emmatyping]

@gvanrossum ah, that explains why it worked before, thanks! Now I just need to make sure this fix actually is working.

[emmatyping]

Okay it appears my environment was messed up (for some reason something is holding a lock on the pip temp folder causing the uninstall to not complete correctly...).

Anyway, I will get started on fixes for mypy and typeshed.

[ddfisher]

I'm feeling a bit conflicted about the backporting of this change to the 2.7 ast -- it looks to me like a pretty major change in development philosophy for a small tweak. Is this something that's feasible to add a flag to parse instead?

Also, we should make sure to do at least a minor version bump (and not just bugfix) when releasing this. Potentially even a major version bump, as this is likely to be a significant change for the folks that depend on us (like astroid).

[ddfisher]

Actually, if I'm understanding this correctly, astroid doesn't version its dependencies. (Can someone double-check? My Python packaging knowledge is a bit rusty.) That means that releasing this change potentially breaks pylint? (Or at least folks' #pylint: disables?)

[emmatyping]

@ddfisher I can understand not wanting to backport to the 2.7 ast, but I feel like it would be weird/harder to deal with if we backported it only to 3.x. 2/3 straddling code would be very painful to parse (we would report two different lines depending on if you were using ast27 or ast3). Considering typeshed contains shared stubs, that is a real concern.

As for asteriod, that is quite unfortunate that they do not version their dependencies. On one hand, I'd hate to break pylint ignores, but I also don't think it would make sense to never make a backwards incompatible release of typed_ast because they don't version either. It appears there has only been one release that uses typed_ast so far, so perhaps it isn't too bad?

[ilevkivskyi]

I think pylint people will be actually happy because of this. It was quite counter-intuitive that function definition line is the line of first decorator. But we indeed should contact them, because they may have some fixes to compensate for this. cc @PCManticore

[msullivan]

(Sorry, I fatfingered)

[PCManticore]

This is probably going to fix some annoying line number discrepancies reported by pylint, so having this merged would be great. Can't say exactly if this is going to affect the # disable/enable pragma in a negative way until we have a release of typed_ast to verify against.

@ddfisher Thanks, I just pinned typed_ast on our side.

[msullivan]

@ethanhs What's the status here?

[emmatyping]

@msullivan I believe David raised concerns about this breaking things for other downstream users, but it sounds like they are happy to have it.

Also it will require a fair amount of source changes in typeshed and mypy's test suite, so we probably want to wait to release this until the next release so there is plenty of time to work on that.

[msullivan]

OK, so it will be for a version 1.3.0, not the version 1.2.0 that we'll be releasing for the Str changes.

[gvanrossum]

I think that when we release typed_ast 1.2.0, we should do a brief blog post explaining the situation with 1.1.0, 1.1.1 and 1.2.0, and also warn people that 1.3.0 will be coming (soon?) and will have this incompatibility regarding the line number -- it's not just mypy and typeshed that will have to change, likely by then pylint (astroid) and perhaps pyflakes will be using it too.

[gvanrossum]

Sorry to be waffling, but I think we should hold off on this until it's been merged upstream. The first opportunity is in 3.8. Hopefully it can build on python/cpython#11645.

[emmatyping]

@gvanrossum no problem. It was already merged in python/cpython#9731 (Serhiy forgot about my PR and made the changes on his own). But let me know when you think a good time to merge is and I will rebase this PR then.

[gvanrossum]

OK, so we'll get this for free once we base typed_ast on 3.8. That may not be too far into the future. AFAIK that PR was not backported to 3.7, so we should not merge it in 3.7 either.

[msullivan]

Would it be possible to backport the fix to 3.7 and then include it in a typed_ast version 3.7.3 (following my proposed version scheme in #81)? Assuming not many changes to the parser get backported it should be pretty easy to sync with upstream by directly porting the small number of patches over.

[gvanrossum]

This is not the kind of thing one should backport -- too many tools depend on it, and breaking those with a patch release is a no-no.

[emmatyping]

Yes, I think this can be closed, since we aren't going to have it in Python < 3.8.