Yet another attempt to fix list.__add__

[Akuli]

Fixes#8292
Fixes#2383

[AlexWaygood]

I like the look of that primer diff. It looks like there's a few regressions in there, but it also looks like it fixes more things than it breaks.

[JelleZijlstra]

The regressions seem to be in subclasses of list that override __add__. I think that's fine.

Could you add some test cases? I want to make sure this also works well with pyright.

[Akuli]

My thoughts on the regressions:

+ pandas/core/indexes/frozen.py:65: error: Incompatible types in assignment (expression has type "Callable[[FrozenList, Any], FrozenList]", base class "list" defined the type as overloaded function) [assignment]
+ pandas/core/reshape/merge.py:1616: error: Cannot determine type of "__add__" [has-type]
+ pandas/core/reshape/reshape.py:313: error: Cannot determine type of "__add__" [has-type]

First error: A list[Any] subclass FrozenList does __add__ = __iadd__ = union, where union is defined as taking an implicitly-Any-typed argument and returning FrozenList. With this PR, the return type is no longer list[Any] in all cases, and maybe list[Any] and list[Any | Foo] are incompatible. But the pandas code is already quite Any-typed, so I think a # type: ignore is fine there.

Second error: join_levels = join_levels + [restore_levels] confuses mypy, because it causes the type of join_levels to depend on itself in a complicated way. I have seen this error message in the past when multiple types (often in multiple files) depend on each other in a cyclic way. (I wish the error message actually said what's wrong instead of making me memorize its meaning...) But I don't think this is a problem, because you can usually .append(), or just add an explicit annotation if you don't want to append, which is useful if you have another reference to the same list that you don't want to change.

Third error comes from this code:

new_levels: FrozenList|list[Index]
ifisinstance(value_columns, MultiIndex):
new_levels=value_columns.levels+ (self.removed_level_full,) # error: Cannot determine type of "__add__" [has-type]new_names=value_columns.names+ (self.removed_name,)
new_codes= [lab.take(propagator) forlabinvalue_columns.codes]
else:
new_levels= [
value_columns,
self.removed_level_full,
]
new_names= [value_columns.name, self.removed_name]
new_codes= [propagator]

There's already an explicit type annotation on new_levels, so how should this be fixed in pandas?

[Akuli]

I have added a simple test for pyright. I don't think we should test for anything more specific than that, because the problems were mypy-specific, and it's hard to say what exactly would break in a different type checker.

[Akuli]

Even if we don't know how the pandas error could be fixed, apart from # type: ignore, this fixes so many other problems that I think it's worth merging anyway.

[AlexWaygood]

LGTM. If we wanted to be extra-extra careful we could also try running the mypy test suite with this fix applied and see if that uncovers any other regressions.

[github-actions]

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

ignite (https://github.com/pytorch/ignite)
+ ignite/handlers/time_profilers.py:257: error: Unused "type: ignore" comment
pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/computation/scope.py:327: error: Unused "type: ignore" comment+ pandas/core/computation/scope.py:328: error: Unused "type: ignore" comment+ pandas/io/common.py:577: error: Unused "type: ignore" comment+ pandas/core/indexes/frozen.py:65: error: Incompatible types in assignment (expression has type "Callable[[FrozenList, Any], FrozenList]", base class "list" defined the type as overloaded function) [assignment]+ pandas/io/pytables.py:3465: error: Unused "type: ignore" comment- pandas/core/groupby/generic.py:686: error: Incompatible types in assignment (expression has type "List[ndarray[Any, dtype[_SCT]]]", variable has type "List[ndarray[Any, dtype[signedinteger[Any]]]]") [assignment]+ pandas/core/reshape/merge.py:1616: error: Cannot determine type of "__add__" [has-type]+ pandas/core/groupby/generic.py:690: error: Unused "type: ignore" comment+ pandas/core/reshape/reshape.py:313: error: Cannot determine type of "__add__" [has-type]+ pandas/tests/resample/test_datetime_index.py:1303: error: Unused "type: ignore" comment+ pandas/tests/frame/indexing/test_setitem.py:268: error: Unused "type: ignore" comment+ pandas/tests/tools/test_to_datetime.py:1088: error: Unused "type: ignore" comment+ pandas/tests/tools/test_to_datetime.py:1092: error: Unused "type: ignore" comment
mypy (https://github.com/python/mypy)
+ mypy/test/testpep561.py:135: error: Unused "type: ignore[operator]" comment
ibis (https://github.com/ibis-project/ibis)
- ibis/tests/expr/test_datatypes.py:35: error: List comprehension has incompatible type List[Tuple[str, GeoSpatial]]; expected List[Tuple[Collection[object], Variadic]]- ibis/backends/postgres/registry.py:172: error: List item 0 has incompatible type "Tuple[str, Callable[[Any, Any], Any]]"; expected "Tuple[str, Callable[[VarArg(Any)], str]]"- ibis/backends/postgres/registry.py:191: error: Cannot infer type of lambda
rotki (https://github.com/rotki/rotki)
+ rotkehlchen/chain/ethereum/modules/adex/types.py:251: error: Unused "type: ignore" comment+ rotkehlchen/chain/ethereum/modules/adex/types.py:252: error: Unused "type: ignore" comment+ rotkehlchen/chain/ethereum/modules/adex/types.py:253: error: Unused "type: ignore" comment+ rotkehlchen/chain/ethereum/modules/adex/adex.py:786: error: Unused "type: ignore" comment
spark (https://github.com/apache/spark)
+ python/pyspark/ml/tuning.py:464: error: Unused "type: ignore" comment
pydantic (https://github.com/samuelcolvin/pydantic)
+ pydantic/color.py:199: error: Unused "type: ignore" comment
psycopg (https://github.com/psycopg/psycopg)
+ tests/test_adapt.py:412: error: Unused "type: ignore" comment

[AlexWaygood]

Looks like this PR might also fix #2383

[Akuli]

It does. I edited the description.

[Akuli]

I ran the mypy test suite locally. This break's mypy's testMapStr test, but only because the error message changes.

[Akuli]

I think the mypy test failure points out a real problem. Consider this code (so buggy that doesn't show up in mypy_primer much):

asd: list[str] = []
print(asd+1)

Error message before this PR:

asd.py:2: error: Unsupported operand types for + ("List[str]" and "int")

Error message after this PR:

asd.py:2: error: No overload variant of "__add__" of "list" matches argument type "int"
asd.py:2: note: Possible overload variants:
asd.py:2: note: def __add__(self, List[str]) -> List[str]
asd.py:2: note: def [_S] __add__(self, List[_S]) -> List[Union[_S, str]]

This isn't exactly very user friendly, as List[str] is hidden deep inside the error message and it doesn't mention +.

Should we figure out what mypy changes would be needed to get a simpler error message before we merge this PR?

[hauntsaninja]

Thanks for making this work! I'm also in favor of merging.

I don't think the error usability is bad enough to prevent merging. IIRC we've had other use of overloads for dunder methods recently that have caused similar changes to error messages.

[AlexWaygood]

I don't think the error usability is bad enough to prevent merging. IIRC we've had other use of overloads for dunder methods recently that have caused similar changes to error messages.

I agree with this. While it's unfortunate to make error messages worse, it also feel slightly outside typeshed's purview to worry too much about them -- I feel like that should be fixed on mypy's side, if possible.

The third pandas error you highlight in #8293 (comment) seems more problematic to me, but I think they may have to just type: ignore that one away. They might be able to do something fancy like casting to a SupportsAdd protocol, but the type: ignore probably comes to the same thing, and would be cleaner.