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

Prohibit referring to class within its definition #3665

Conversation

Michael0x2a
Copy link
Collaborator

This partially, but not completely, addresses #3088

Previously, code of the following form was accepted without an error by mypy:

class A:
    def foo(self) -> A:
        ...

However, this results in an error at runtime because 'A' is not yet defined. This pull request modifies the parsing process to report an error when a type is being incorrectly used within its class definition.

However, this commit does not attempt to handle other cases where the user is using a type that is undefined at runtime. For example, mypy will continue to accept the following code without an error:

def f() -> A:
    ...

class A:
    ...

I decided it would probably be best to address this in a future pull request.

This partially, but not completely, resolves
python#3088

Previously, code of the following form was accepted by mypy:

    class A:
        def foo(self) -> A:
            ...

This results in an error at runtime because 'A' is not yet defined.
This pull request modifies the parsing process to report an error when
a type is being incorrectly used within its class definition.

However, this commit does *not* attempt to handle other cases where
the user is using a type that is undefined at runtime. For example, mypy
will continue to accept the following code without an error:

    def f() -> A:
        ...

    class A:
        ...

I decided it would probably be best to address this in a future pull
request.
@Michael0x2a
Copy link
Collaborator Author

As a note, I wasn't really sure where to put these tests, but decided to put them in test-data\unit\semanal-errors.test for now (the checks I added are really part of the parsing stage, not the semantic analysis stage). Should I move the tests to some other file?

@ethanhs
Copy link
Collaborator

ethanhs commented Jul 5, 2017

There is a test-data\unit\parse-errors.test. That seems to be the file for the parse tests.

@gvanrossum
Copy link
Member

gvanrossum commented Jul 5, 2017 via email

@Michael0x2a
Copy link
Collaborator Author

Yep, the checks I added should ignore anything in stubs (and types defined using comments).

(It turns out that if you try applying the new checks in stubs, you'll actually get catastrophic failures when you try running the tests because it'll basically break typeshed)

@Michael0x2a Michael0x2a mentioned this pull request Jul 6, 2017
@gvanrossum
Copy link
Member

I think this fell victim to the code freeze. There's always another release (and some people just run from master).

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 15, 2017

@Michael0x2a Are you interested in getting this merged? There are a bunch of merge conflicts now.

@Michael0x2a
Copy link
Collaborator Author

@JukkaL -- Sorry, I forgot about this PR (but I have much more free time now...)

In any case, I think I'm going to close this -- I think this change is unnecessary now with the acceptance of PEP 563. (Or more precisely, it'll become unnecessary in a few years, and I don't think it's worth burdening the mypy codebase with this change in the meantime.)

If you disagree and think it's still worth adding this feature, let me know and I can reopen this + fix the merge conflicts.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 28, 2018

This feature is still worth adding, since many projects are slow to drop support for older Python versions, and new users may take a while to learn about from __future__ import annotations.

The check should be disabled when using from __future__ import annotations, though. If the target Python version is 3.7 but the __future__ import is missing, we could generate a hint about it as part of the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants