Skip to content

Hide imported names in stubs unless 'as id' is used (PEP 484) #3706

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

Merged
merged 16 commits into from
Jul 31, 2017

Conversation

ilevkivskyi
Copy link
Member

Fixes #2927

PEP 484 specifies that only names imported as from mod import name as name should be re-exported. However, mypy didn't follow this rule so that this code passed without errors but obviously fails at runtime:

from collections import TypeVar
from weakref import Generic
from getopt import List

T = TypeVar('T')

class C(Generic[T], List[int]):
    ...

This PR makes all these errors. CI will fail since there are several dozens of errors in typeshed, I am going to make a PR to typeshed soon.

@gvanrossum
Copy link
Member

I'll test this against Dropbox codebases.

@ilevkivskyi
Copy link
Member Author

It looks like some of the typeshed failures are due to a bug in my PR, will investigate now.

@gvanrossum
Copy link
Member

Yeah, I get this for example:

mypy -2 -c 'import xml.sax'
typeshed/stdlib/2and3/xml/sax/__init__.pyi:27: error: Name 'xml.sax.xmlreader.XMLReader' is not defined
typeshed/stdlib/2and3/xml/sax/__init__.pyi:29: error: Name 'xml.sax.handler.ContentHandler' is not defined
typeshed/stdlib/2and3/xml/sax/__init__.pyi:30: error: Name 'xml.sax.handler.ErrorHandler' is not defined
typeshed/stdlib/2and3/xml/sax/__init__.pyi:32: error: Name 'xml.sax.handler.ContentHandler' is not defined
typeshed/stdlib/2and3/xml/sax/__init__.pyi:33: error: Name 'xml.sax.handler.ErrorHandler' is not defined
typeshed/stdlib/2and3/xml/sax/__init__.pyi:35: error: Name 'xml.sax.xmlreader.XMLReader' is not defined

@ilevkivskyi
Copy link
Member Author

After I fixed the bug in this PR, it looks like there are only two errors in typeshed:

error: Module 'json' has no attribute 'JSONDecodeError'
error: Module 'asyncio.futures' has no attribute 'Awaitable'

Both seem to be legitimate errors.

@ilevkivskyi
Copy link
Member Author

only two errors in typeshed

Actually two in stdlib plus a dozen or so in third party stubs.

@gvanrossum
Copy link
Member

So do we need to fix all the typeshed errors first? Also I can't re-test this with the Dropbox codebases until you've fixed the merge conflict.

@ilevkivskyi
Copy link
Member Author

@gvanrossum I fixed the merge conflict and made a PR to typeshed python/typeshed#1484

@gvanrossum
Copy link
Member

Please hold off on this until after the release of 0.521 (or until Jukka vets it).

@ilevkivskyi
Copy link
Member Author

Please hold off on this until after the release of 0.521 (or until Jukka vets it).

NP, I don't think this is urgent. But it would be great to test this with internal codebases (maybe there are more errors in stubs).

@ilevkivskyi
Copy link
Member Author

OK, I have fixed the logic as discussed in python/typeshed#1484, this should be now ready for review and testing with internal codebases.

@ilevkivskyi
Copy link
Member Author

I added one more test and simplified the PR, still I cannot do this with only one flag module_public since it is used for other things like __all__, import * and _private_names. It is possible to have a single flag with three values HIDDEN, PRIVATE, PUBLIC = 0, 1, 2 instead of two bool flags module_public and module_hidden but this will induce some code churn.

Also typeshed is synced, so that this ready to be reviewed and merged.

Copy link
Member

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

One style nit, otherwise looks good to me.

mypy/semanal.py Outdated
return n
if n and n.module_hidden:
self.name_not_defined(name, ctx)
return n if n and not n.module_hidden else None
Copy link
Member

Choose a reason for hiding this comment

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

Minor style nit. Take it if you want. I prefer to not have compound if statements in returns. It makes things clearer if you break it up, but up to you.

Copy link
Member

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

@ilevkivskyi
Copy link
Member Author

@gvanrossum Do you think Ethan's review is enough, or we need another one? (This is quite a minor change, so that I would merge this soon to avoid more merge conflicts.)

@gvanrossum
Copy link
Member

I'm taking a look now.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM. I'm pressed for time, can you merge it yourself?

@ilevkivskyi ilevkivskyi merged commit c87e413 into python:master Jul 31, 2017
@ilevkivskyi ilevkivskyi deleted the no-re-export-from-stubs branch July 31, 2017 20:05
gvanrossum pushed a commit to python/typeshed that referenced this pull request Aug 3, 2017
After mypy [started hiding](python/mypy#3706) imported names in stubs unless `from ... import ...` is used, we found an error with stubs of ast module. 

It looks like ast module should re-export everything in `_ast` and according to PEP 484, it can do that by doing `from _ast import *`, so this is what this PR does.
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.

3 participants