Skip to content

gh-102721: Improve coverage of _collections_abc._CallableGenericAlias#102722

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.

Sign up for GitHub

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
AlexWaygood merged 2 commits into from
Mar 16, 2023
Merged

gh-102721: Improve coverage of _collections_abc._CallableGenericAlias#102722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ed3dd8
gh-102721: Improve coverage of `_collections_abc._CallableGenericAlias`
sobolevn Mar 15, 2023
afe8f37
Apply suggestions from code review
sobolevn Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions Lib/_collections_abc.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -481,15 +481,8 @@ def __getitem__(self, item):
# rather than the default types.GenericAlias object. Most of the
# code is copied from typing's _GenericAlias and the builtin
# types.GenericAlias.

if not isinstance(item, tuple):
item = (item,)
# A special case in PEP 612 where if X = Callable[P, int],
# then X[int, str] == X[[int, str]].
if (len(self.__parameters__) == 1
and _is_param_expr(self.__parameters__[0])
and item and not _is_param_expr(item[0])):
item = (item,)
Comment on lines -487 to -492
Copy link
Member

@AlexWaygoodAlexWaygoodMar 15, 2023•
edited
Loading

Choose a reason for hiding this comment

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

This makes me slightly nervous, but I can't find any changes in behaviour from removing this code.

@Fidget-Spinner and @serhiy-storchaka, can either of you think of anything bad that could happen if we remove this code? The whole test suite passes with it removed.

Copy link
MemberAuthor

@sobolevnsobolevnMar 15, 2023

Choose a reason for hiding this comment

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

Yes, me too. Let's wait for others to comment.

Copy link
Member

@gvanrossumgvanrossumMar 16, 2023

Choose a reason for hiding this comment

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

It makes me nervous too. In case you're looking for an example that triggers this path, here's one:

fromtypingimportParamSpecP=ParamSpec("P") fromcollections.abcimportCallableC=Callable[[P], int] print(C[str])

However I agree that if I just comment this out, the output is the same. It's possible that @serhiy-storchaka fixed the issue in the C code (the super() method) -- the code here is from Aug 4th, 2021, while he touched _Py_subs_parameters on May 8th, 2022.

Copy link
Member

@AlexWaygoodAlexWaygoodMar 16, 2023

Choose a reason for hiding this comment

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

Yes, I found examples that triggered this code path, but for all of the examples I found, I couldn't find any differences in behaviour with this code deleted (repr, __args__, __parameters__, equality)

Copy link
Member

@gvanrossumgvanrossumMar 16, 2023

Choose a reason for hiding this comment

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

Someone could trace through the C code that gets called and see if it does the right thing.

Copy link
MemberAuthor

@sobolevnsobolevnMar 16, 2023

Choose a reason for hiding this comment

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

Looks like _unpack_args C function which was added in May now handles this case: https://github.com/python/cpython/blame/2dc94634b50f0e5e207787e5ac1d56c68b22c3ae/Objects/genericaliasobject.c#L361

It unpacks nested tuples to a flat format, so - this code is not needed anymore.

Copy link
Member

@AlexWaygoodAlexWaygoodMar 16, 2023

Choose a reason for hiding this comment

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

I don't suppose the pure-Python code could be useful for PyPy, could it? (Don't see how it could, just throwing that out there)

Copy link
MemberAuthor

@sobolevnsobolevnMar 16, 2023

Choose a reason for hiding this comment

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

Sorry, the only other iterperter I worked on is RustPython.


new_args = super().__getitem__(item).__args__

Expand Down
39 changes: 32 additions & 7 deletions Lib/test/test_typing.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1921,14 +1921,29 @@ def test_weakref(self):
self.assertEqual(weakref.ref(alias)(), alias)

def test_pickle(self):
global T_pickle, P_pickle, TS_pickle # needed for pickling
Callable = self.Callable
alias = Callable[[int, str], float]
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
s = pickle.dumps(alias, proto)
loaded = pickle.loads(s)
self.assertEqual(alias.__origin__, loaded.__origin__)
self.assertEqual(alias.__args__, loaded.__args__)
self.assertEqual(alias.__parameters__, loaded.__parameters__)
T_pickle = TypeVar('T_pickle')
P_pickle = ParamSpec('P_pickle')
TS_pickle = TypeVarTuple('TS_pickle')

samples = [
Callable[[int, str], float],
Callable[P_pickle, int],
Callable[P_pickle, T_pickle],
Callable[Concatenate[int, P_pickle], int],
Callable[Concatenate[*TS_pickle, P_pickle], int],
]
for alias in samples:
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
with self.subTest(alias=alias, proto=proto):
s = pickle.dumps(alias, proto)
loaded = pickle.loads(s)
self.assertEqual(alias.__origin__, loaded.__origin__)
self.assertEqual(alias.__args__, loaded.__args__)
self.assertEqual(alias.__parameters__, loaded.__parameters__)

del T_pickle, P_pickle, TS_pickle # cleaning up global state

def test_var_substitution(self):
Callable = self.Callable
Expand All@@ -1954,6 +1969,16 @@ def test_var_substitution(self):
self.assertEqual(C5[int, str, float],
Callable[[typing.List[int], tuple[str, int], float], int])

def test_type_subst_error(self):
Callable = self.Callable
P = ParamSpec('P')
T = TypeVar('T')

pat = "Expected a list of types, an ellipsis, ParamSpec, or Concatenate."

with self.assertRaisesRegex(TypeError, pat):
Callable[P, T][0, int]

def test_type_erasure(self):
Callable = self.Callable
class C1(Callable):
Expand Down