FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Update ModuleType.__file__ to be Optional by ozanani · Pull Request #6186 · python/typeshed · GitHub

  1. python
  2. typeshed

Repository navigation

Update ModuleType.__file__ to be Optional - #6186

Merged
JelleZijlstra merged 1 commit into
python:masterfrom
ozanani:patch-1
Nov 7, 2021
ConversationCommits (1)ChecksFiles changed
Merged

Update ModuleType.__file__ to be Optional#6186
JelleZijlstra merged 1 commit into
python:masterfrom
ozanani:patch-1

Conversation

ozanani commented Oct 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Per the Python documentation, ModuleType.__file__ is Optional:

file is optional. If set, this attribute’s value must be a string. The import system may opt to leave file unset if it has no semantic meaning (e.g. a module loaded from a database).

Source: https://docs.python.org/3/reference/import.html#file__

Update ModuleType.__file__ to be Optional
d6cc323 
Per the Python documentation, `ModuleType.__file__` is `Optional`: https://docs.python.org/3/reference/import.html#file__

JelleZijlstra commented Oct 22, 2021

Copy link
Copy Markdown
Member

It sounds like they mean it is either a str or unset, not that it's a str or None. It's optional, not typing.Optional.

Unfortunately the type system has no way to express that an attribute may not exist.

github-actions Bot commented Oct 22, 2021

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pip (https://github.com/pypa/pip.git)
+ src/pip/_internal/commands/debug.py:70: error: Value of type variable "AnyStr" of "dirname" cannot be "Optional[str]"
+ src/pip/_internal/commands/debug.py:70: error: List item 0 has incompatible type "Optional[str]"; expected "str"

ibis (https://github.com/ibis-project/ibis.git)
+ ibis/backends/tests/base.py:82: error: Argument 1 to "Path" has incompatible type "Union[str, None, Any]"; expected "Union[str, PathLike[str]]"

pytest (https://github.com/pytest-dev/pytest.git)
+ src/_pytest/pathlib.py:542: error: Item "None" of "Optional[str]" has no attribute "endswith"  [union-attr]
+ src/_pytest/pathlib.py:543: error: Value of type "Optional[str]" is not indexable  [index]
+ src/_pytest/pathlib.py:544: error: Item "None" of "Optional[str]" has no attribute "endswith"  [union-attr]
+ src/_pytest/pathlib.py:545: error: Value of type "Optional[str]" is not indexable  [index]
+ src/_pytest/pathlib.py:548: error: Argument 2 to "_is_same" has incompatible type "Optional[str]"; expected "str"  [arg-type]
+ src/_pytest/config/__init__.py:1469: error: Argument 1 to "Path" has incompatible type "Optional[str]"; expected "Union[str, PathLike[str]]"  [arg-type]
+ src/_pytest/python.py:330: error: Item "None" of "Optional[str]" has no attribute "endswith"  [union-attr]
+ src/_pytest/python.py:331: error: Value of type "Optional[str]" is not indexable  [index]
+ src/_pytest/python.py:332: error: Incompatible types in assignment (expression has type "Optional[str]", variable has type "Union[PathLike[str], str]")  [assignment]
- src/_pytest/python.py:946: error: Unexpected keyword argument "arg2scope" for "CallSpec2"  [call-arg]
- src/_pytest/python.py:946: error: Unexpected keyword argument "indices" for "CallSpec2"  [call-arg]
- src/_pytest/python.py:946: error: Unexpected keyword argument "idlist" for "CallSpec2"  [call-arg]
- src/_pytest/python.py:946: error: Unexpected keyword argument "marks" for "CallSpec2"  [call-arg]
- src/_pytest/logging.py:634: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

rich (https://github.com/willmcgugan/rich.git)
+ rich/traceback.py:249: error: Value of type variable "AnyStr" of "dirname" cannot be "Optional[str]"
+ rich/traceback.py:252: error: Value of type variable "AnyStr" of "normpath" cannot be "Optional[str]"
+ rich/traceback.py:252: error: Value of type variable "AnyStr" of "abspath" cannot be "Optional[str]"
+ rich/traceback.py:253: error: Argument 1 to "append" of "list" has incompatible type "Optional[str]"; expected "str"

srittau added the reason: inexpressible Closed, because this can't be expressed within the current type system label Oct 22, 2021

ozanani commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra I agree with your interpretation of the docs. However, I encountered a case in which __file__ was accessible and set to None which I'm trying to repro.
Also, the Python2 version of this file marks __file__ as Optional:
https://github.com/python/typeshed/blob/master/stdlib/@python2/types.pyi#L126

Which looks like an undesirable inconsistency between the two.

Akuli commented Oct 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

However, I encountered a case in which _file_ was accessible and set to None which I'm trying to repro.

I would appreciate this. For imports not coming from files, I tried sys (built-in module, __file__ attribute doesn't exist) and gi.repository.Gtk (installed on most Linux systems, __file__ set to a dummy string '<gi.repository.Gtk>' as the importer follows PEP 302).

Also, the Python2 version of this file marks __file__ as Optional

This was introduced in c0aea8e which was before typeshed used pull requests, and the commit message doesn't say anything about this, so we can't really know whether this was intentional.

srittau removed the reason: inexpressible Closed, because this can't be expressed within the current type system label Oct 22, 2021

ozanani commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

Ok, apparently the repro is easy (when not using IPython, behaviour is different there).
When querying package.__file__, None is returned.

Repro by creating:

    package (directory)
    runner.py

Code for runner.py:

import package

print("package.__file__:", package.__file__)

And run:

$ python3 runner.py
package.__file__: None

Akuli commented Oct 22, 2021

Copy link
Copy Markdown
Collaborator

The repro applies only to namespace packages:

akuli@akuli-desktop:/tmp$ mkdir package
akuli@akuli-desktop:/tmp$ python3 -c 'import package; print(package.__file__)'
None
akuli@akuli-desktop:/tmp$ touch package/__init__.py
akuli@akuli-desktop:/tmp$ python3 -c 'import package; print(package.__file__)'
/tmp/package/__init__.py
akuli@akuli-desktop:/tmp$

On the one hand, namespace packages are quite rare in practice, so not worrying about them would be in the spirit of avoiding false positives. On the other hand, this seems like a corner case you would want to handle correctly, even if it's rare.

hauntsaninja commented Oct 22, 2021

Copy link
Copy Markdown
Collaborator

Trying to trace the mypy primer examples, I think this results in false positives (e.g. for rich, don't think you can get a traceback into a namespace, for pytest, it seems to check existence of __init__.py)
If you also consider the aspect that __file__ might not even exist, so we're not even warning about all the rare corner cases, I think I'd stick with the happy __file__: str

ozanani commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

So __file__ could be one of three things:

  1. str
  2. None (namespace packages)
  3. Not set

I think that keeping it as __file__: str is fragile (and have already seen code breaks because of it). Solid str will make the reader rely on this without worrying too much. Setting it as Optional might make the reader google it and learn about the different cases, which could be beneficial.

ewerybody mentioned this pull request Oct 25, 2021
Merged

ozanani commented Oct 29, 2021

Copy link
Copy Markdown
Contributor Author

@Akuli , wdyt?

Akuli commented Oct 29, 2021

Copy link
Copy Markdown
Collaborator

I think this is a good idea. Other maintainers can merge if they agree.

ozanani commented Nov 6, 2021

Copy link
Copy Markdown
Contributor Author

cc @srittau, @JelleZijlstra

srittau commented Nov 7, 2021

Copy link
Copy Markdown
Collaborator

I'm fine with this change.

JelleZijlstra merged commit 4601581 into python:master Nov 7, 2021
hauntsaninja mentioned this pull request Nov 24, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL