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

Overloading based on an argument which can be str or None doesn't seem to work #3805

Closed
OddBloke opened this issue Aug 7, 2017 · 4 comments

Comments

@OddBloke
Copy link
Contributor

OddBloke commented Aug 7, 2017

When digging in to an issue with the stubs for configparser in the stdlib (python/typeshed#1527), I discovered that RawConfigParser.items() has two very different modes of operation. When it is given a section name (as a string, parser.items('name')), it will (with a little processing that shouldn't affect the types) return an iterable of tuples containing that sections keys and values (both of which are strings). When it is not given a section name (parser.items()), it instead returns an iterable of tuples containing the names and contents of the sections of the parser itself (the names are strings, and the contents are SectionProxy instances).

I have attempted to annotate this using overloading (because I believe that it's more precise than a union in this case):

@overload
def items(self, section: str = ..., raw: bool = ..., vars: _section = ...) -> Iterable[Tuple[str, str]]: ...

@overload
def items(self, section: None = ..., raw: bool = ..., vars: _section = ...) -> Iterable[Tuple[str, SectionProxy]]: ...

but I hit the following errors running ./tests/mypy_test.py in typeshed:

running mypy --python-version 3.6 --strict-optional # with 624 files
stdlib/3/configparser.pyi:103: error: Signature of "items" incompatible with supertype "Mapping"
stdlib/3/configparser.pyi:104: error: Overloaded function signatures 1 and 2 overlap with incompatible return types

The first one is expected (and, in fact, is the reason that this annotation has a #type: ignore on it in typeshed currently).

The second one, however, I don't expect because (with --strict-optional) I don't think None and str should overlap.

@OddBloke
Copy link
Contributor Author

OddBloke commented Aug 7, 2017

This is in the same realm as #3295, but as there's been a fair bit of discussion there, I didn't want to add to it without being sure.

OddBloke pushed a commit to OddBloke/typeshed that referenced this issue Aug 7, 2017
TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.
@gvanrossum
Copy link
Member

I don't think it's related to #3295. The problem (with the example as written) is that both overloads can be called without arguments, so the error is correct. Note that the real function in fact does not work when called with None. Try using

    @overload
    def items(self, *, raw: bool = ...) -> AbstractSet[Tuple[str, _section]]: ...
    @overload
    def items(self, section: str, raw: bool = ...) -> Iterable[Tuple[str, str]]: ...

but add back the other args.

Also note that using AbstractSet makes the argument-less signature compatible with MutableMapping.items() so no # type: ignore is needed.

@OddBloke
Copy link
Contributor Author

OddBloke commented Aug 7, 2017

Well, that seems extremely obvious now you've pointed it all out. :-p Thanks for taking the time!

I've updated the typeshed PR (and I'll consider filing some issues/PRs about improving the output of these warnings in the morning; I'll flatter myself by thinking I was only a couple of hints away from grokking this myself ;-).

@OddBloke OddBloke closed this as completed Aug 7, 2017
@gvanrossum
Copy link
Member

Don't feel bad! I end up having to reinvent this regularly myself when it appears in a new context.

OddBloke pushed a commit to OddBloke/typeshed that referenced this issue Aug 9, 2017
TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.
OddBloke pushed a commit to OddBloke/typeshed that referenced this issue Aug 21, 2017
TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.
OddBloke pushed a commit to OddBloke/typeshed that referenced this issue Aug 21, 2017
TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.
ambv pushed a commit to python/typeshed that referenced this issue Aug 21, 2017
…1527)

* Make configparser.RawConfigParser.__getitem__ return a SectionProxy

This reflects the code in the cpython tree and makes the following
(valid) code type-check correctly:

```
from configparser import ConfigParser

config = ConfigParser()
config.read_dict({'section': {'key': 'false'}})
assert config['section'].getboolean('key') is False
```

* RawConfigParser.items() returns SectionProxys not mappings

Because .items() uses __getitem__ to produce second item in each tuple.

* section argument to RawConfigParser.items is Optional

* Add comment explaining the status of RawConfigParser.items

TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.

* Correctly implement RawConfigParser.items overloading

* RawConfigParser.items(str) returns a List not an Iterable
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

No branches or pull requests

3 participants
@OddBloke @gvanrossum and others