gh-139276: Remove generator type check in _testcapimodule.c:raise_SIGINT_then_send_None #139252
+3
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please create an issue for this one first (no need for a NEWS entry however).
Maybe we should instead mark the test as CPython-only? it seems that it's quite specific to CPython in the sense that a comment says that we check for signals before every bytecode instruction (but maybe this is different in other implementations). There are some other test functions that also check for specific types which could be different on other implementations, so maybe it's a wider issue (I don't know the answer to that question). cc @njsmith as the original PR's author (though I don't know if they are still active on GitHub). |
jbower-fb
changed the title
As an alternative implementation author, this is functionality I'd probably be aiming to reproduce by default. Or at least it would be very considered if we decided not to handle signals in the same places as CPython. If we didn't do that, there will probably be some existing code which depends on this behavior and subtly breaks.
There area a few other issues with using our own types for generators... At some point I, or someone I work with will hopefully come up with some proposals to fix these. For now we've been able to work around most of them without upstream changes though. |
sobolevn
previously approved these changes
Sep 24, 2025
|
(sorry, "approve" is a misclick, I meant to ask a question) |
|
Ah yes, I didn't even notice that before. Fixed now. |
sobolevn
approved these changes
Sep 24, 2025
In the Cinder JIT we use a different type for generators, which breaks the test which uses this function. In general I believe the intent with generators is they have the right structure rather than type, so a failure to find the 'send()' method is arguably more correct if the wrong object is used.
jbower-fb
force-pushed
the
removetypecheck
branch
from
95cedb9 to
61543b3
Compare
|
|
The buildbot failure looks like it's unrelated to this change. Past runs on the same buildbot have failed with the same error. |
|
@jbower-fb We could backport this change up to 3.13. Would it be good for you that we do so? or is it fine to keep it in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the Cinder JIT we use a different type for generators, which breaks the test which uses this function.
In general I believe the intent with generators is they have the right structure rather than type, so a failure to find the
send()method is arguably more correct if the wrong object is used.