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

Fix two crashes in duplicate decorated functions #5427

Merged
merged 2 commits into from Aug 8, 2018

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Aug 7, 2018

Fixes #5394
Fixes #5181

The problem is that duplicate decorated functions are parsed as OverloadedFuncDef even if the actual decorator used is not typing.overload. This PR fixes possible crashes related to this. I could imagine there may be some extra spurious errors in some scenarios (in addition to duplicate definition correctly reported by mypy), but at least it should not crash.

@ilevkivskyi ilevkivskyi requested a review from msullivan Aug 7, 2018

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 7, 2018

Collaborator

Oops pushed too soon. Will fix the failures in other tests now.

Collaborator

ilevkivskyi commented Aug 7, 2018

Oops pushed too soon. Will fix the failures in other tests now.

Ivan Levkivskyi
@msullivan

Looks reasonable!

@@ -427,17 +427,24 @@ class OverloadedFuncDef(FuncBase, SymbolNode, Statement):
"""
items = None # type: List[OverloadPart]
unanalyzed_items = None # type: List[OverloadPart]

This comment has been minimized.

@msullivan

msullivan Aug 8, 2018

Collaborator

I really don't love all these unanalyzed things that aststrip forces upon us. Oh well.

@msullivan

msullivan Aug 8, 2018

Collaborator

I really don't love all these unanalyzed things that aststrip forces upon us. Oh well.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 8, 2018

Collaborator

I tried few other solutions, but after few attempts to undo the logic of deleting overloads in second pass, I just went with this, although I don't like this either.

@ilevkivskyi

ilevkivskyi Aug 8, 2018

Collaborator

I tried few other solutions, but after few attempts to undo the logic of deleting overloads in second pass, I just went with this, although I don't like this either.

@msullivan msullivan merged commit f637215 into python:master Aug 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:fix-overload-crash branch Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment