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

Push correct enclosing class while deferring a method #4073

Merged
merged 5 commits into from Oct 12, 2017

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Oct 8, 2017

Fixes #4069

This pushes the correct enclosing class while deferring a method instead of just the top class.

A related comment: the name active_class is a bit misleading, it returns None from within methods, I think it would be more consistent if active_class behave the same way as self.type in semantic analysis, but it is too late to change this, it is already used in several other places with this semantics.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Oct 8, 2017

Contributor

I think I am to blame for the name active_class, and I agree it's not the best. But the intention is that active is active scope, and inside a method there is no active class scope. I don't think it's the same as "the current type".

Can't the issue be resolved by freezing the entire scope, as suggested by the TODO?

Anyway, if you are leaving the method active_class(), and assigning enclosing_scope() to a variable, I think it can't be called active_class; it will be confusing.

Contributor

elazarg commented Oct 8, 2017

I think I am to blame for the name active_class, and I agree it's not the best. But the intention is that active is active scope, and inside a method there is no active class scope. I don't think it's the same as "the current type".

Can't the issue be resolved by freezing the entire scope, as suggested by the TODO?

Anyway, if you are leaving the method active_class(), and assigning enclosing_scope() to a variable, I think it can't be called active_class; it will be confusing.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 8, 2017

Collaborator

Can't the issue be resolved by freezing the entire scope, as suggested by the TODO?

It is unnecessary, and will only make solution more complex.

Anyway, if you are leaving the method active_class(), and assigning enclosing_scope() to a variable, I think it can't be called active_class; it will be confusing.

OK, I renamed this variable, I keep however the named tuple attribute name, active_typeinfo, it makes sense, since the enclosing class for a method becomes the active class when the deferred node rechecking starts.

Collaborator

ilevkivskyi commented Oct 8, 2017

Can't the issue be resolved by freezing the entire scope, as suggested by the TODO?

It is unnecessary, and will only make solution more complex.

Anyway, if you are leaving the method active_class(), and assigning enclosing_scope() to a variable, I think it can't be called active_class; it will be confusing.

OK, I renamed this variable, I keep however the named tuple attribute name, active_typeinfo, it makes sense, since the enclosing class for a method becomes the active class when the deferred node rechecking starts.

CR

@ilevkivskyi ilevkivskyi referenced this pull request Oct 9, 2017

Closed

Release 0.540 planning #4084

@JukkaL JukkaL self-assigned this Oct 11, 2017

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 11, 2017

Collaborator

Can you fix the merge conflict?

Collaborator

JukkaL commented Oct 11, 2017

Can you fix the merge conflict?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 11, 2017

Collaborator

@JukkaL

Can you fix the merge conflict?

Fixed.

Collaborator

ilevkivskyi commented Oct 11, 2017

@JukkaL

Can you fix the merge conflict?

Fixed.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 11, 2017

Collaborator

This generates unexpected errors when run against an internal Dropbox codebase. Here is a simplified example:

class A:
    def f(self) -> None:
        def g(x: int) -> int:  # Self argument missing for a non-static method 
                               # (or an invalid type for self)
            return y

y = int()
Collaborator

JukkaL commented Oct 11, 2017

This generates unexpected errors when run against an internal Dropbox codebase. Here is a simplified example:

class A:
    def f(self) -> None:
        def g(x: int) -> int:  # Self argument missing for a non-static method 
                               # (or an invalid type for self)
            return y

y = int()
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 11, 2017

Collaborator

This generates unexpected errors when run against an internal Dropbox codebase. Here is a simplified example: ...

Indeed, since the active_typeinfo is only used by bind_self and friends, we should only push the immediate enclosing class. Will push a fix in a second.

Collaborator

ilevkivskyi commented Oct 11, 2017

This generates unexpected errors when run against an internal Dropbox codebase. Here is a simplified example: ...

Indeed, since the active_typeinfo is only used by bind_self and friends, we should only push the immediate enclosing class. Will push a fix in a second.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 11, 2017

Collaborator

@JukkaL Fixed.

Collaborator

ilevkivskyi commented Oct 11, 2017

@JukkaL Fixed.

Ivan Levkivskyi
@JukkaL

JukkaL approved these changes Oct 12, 2017

LGTM, thanks for the PR!

@JukkaL JukkaL merged commit ed5a057 into python:master Oct 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment