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

Return Union type for List.__add__ #3183

Closed
wants to merge 1 commit into from
Closed

Conversation

kaste
Copy link
Contributor

@kaste kaste commented Aug 13, 2019

In Python, lists of union types are just common. We want to support easy
construction of lists e.g.

`[1, 2] + ['3']` or
`['foo' if p else None, 'bar'] + ['zoo']`

We adjust __iadd__ accordingly. Although returning a union here too looks
strange, it has the effect that

l = [1,2]
l += ['3']  # ERR: Result type of + incompatible in assignment

correctly fails.

@bluetech
Copy link
Contributor

My opinion: while this indeed describes the dynamic behavior more precisely, such occurrences in real code are often bugs, and I therefore prefer the current behavior that rejects it.

For the __iadd__ case, I think the list should be annotated with the union type ahead of time.

The __add__ case is a bit harder to annotate. A safe upcast operation would have been nice (see python/typing#565), although List is invariant so the cast is not actually safe. You might be able to use Sequence instead. Also the following alternative seems to typecheck fine so you might be able to use [*a, *b] instead of a + b:

from typing import List, TypeVar, Union

S = TypeVar('S')
T = TypeVar('T')

def add_lists(a: List[S], b: List[T]) -> List[Union[S, T]]:
    return [*a, *b]                                        

`['foo' if p else None, 'bar']

This does work as you expect already - List[Union[str, None]]. However it seems it is unique to None. If you change None to have some other type like 5, then it takes the common base class, so List[object].

@ilevkivskyi
Copy link
Member

Please note last time this was attempted (see #2404) it caused severe regressions elsewhere and was ultimately reverted.

@kaste
Copy link
Contributor Author

kaste commented Aug 14, 2019

@ilevkivskyi Sorry for not seeing this. Following #2404 and its linked issues, there aren't any good, easy to grok examples of the regressions. I considered the following before the PR.

Given

class Animal: ...; class Cat(Animal): ...; class Dog(Animal): ...
def get_cats() -> List[Cat]: ...
def get_dogs() -> List[Dog]: ...

Situation in master

a = get_cats() + get_dogs() # ERRS surprisingly
c: List[Animal] = get_cats() + get_dogs() # ERRS
e: List[Animal] = get_cats() # ERRS invariant message
f: List[Animal] = [Cat(), Dog()] # OK
h: List[Union[Cat, Dog]] = get_cats() # ERRS also invariant message
j: List[Union[Cat, Dog]] = list(get_cats()) # OK
k: List[Union[Cat, Dog]] = list(get_cats()) + list(get_dogs()) # ERRS

After PR

a = get_cats() + get_dogs() # OK
c: List[Animal] = get_cats() + get_dogs()  # ERRS surprisingly
e: List[Animal] = get_cats() # ERRS invariant message
f: List[Animal] = [Cat(), Dog()] # OK
h: List[Union[Cat, Dog]] = get_cats() # ERRS also invariant message
j: List[Union[Cat, Dog]] = list(get_cats()) # OK
k: List[Union[Cat, Dog]] = list(get_cats()) + list(get_dogs()) # OK

# This always works
d: List[Animal]
d = list(get_cats())
d.extend(get_cats())

So still, after this PR, there are a lot of confusing statements, but I don't see where I flip from 'OK' in master to 'FAIL' in PR. 🤷‍♂

I also don't see/have a workaround for 'a', 'c' or 'k' in master to get '+' (concat) to actually work.

@bluetech Apple/bananas examples like [1] + ['2'] usually don't make sense. concat/+ though is a main constructor of lists. There is nothing really fishy when you consider [customers] + their_salesman() + some_managers() and then send these to someone.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Aug 15, 2019

@kaste

but I don't see where I flip from 'OK' in master to 'FAIL' in PR.

Have you tried

a: Sequence[object]
b1: List[Any]
b2: List[int]

a = b1 + b2

?

@kaste
Copy link
Contributor Author

kaste commented Aug 15, 2019

I actually have. And yes it flips. But then I could not make any sense of it, so I did not consider it. Probably nobody writes something like this, but mypy could infer such a situation. But then, it is not type-safe t.i. in master b1 + b2 != b2 + b1. So I thought it's an interesting bug, or the internals of mypy shine through the code... again 🤷‍♂

@kaste
Copy link
Contributor Author

kaste commented Sep 8, 2019

When we implement __radd__ as well we can trick mypy so that your example now passes. T.i. both

	a = b1 + b2
	a = b2 + b1

from your testcase now pass.

@srittau
Copy link
Collaborator

srittau commented Oct 8, 2019

@ilevkivskyi @JukkaL As the ones handling the fallout from #2404 do you think we should try this approach? Is there a good test case for making sure this doesn't cause regressions?

@JukkaL
Copy link
Contributor

JukkaL commented Oct 10, 2019

I don't think that we have good test cases for checking that this doesn't cause any regressions, other than type checking some big enough codebase with the change. I can try this against Dropbox internal codebases (not sure exactly when though).

@srittau
Copy link
Collaborator

srittau commented Oct 11, 2019

FWIW, I just ran this against a code-base with ~2000 files without error.

@JukkaL
Copy link
Contributor

JukkaL commented Oct 11, 2019

That's promising!

@lovasoa
Copy link
Contributor

lovasoa commented Dec 19, 2019

Has there been any progress on this ? I was hit by this bug today while testing a code that looked like :

for current, previous in zip(l, [None] + l): ...

I would then have expected current to be of type T and previous to be of type Optional[T]. Instead, I got Unsupported operand types for + ("List[None]" and "List[T]").

@kaste
Copy link
Contributor Author

kaste commented Dec 24, 2019

With the newer mypy this "hacky" approach is probably outdated because we can now annotate/give a hint about self itself.

@srittau
Copy link
Collaborator

srittau commented May 27, 2020

Closing per last comment by @kaste. Thank you for the effort!

@srittau srittau closed this May 27, 2020
@kaste
Copy link
Contributor Author

kaste commented May 27, 2020

Is there another PR? T.i. I couldn't get my idea to work actually. Should we just try harder?

@lovasoa
Copy link
Contributor

lovasoa commented May 27, 2020

Since the issue is still there, it should probably be tracked somewhere

@srittau
Copy link
Collaborator

srittau commented May 27, 2020

Sorry, I think I misunderstood. We still need to try this against other large codebases.

@srittau srittau reopened this May 27, 2020
@JelleZijlstra
Copy link
Member

@JukkaL would you mind trying this PR on your codebase?

@JelleZijlstra
Copy link
Member

Also stubtest is now complaining because list.__radd__ doesn't exist at runtime. We should probably whitelist it.

@JukkaL
Copy link
Contributor

JukkaL commented Jun 4, 2020

I've been a bit distracted but managed finally to check this against our big internal codebase.

It generated about 50 new errors, mostly of form Unsupported operand types for + ("List[str]" and "List[str]"). It wasn't immediately clear what was going on here, but it looks like an issue with mypy type inference.

@Akuli
Copy link
Collaborator

Akuli commented Aug 11, 2020

It would be nice if you could add List[pathlib.Path] and List[str] together for subprocess.call arguments, but List[str] + List[int] is probably a bug imo.

@JelleZijlstra
Copy link
Member

This has been open for a while (and now has a merge conflict). Do people think we should merge this? The errors @JukkaL found seem concerning.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 1, 2020

I fixed the merge conflict, although given what Jukka reports I wouldn't merge until the mypy kinks are worked out (hopefully we can remove the __radd__ lie as well)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2020

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

+ optuna/integration/__init__.py:32: error: Cannot infer type argument 1 of "__add__" of "list"

- ibis/client.py:468: error: Incompatible return value type (got "List[str]", expected "Set[Any]")
+ ibis/client.py:468: error: Incompatible return value type (got "List[Union[str, Any]]", expected "Set[Any]")

+ paasta_tools/config_utils.py:72: error: Unsupported operand types for + ("List[str]" and "List[str]")

- poetry/utils/setup_reader.py:174: error: Unsupported left operand type for + ("None")
+ poetry/utils/setup_reader.py:174: error: Unsupported operand types for + ("None" and "List[Any]")

+ mypy/fastparse.py:1071: error: Unsupported operand types for + ("List[expr]" and "List[expr]")  [operator]
+ mypy/fastparse2.py:917: error: Unsupported operand types for + ("List[expr]" and "List[expr]")  [operator]
+ mypy/test/testcmdline.py:59: error: Unsupported operand types for + ("List[str]" and "List[str]")  [operator]
+ mypyc/test/test_commandline.py:51: error: Unsupported operand types for + ("List[str]" and "List[str]")  [operator]

+ src/schemathesis/utils.py: note: In function "import_app":
+ src/schemathesis/utils.py:193: error: Argument 2 to "getattr" has incompatible type "Optional[str]"; expected "str"

+ pathod/language/websockets.py:245: error: List item 0 has incompatible type "Type[NestedFrame]"; expected "Type[_Component]"

+ src/graphql/type/validate.py:387: error: Unsupported operand types for + ("List[NamedTypeNode]" and "List[NamedTypeNode]")
+ src/graphql/validation/rules/overlapping_fields_can_be_merged.py:81: error: Unsupported operand types for + ("List[FieldNode]" and "List[FieldNode]")

+ src/pip/_internal/vcs/subversion.py:109: error: Result type of + incompatible in assignment

- pandas/io/common.py:452: error: Unsupported operand types for + ("List[Optional[str]]" and "List[str]")  [operator]
+ pandas/io/stata.py:892: error: unused 'type: ignore' comment
+ pandas/io/pytables.py:3387: error: unused 'type: ignore' comment

+ tornado/autoreload.py:238: error: Unsupported operand types for + ("List[str]" and "List[str]")

+ pydantic/color.py:199: error: unused 'type: ignore' comment

- scipy/ndimage/tests/__init__.py:11: error: Unsupported operand types for + ("List[Type[integer]]" and "List[Type[floating]]")  [operator]

hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this pull request Nov 2, 2020
This will run mypy_primer on mypy PRs. Changes on the open source
corpus are reported as comments on the PR.

We integrated this into typeshed CI; you can see examples here:
python/typeshed#3183
python/typeshed#4734

This might be a little slow. On typeshed this runs in 10 minutes, but
it's using a mypyc compiled wheel. It looks like it takes three minutes
to compile a MYPYC_OPT_LEVEL=0 wheel in our CI currently (for a ~2x
speedup), so that's probably worth it.
JukkaL pushed a commit to python/mypy that referenced this pull request Nov 4, 2020
This will run mypy_primer on mypy PRs. Changes on the open source
corpus are reported as comments on the PR.

We integrated this into typeshed CI; you can see examples here:
python/typeshed#3183
python/typeshed#4734

This might be a little slow. On typeshed this runs in 10 minutes, but
it's using a mypyc compiled wheel. It looks like it takes three minutes
to compile a MYPYC_OPT_LEVEL=0 wheel in our CI currently (for a ~2x
speedup), so that's probably worth it.

Co-authored-by: hauntsaninja <>
@srittau
Copy link
Collaborator

srittau commented Jan 23, 2021

I am going to close this for now due to the upcoming reshuffle. I still think this is generally worthwhile, though, although it obviously needs more investigation.

@mrkmndz
Copy link
Contributor

mrkmndz commented Apr 28, 2021

Is there any interest in reopening this?

@JelleZijlstra
Copy link
Member

Reopening directly won't work because of the change in file structure. It still seems worth fixing but tricky to get right.

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.

10 participants