Skip to content
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

Analyze super() expressions more precisely #7232

Merged
merged 1 commit into from Jul 25, 2019

Conversation

@tavianator
Copy link
Contributor

commented Jul 17, 2019

The previous implementation assumed that all super(T, x) expressions
were for instances of the current class. This new implementation uses
the precise types of T and x to do a more accurate analysis.

Fixes #5794

Copy link
Collaborator

left a comment

The general approach seems plausible, though I'm not sure about one thing. It may be a matter of getting the remaining tests passing.

test-data/unit/check-super.test Show resolved Hide resolved
mypy/checkexpr.py Outdated Show resolved Hide resolved
@tavianator tavianator force-pushed the tavianator:fix-super-in-class-methods branch from 1e6bf99 to 3985542 Jul 19, 2019
@tavianator tavianator changed the title Fix super() use in non-instance methods Analyze super() expressions more precisely Jul 19, 2019
@tavianator tavianator marked this pull request as ready for review Jul 19, 2019
@tavianator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

I've updated the PR to do much more complete typing of super() expressions.

mypy/checkexpr.py Outdated Show resolved Hide resolved
@tavianator tavianator force-pushed the tavianator:fix-super-in-class-methods branch from 3985542 to 0442838 Jul 22, 2019
Copy link
Collaborator

left a comment

Thanks for the updates! Based on a quick pass, this looks pretty solid.

It would be nice if you could split the long method into at least a few smaller ones, however, before a full review. Generally smaller methods that do one thing make things easier to understand and modify, in my experience. This is up to you though. One idea is to extract some (smaller) parts into helper methods. Another idea would be to split it into 2 or more big methods that are invoked in sequence, similar to the original implementation. What do you think?

@tavianator tavianator force-pushed the tavianator:fix-super-in-class-methods branch from 0442838 to 4ce60ca Jul 23, 2019
@tavianator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@JukkaL Good call, I split the implementation up a bit so it should be easier to follow.

@tavianator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

The CI failure is a socket timeout in mypy/test/testipc.py which I assume is unrelated to this

Copy link
Collaborator

left a comment

Thanks for the updates! The refactoring made this much nicer to read. This looks great now and is almost ready -- I just have a few minor comments.

if e.info and e.info.fallback_to_any and base == mro[-1]:
# There's an undefined base class, and we're at the end of the
# chain. That's not an error.
return AnyType(TypeOfAny.special_form)

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 24, 2019

Collaborator

I think that we could use TypeOfAny.from_another_any as the kind here, since this due to fallback_to_any.

This comment has been minimized.

Copy link
@tavianator

tavianator Jul 24, 2019

Author Contributor

Probably yeah, this is just what the old code did. Is source_any=mro[-1] fine in this case?

This comment has been minimized.

Copy link
@tavianator

tavianator Jul 24, 2019

Author Contributor

Looks like not, it's just <TypeInfo builtins.object> in that case. I guess that's why it's a special_form.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 25, 2019

Collaborator

Let's keep as it is since that's how it was previously.

mypy/checkexpr.py Show resolved Hide resolved
return AnyType(TypeOfAny.from_error)

# Use the type of the self argument, in case it was annotated
method = self.chk.scope.top_function()

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 24, 2019

Collaborator

What if this was a nested function within a method?

This comment has been minimized.

Copy link
@tavianator

tavianator Jul 24, 2019

Author Contributor

Also copied from the old code, but I wouldn't be surprised if this is wrong in that case. I guess we need to walk the scopes to find the current method?

This comment has been minimized.

Copy link
@tavianator

tavianator Jul 24, 2019

Author Contributor

Oh actually I think this code is right. The Python interpreter uses the innermost function even if it's not a method. Something like

class A:
    def f(self): pass

class B(A):
    def f(self):
        def g():
            super().f()
        g()

B().f()

gives

RuntimeError: super(): no arguments

testSuperInMethodWithNoArguments already checks for this.


return type_type, instance_type

def _type_info(self, typ: Type) -> Optional[TypeInfo]:

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 24, 2019

Collaborator

Style nit: since this doesn't use self in the body, this could be changed to a module-level function. If you do this, the name would have to be changed, since we don't generally use _ prefix at module level. Suggestion: type_info_from_type.

The previous implementation assumed that all super(T, x) expressions
were for instances of the current class.  This new implementation uses
the precise types of T and x to do a more accurate analysis.

Fixes #5794
@tavianator tavianator force-pushed the tavianator:fix-super-in-class-methods branch from 4ce60ca to 960c38b Jul 24, 2019
@JukkaL
JukkaL approved these changes Jul 25, 2019
Copy link
Collaborator

left a comment

Great! Looks good now!

Note that our recommended workflow is to push additional commits to a branch instead of force pushing an amended commit to make it easier to review incremental changes to a PR.

if e.info and e.info.fallback_to_any and base == mro[-1]:
# There's an undefined base class, and we're at the end of the
# chain. That's not an error.
return AnyType(TypeOfAny.special_form)

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 25, 2019

Collaborator

Let's keep as it is since that's how it was previously.

@JukkaL JukkaL merged commit 56ed5c3 into python:master Jul 25, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tavianator

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Ah okay, I'll keep that in mind for next time. Thanks for merging!

@tavianator tavianator deleted the tavianator:fix-super-in-class-methods branch Jul 25, 2019
@msullivan

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

@tavianator This rejects calls where the instance is of type Any, which seems wrong. Could you produce a follow-up that fixes that? Thanks!

@tavianator

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@msullivan Do you have an example? There is e.g. the test case

[case testSuperWithAny]
class B:
    def f(self) -> None: pass
class C(B):
    def h(self, x) -> None:
        reveal_type(super(x, x).f) # N: Revealed type is 'def ()'
        reveal_type(super(C, x).f) # N: Revealed type is 'def ()'
        reveal_type(super(C, type(x)).f) # N: Revealed type is 'def (self: __main__.B)'

which passes.

@msullivan

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

I'm going to withdraw my report. The basic case seems to work fine. I tripped over a small number cases in internal code but they had a lot of additional complications and to be honest are all pretty heinous, so I'm not going to worry about it.

kflynn added a commit to datawire/ambassador that referenced this pull request Sep 26, 2019
(0.730 includes python/mypy#7232)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.