gh-87390: Fix starred tuple equality and pickling by mrahtz · Pull Request #92249 · python/cpython

mrahtz · 2022-05-03T18:30:09Z

Hilariously, before this PR, it turned out that *tuple[int] was equal to tuple[int]. Fixing it, I realised that pickling was also broken: it didn't preserve gaobject.starred. This PR fixes both.

I've tested for refleaks with python3 -m test -v test_genericalias -R 3:3, and it came back clean, so I think we're good.

@Fidget-Spinner I guess you're the one most familiar with this code? :)

JelleZijlstra · 2022-05-03T19:28:26Z

I think this is worth a NEWS entry; it's been a while since we put the rest of this code into main. I'll review the PR now.

JelleZijlstra

Thanks! I'm a little unhappy about making unpacked a public argument to the constructor, but I'm not sure how else we'd get pickling to work.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

mrahtz · 2022-05-03T20:52:47Z

I think this is worth a NEWS entry; it's been a while since we put the rest of this code into main.

I've added a NEWS entry for this specific change, but just to double check - were you thinking of this specific change, or the rest of the PEP 646-related changes we made to genericaliasobject.c recently? (I had to check, but we did have something in the NEWS for 3.11.0a7 saying that we "Allow unpacking types.GenericAlias objects".)

JelleZijlstra · 2022-05-03T20:55:03Z

I think this is worth a NEWS entry; it's been a while since we put the rest of this code into main.
I've added a NEWS entry for this specific change, but just to double check - were you thinking of this specific change, or the rest of the PEP 646-related changes we made to genericaliasobject.c recently? (I had to check, but we did have something in the NEWS for 3.11.0a7 saying that we "Allow unpacking types.GenericAlias objects".)

Just for this change (the PR title would be good as a NEWS entry too).

JelleZijlstra · 2022-05-03T23:37:37Z

+ "must be a bool (since it's a flag controlling "
+ "whether the alias is unpacked)");


Suggested change
"must be a bool (since it's a flag controlling "
"whether the alias is unpacked)");
"must be a bool");
We don't need the explanation.

Fidget-Spinner

Great work! We're pretty close.

Fidget-Spinner · 2022-05-04T06:06:21Z

+ PyObject *starred = PyBool_FromLong(alias->starred);
+ PyObject *value = Py_BuildValue("O(OOO)", Py_TYPE(alias),
+ alias->origin, alias->args, starred);
+ // Avoid double increment of reference count on Py_True/Py_False - once from
+ // PyBool_FromLong, and once from Py_BuildValue.
+ Py_CLEAR(starred);
+ return value;


How about:
Suggested change
PyObject*starred=PyBool_FromLong(alias->starred);
PyObject*value=Py_BuildValue("O(OOO)", Py_TYPE(alias),
alias->origin, alias->args, starred);
// Avoid double increment of reference count on Py_True/Py_False - once from
// PyBool_FromLong, and once from Py_BuildValue.
Py_CLEAR(starred);
returnvalue;
PyObject*starred=alias->starred ? Py_True : Py_False;
returnPy_BuildValue("O(OOO)", Py_TYPE(alias),
alias->origin, alias->args, starred);
BTW, if you don't want to use the code above, you can avoid the double incref by using the format string O(OON). N doesn't incref. https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue

Fidget-Spinner · 2022-05-04T06:09:35Z

 return NULL;
 }
- if (!_PyArg_CheckPositional("GenericAlias", PyTuple_GET_SIZE(args), 2, 2)) {
+ if (!_PyArg_CheckPositional("GenericAlias", PyTuple_GET_SIZE(args), 2, 3)) {


Please update the creation signature in the docs https://docs.python.org/3/library/types.html#types.GenericAlias.
Also, this doesn't break backwards compatibility right? IIUC, the two-argument form will still work.

Fidget-Spinner · 2022-05-04T06:20:06Z

 PyObject *origin = PyTuple_GET_ITEM(args, 0);
 PyObject *arguments = PyTuple_GET_ITEM(args, 1);
+


This code has gotten complex enough that I recommend using https://docs.python.org/3/c-api/arg.html#c.PyArg_ParseTuple or https://docs.python.org/3/c-api/arg.html#c.PyArg_UnpackTuple instead to parse our arguments for us.
The format value list is on that page. But off the top of my head, it should be OO|O:GenericAlias. Then you keep your bool checking/error raising code below.

Argument clinic can also be used here.

Fidget-Spinner · 2022-05-04T06:21:14Z

+ "whether the alias is unpacked)");
+ return NULL;
+ }
+ starred = PyLong_AsLong(py_starred);


I'm surprised you don't need to cast to (bool) here (or maybe it's implicit, I don't remember how C treats this).

Fidget-Spinner · 2022-05-04T06:24:02Z

Azure pipelines patcheck fail is unrelated, I will fix it on main.

Fidget-Spinner · 2022-05-04T06:43:02Z

Azure pipelines patcheck fail is unrelated, I will fix it on main.

Nevermind, it seems that 65f88a6 fixed the whitespace issues, so this just needs updating from main.

serhiy-storchaka

I do not like an idea about extending GenericAlias() with the third parameter. It is a public API.

I'll provide an alternate PR.

serhiy-storchaka · 2022-05-05T14:52:25Z

See #92337.

carljm · 2022-05-07T04:21:15Z

Looks like this can be closed since the alternative was already merged.

Fix starred tuple equality and pickling
335747d

mrahtz force-pushed the unpacked-tuple-equality branch from b808ffd to 335747dCompare May 3, 2022 18:30

bedevere-bot added the awaiting review label May 3, 2022

Update test comment
5d384b9

mrahtz mentioned this pull request May 3, 2022
gh-91162: Disallow tuple[T][*anything] #92255
Closed

JelleZijlstra reviewed May 3, 2022
View reviewed changes

Comment threadObjects/genericaliasobject.c Outdated
Comment threadObjects/genericaliasobject.c

mrahtzand others added 3 commits May 3, 2022 21:33

PyTuple_Size → PyTuple_GET_SIZE
4143451
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Check type of unpackedness argument to GenericAlias
f33b28c

📜🤖 Added by blurb_it.
57e18e8

JelleZijlstra reviewed May 3, 2022
View reviewed changes

JelleZijlstra self-assigned this May 3, 2022

Fidget-Spinner reviewed May 4, 2022
View reviewed changes

AlexWaygood added the topic-typing label May 4, 2022

serhiy-storchaka reviewed May 5, 2022
View reviewed changes

serhiy-storchaka mentioned this pull request May 5, 2022
gh-87390: Fix starred tuple equality and pickling (alt) #92337
Merged

carljm closed this May 7, 2022

mrahtz deleted the unpacked-tuple-equality branch May 7, 2022 11:22

		"must be a bool (since it's a flag controlling "
		"whether the alias is unpacked)");

	"must be a bool (since it's a flag controlling "
	"whether the alias is unpacked)");
	"must be a bool");

		PyObject *origin = PyTuple_GET_ITEM(args, 0);
		PyObject *arguments = PyTuple_GET_ITEM(args, 1);

Uh oh!

Uh oh!

Conversation

mrahtz commented May 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelleZijlstra commented May 3, 2022

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrahtz commented May 3, 2022

Uh oh!

JelleZijlstra commented May 3, 2022

Uh oh!

JelleZijlstraMay 3, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

kumaraditya303May 4, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerMay 4, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented May 4, 2022

Uh oh!

Fidget-Spinner commented May 4, 2022

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 5, 2022

Uh oh!

carljm commented May 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mrahtz commented May 3, 2022•
edited
Loading