bpo-15088 : Remove PyGen_NeedsFinalizing()

[nanjekyejoannah]

I made PyGen_NeedsFinalizing() internal instead of deleting the logic all together much as the function is not called anywhere in the codebase. Should I have done the later? @vstinner @pitrou

Edit: We decided to remove it all together.

https://bugs.python.org/issue15088

[pitrou]

Well, it makes no sense to make it internal if it's not used internally. I see two possible resolutions:

• If it's used by third-party code, leave it as-is (or deprecate it if the semantics aren't useful)
• If it's not used by third-party code, remove it (or deprecate it with a removal notice for a later release)

[nanjekyejoannah]

Well, it makes no sense to make it internal if it's not used internally. I see two possible resolutions:

* If it's used by third-party code, leave it as-is (or deprecate it if the semantics aren't useful)

* If it's not used by third-party code, remove it (or deprecate it with a removal notice for a later release)

My searches show references to this function in CPython cloned repositories. From your views, I think it is better to deprecate it with a removal notice for a later release.

[nanjekyejoannah]

I have deprecated the Function instead and will be removed in the next release.

[vstinner]

IMHO it's not worth it to have a deprecation period, since the function is not documented, not tested and I cannot find any project using it. See my rationale:
https://bugs.python.org/issue15088#msg351207

This function really looks like an internal function which leaked to the C API by mistake.

I propose to remove it immediately. What do you think @pitrou?

@pitrou wrote:

If it's not used by third-party code, remove it (or deprecate it with a removal notice for a later release)

Since I understand that he is fine with removing it.

We just have to document clearly its removal at: https://docs.python.org/dev/whatsnew/3.9.html#build-and-c-api-changes

[nanjekyejoannah]

Appveyor is giving me a red with a result I don't understand to resolve i.e "Tests result: ENV CHANGED" @vstinner ideas?

[aeros]

IMHO it's not worth it to have a deprecation period, since the function is not documented, not tested and I cannot find any project using it.

If it's not being used anywhere else, I think it's safe to remove it. Having a deprecation period adds additional maintenance. From my understanding, our general practice has been that anything which is not documented is not guaranteed to be part of the public API.

[pitrou]

I propose to remove it immediately. What do you think @pitrou?

No opposition from me, though I generally find it nicer to at least leave it deprecated for one version (you never know what non-public code may do).

[aeros]

Thanks for the PR @nanjekyejoannah.

From what I can tell, AppVeyor seems to be failing with ENV_CHANGED due to
1 test altered the execution environment: test_asyncio
Upon further inspection, it looks like env changed occurred only on line 1094 of the build log:

[00:09:54] 0:03:38 load avg: 5.63 [224/419/1] test_asyncio failed (env changed) (1 min 18 sec) -- running: test_multiprocessing_spawn (1 min 48 sec)

I'm not certain as to the cause though.

[aeros]

I would recommend some minor modifications to the Misc/NEWS and What's New entries.

Current:

The C function ``PyGen_NeedsFinalizing`` has been removed. It was not
documented, tested or even used anywhere in the code base after
PEP 442 implementation made it useless.
(Patch by Joannah Nanjekye)

Suggested:

The C function ``PyGen_NeedsFinalizing`` has been removed. It was not
documented, tested or used anywhere within CPython after the implementation
of :pep:`442`. Patch by Joannah Nanjekye.

I adjusted the phrasing, made the entry more succinct, and made a few formatting changes. Most notably, I condensed the lines so they could fit the maximum number of words on each line (within the 80 char width limit of reST) and added Sphinx markup for PEP 442 to provide an external link for readers.

[vstinner]

No opposition from me, though I generally find it nicer to at least leave it deprecated for one version (you never know what non-public code may do).

My worry is that nobody pays attention to compiler warnings.

I'm not sure that mentioning the deprecation in the documentation would be enough to notify potential users that it's deprecated.

Outside Python internals, I don't see why anyone would need this function?

PyGenObject structure is currently exposed, so it's possible to copy/paste this function in a closed source project if needed. It's short:

int 7(PyGenObject *gen)
{
    PyFrameObject *f = gen->gi_frame;
    if (f == NULL || f->f_stacktop == NULL) return 0;
    if (f->f_iblock > 0) return 1;
    return 0;
}

If we remove the function at the beginning of the 3.9 dev cycle, I also hope that enough users will test Python 3.8 beta versions, to check if they still compile without PyGen_NeedsFinalizing :-)

[vstinner]

@nanjekyejoannah:

Appveyor is giving me a red with a result I don't understand to resolve i.e "Tests result: ENV CHANGED" @vstinner ideas?

It's a random falure. If you make a change to your PR, a new CI job will be run and it will be fine (I hope).

I reported this asyncio bug more than one year ago. I even wrote a fix:
https://bugs.python.org/issue34037

But my fix has been reverted:
#13802

Does anyone want to make changes requested by @1st1?

[vstinner]

LGTM.

[vstinner]

@nanjekyejoannah: I let you modify the documentation according to @aeros167 suggestion, and then I plan to merge your change.

[aeros]

@vstinner

Does anyone want to make changes requested by @1st1?

Although I am not the most experienced with the internals of asyncio, I would be willing to try to help with this.

[nanjekyejoannah]

Thanks, @aeros167 for suggestions I have made the requested changes. @vstinner PTAL.

[vstinner]

I merged your PR @nanjekyejoannah, thanks. If anyone uses it, please complain before Python 3.9.0 release :-)