| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| PyObject *iter_o = PyStackRef_AsPyObjectBorrow(iter); | ||
| PyObject *next_o = (*Py_TYPE(iter_o)->tp_iternext)(iter_o); | ||
| PyObject *next_o = NULL; | ||
| if (PyIter_Check(iter_o)) { |
There was a problem hiding this comment.
Calling PyIter_Check feels a little wasteful. Instead, I'd assign (*Py_TYPE(iter_o)->tp_iternext) to a variable and check if it's NULL. If it is NULL, we should raise a TypeError. Your code instead makes it immediately end the loop, which doesn't feel like the right behavior.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, I've tried to add a TypeError
Such piece of code is provided:
PyTypeObject *type = Py_TYPE(iter_o);
iternextfunc iternext = type->tp_iternext;
if (iternext == NULL) {
_PyErr_Format(tstate, PyExc_TypeError,
"'for' requires an object with "
"__iter__ method, got %.100s",
type->tp_name);
DECREF_INPUTS();
ERROR_IF(true, error);
}
PyObject *next_o = (*iternext)(iter_o);
And there is a temporarily result of my test case:
-> % ./python -m unittest -v test.test_generators.GeneratorTest.test_issue125038
test_issue125038 (test.test_generators.GeneratorTest.test_issue125038) ... ERROR
======================================================================
ERROR: test_issue125038 (test.test_generators.GeneratorTest.test_issue125038)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/mikhail.efimov/projects/cpython/Lib/test/test_generators.py", line 274, in test_issue125038
l = list(g)
File "/home/mikhail.efimov/projects/cpython/Lib/test/test_generators.py", line 272, in <genexpr>
g = (x for x in range(10))
~~~~~^^^^
TypeError: 'for' requires an object with __iter__ method, got range
----------------------------------------------------------------------
Ran 1 test in 0.001s
It seems like such a message doesn't provide any clarity.
Do you have any better suggestions about error message?
Sorry, something went wrong.
There was a problem hiding this comment.
That seems like a pretty good error message, much better than silently stopping the loop.
Sorry, something went wrong.
| except TypeError: | ||
| return "TypeError" | ||
|
|
||
| # This should not raise |
There was a problem hiding this comment.
I think you want with self.assertRaisesRegex.
Sorry, something went wrong.
SIGSEGV on generators in case of gi_frame.f_locals is fixed. This applies to _FOR_ITER bytecode. Similar checks are added to _FOR_ITER_TIER_TWO and INSTRUMENTED_FOR_ITER bytecode implementations.
|
We should confirm with @markshannon whether we should fix the interpreter or f_locals. Maybe it's a valid assumption? PEP 667 does not implement everything dict-like for FrameLocalsProxy, but maybe this is necessary? We are adding a small overhead to a very commonly used instruction if I understood it correctly. |
Sorry, something went wrong.
|
I looked at PEP 668 and it seems to assume that mutating f_locals is supported, which makes me think that we need to add this check. I wouldn't personally mind blocking the f_locals mutation instead if you can find a principled way to do it.
Yes, that's right; we're adding a NULL check and a branch. However, FOR_ITER has a couple of specialized versions, so the overhead should be minimal when iterating over the most common types. Still, it may be worth benchmarking how bad the overhead is. |
Sorry, something went wrong.
There was a problem hiding this comment.
This looks correct to me, but will leave to Mark to see if there's a better solution.
Sorry, something went wrong.
…e-125038.ffSLCz.rst News improvement Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
Thanks for your advice, it definitely looks better now. But I have a question. |
Sorry, something went wrong.
|
It seems that this code is incorrect now: g = (x for x in range(10)) g.gi_frame.f_locals['.0'] = range(20) But very similar code is correct: g = (x for x in range(10)) g.gi_frame.f_locals['.0'] = iter(range(20)) Maybe, some change in framelocalsproxy_setitem function can be provided? if (_PyUnicode_EqualToASCIIString(key, ".0")) {
PyObject *it = PyObject_GetIter(value);
if (it != NULL) {
value = it;
}
}
I've provided this change, but it breaks two tests: test_proxy_key_stringlikes_overwrite and test_proxy_key_stringlikes_ftrst_write. Those tests are authored by @encukou and @ncoghlan. |
Sorry, something went wrong.
|
Setting .0 to a iter(range(20)) should work for now, yes. I don't know what you're trying to use this for at a higher level, but I'd recommend trying to find a solution that doesn't involve mutating f_locals; that's a low-level interface that should probably only be used by tools like debuggers. I would oppose adding special handling in the FrameLocalsProxy that mutates objects; doing so would complicate the implementation and make it less useful for tools like debuggers. |
Sorry, something went wrong.
|
Actually, there is no real world use case when changing underlying iterator for a generator is needed. Main purpose of my questions is achieving some clarity. |
Sorry, something went wrong.
…cpython into sigsegv_fix_in_iterators
|
I've moved my test from test_generators.py to test_frame.py, renamed it, and added more test cases to emphasize current code behavior and save it explicitly in tests. |
Sorry, something went wrong.
… section for assignment expression topic (python#125074)
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> Co-authored-by: Tomas R <tomas.roun8@gmail.com>
…re enabled by default (python#123859)
…ython#124657) * pythongh-124612: Use ghcr.io/python/autoconf instead of public image * Update
…ython#124669) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: musvaage <musvaage@users.noreply.github.com>
* Replace unicode_compare_eq() with unicode_eq(). * Use unicode_eq() in setobject.c. * Replace _PyUnicode_EQ() with _PyUnicode_Equal(). * Remove unicode_compare_eq() and _PyUnicode_EQ().
…008 implementation issues (python#125151) Skip test_fma_zero_result on NetBSD due to IEE 754-2008 implementation issues
…124974) Now it returns a tuple of up to 100 strings (an empty tuple on most locales). Previously it returned the first item of that tuple or an empty string.
…pire far in the future by default (pythonGH-107594) This allows testing Y2038 with system time set to after that, so that actual Y2038 issues can be exposed, and not masked by expired certificate errors. Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
| Back | FazBrowse Home | New Git URL |
SIGSEGV on generators in case of gi_frame.f_locals is fixed.
This applies to _FOR_ITER bytecode implementation.
Similar checks are added to _FOR_ITER_TIER_TWO and INSTRUMENTED_FOR_ITER bytecode implementations.