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

Type check arguments for "super" #3919

Merged
merged 9 commits into from Sep 6, 2017

Conversation

Projects
None yet
3 participants
@JukkaL
Collaborator

JukkaL commented Sep 5, 2017

This is not quite as strict as we could be, but this is still better
than what we had before. A full implementation will be somewhat
complicated.

Remaining issues:

  • Assume that the first argument refers to the enclosing class, even
    though that's not generally the case.

  • Assume that a type object as the second argument is always fine.

  • Don't validate some kinds of type object types as the first argument.

  • Single-argument variant is not supported.

Fixes #526 (modulo the open issues).

Also, I no longer think that defining messages as constants in
mypy.messages is generally a good idea so I'm not following
that pattern here. I'll officially update the convention in a
separate PR.

Type check arguments for "super"
This is not quite as strict as we could be, but this is still better
than what we had before. A full implementation will be somewhat
complicated.

Remaining issues:

* Assume that the first argument refers to the enclosing class, even
  though that's not generally the case.

* Assume that a type object as the second argument is always fine.

* Don't validate some kinds of type object types as the first argument.

* Single-argument variant is not supported.

Fixes #526 (modulo the open issues).

Also, I no longer think that defining messages as constants in
`mypy.messages` is generally a good idea so I'm not following
that pattern here. I'll officially update the convention in a
separate PR.
@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 5, 2017

Collaborator

This is ready for review but this is not ready to be merged yet, as this requires some changes to internal Dropbox code.

I'll create follow-up issues for any remaining open issues after this has been merged.

Collaborator

JukkaL commented Sep 5, 2017

This is ready for review but this is not ready to be merged yet, as this requires some changes to internal Dropbox code.

I'll create follow-up issues for any remaining open issues after this has been merged.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Sep 5, 2017

Collaborator

Appveyor failure is a flake (see #3895)

Collaborator

ethanhs commented Sep 5, 2017

Appveyor failure is a flake (see #3895)

@ilevkivskyi

Thanks! This is now the oldest high priority issue, I am glad it will be fixed now.

elif len(e.call.args) > 2:
self.chk.fail('Too many arguments for "super"', e)
elif self.chk.options.python_version[0] == 2 and len(e.call.args) == 0:
self.chk.fail('Too few arguments for "super"', e)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would move all the logic until this point to semanal.py, but it is optional.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would move all the logic until this point to semanal.py, but it is optional.

This comment has been minimized.

@JukkaL

JukkaL Sep 6, 2017

Collaborator

I'm keeping the logic here because we want to execute the final elif block only if all the previous checks are false, and having all the checks here makes it explicit. Also, we check the argument counts of other calls during type checking (though super() is special so we could do it during semantic analysis as well).

@JukkaL

JukkaL Sep 6, 2017

Collaborator

I'm keeping the logic here because we want to execute the final elif block only if all the previous checks are false, and having all the checks here makes it explicit. Also, we check the argument counts of other calls during type checking (though super() is special so we could do it during semantic analysis as well).

Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated mypy/treetransform.py
Show outdated Hide outdated test-data/unit/check-super.test
Show outdated Hide outdated test-data/unit/check-super.test
def f(self) -> None: pass
class C(B):
def f(self) -> None: pass

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would make return types different here and in superclass (e.g. float and int), so that we could be sure in reveal_type that the correct one is picked up.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would make return types different here and in superclass (e.g. float and int), so that we could be sure in reveal_type that the correct one is picked up.

This comment has been minimized.

@JukkaL

JukkaL Sep 6, 2017

Collaborator

Updated.

@JukkaL

JukkaL Sep 6, 2017

Collaborator

Updated.

@ilevkivskyi

Just one more question. You can merge this when the internal changes you mentioned are ready.

Show outdated Hide outdated mypy/checkexpr.py

@JukkaL JukkaL merged commit 2247870 into master Sep 6, 2017

3 checks passed

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

@JukkaL JukkaL deleted the type-check-super branch Sep 6, 2017

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