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

[stubtest] Error related to __all__ must have it in the error path #14217

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Nov 29, 2022

Right now stubtest gives this error for __all__:

error: passlib.handlers.oracle names exported from the stub do not correspond to the names exported at runtime. This is probably due to an inaccurate `__all__` in the stub or things being missing from the stub.
Stub: in file /Users/sobolev/Desktop/typeshed/stubs/passlib/passlib/handlers/oracle.pyi
Names exported in the stub but not at runtime: ['oracle10', 'oracle11']
Runtime:
Names exported at runtime but not in the stub: ['oracle10g', 'oracle11g']

(Context: there's a real error in __all__: https://foss.heptapod.net/python-libs/passlib/-/blob/branch/stable/passlib/handlers/oracle.py#L18)

The only way to ignore it is to ignore the whole module. This is not right.
We need to be able to ignore just some_module.__all__.

@sobolevn
Copy link
Member Author

Running stubtest with my change on the same problem:

error: passlib.handlers.oracle.__all__ names exported from the stub do not correspond to the names exported at runtime. This is probably due to an inaccurate `__all__` in the stub or things being missing from the stub.
Stub: in file /Users/sobolev/Desktop/typeshed/stubs/passlib/passlib/handlers/oracle.pyi
Names exported in the stub but not at runtime: []
Runtime:
Names exported at runtime but not in the stub: ['oracle10g', 'oracle11g']

Looks like it is fixed!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check verifies that the publicly exported names are the same in the stub as at runtime. That's not exactly the same as verifying that __all__ is the same in the stub as at runtime (though having an inaccurate __all__ is the most likely cause of the error).

E.g. I believe you'll also get this error emitted if __all__ in the stub is accurate, but something is missing from the stub (despite being included in __all__ in the stub). In that situation, it might not be so appropriate to have __all__ in the error path.

@sobolevn
Copy link
Member Author

I believe you'll also get this error emitted if all in the stub is accurate, but something is missing from the stub (despite being included in all in the stub). In that situation, it might not be so appropriate to have all in the error path.

I've recreated it:

error: passlib.handlers.mysql.__all__ names exported from the stub do not correspond to the names exported at runtime. This is probably due to an inaccurate `__all__` in the stub or things being missing from the stub.
Stub: in file /Users/sobolev/Desktop/typeshed/stubs/passlib/passlib/handlers/mysql.pyi
Names exported in the stub but not at runtime: []
Runtime:
Names exported at runtime but not in the stub: ['mysq41']

It looks fine to me, much better than ignoring the whole module.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 29, 2022

To be clear, if you have an allowlist entry of passlib.handlers.mysql, that won't ignore all errors in the module — you'd need the allowlist entry to be passlib.handlers.mysql.* if you wanted to do that. It will only ignore:

  • Errors relating to __all__/discrepancies in publicly exported names
  • Errors occurring when the module can't be imported at runtime
  • Errors occurring when the module exists at runtime but is missing from the stub.

Having said that, I agree that the current situation isn't ideal, and you're right that it doesn't look as confusing as I worried it might do if we add __all__ to the error path! This is basically my explanation of why I didn't do it like this initially :)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems reasonable.

(I guess the other thing we could do is emit a single error for each name, but that could result in similar allowlisting issues for specific defs)

mypy/stubtest.py Outdated Show resolved Hide resolved
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants