Skip to content

bpo-31315: Fix an assertion failure in imp.create_dynamic(), when spec.name is not a string #3257

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
merged 5 commits into from
Sep 19, 2017
Merged

bpo-31315: Fix an assertion failure in imp.create_dynamic(), when spec.name is not a string #3257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99ce6b9
init commit
orenmn Aug 31, 2017
854d84b
Add missing blank line
ncoghlan Sep 1, 2017
b65be79
improve the test and its doc
orenmn Sep 1, 2017
9107d14
improve the docstring
orenmn Sep 1, 2017
64e68ff
remove requires_create_dynamic(), and make the test more realistic
orenmn Sep 9, 2017
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Lib/test/test_imp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ def requires_load_dynamic(meth):
meth = support.cpython_only(meth)
return unittest.skipIf(not hasattr(imp, 'load_dynamic'),
'imp.load_dynamic() required')(meth)
def requires_create_dynamic(meth):
Copy link
Contributor

@ncoghlan ncoghlan Sep 1, 2017

Choose a reason for hiding this comment

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

Readability improvement: blank line before the function
"""Decorator to skip a test if not running under CPython or lacking
imp.create_dynamic()."""
Copy link
Member

@ericsnowcurrently ericsnowcurrently Sep 1, 2017

Choose a reason for hiding this comment

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

Let me be clear that my recommendation here is not critical. Rather it is a reflection of my belief that little suboptimal things add up over time to become pain points. Docstrings (including in test code) are one of those things we cut corners on sometimes.

While there is precedent (unfortunately) for docstrings formatted this way, I'd rather this docstring were a bit more idiomatic. It's better to take a little time now to strengthen a healthier precedent. This is a case where a little bit of extra time spent adds up over time to make a positive difference. I don't mean to be pedantic here about a relatively insignificant style point or to waste anyone's time, but instead want to encourage a more thoughtful approach with regard to the little things. :) With that out of the way...

I'd expect a docstring here like one of the following (and including some shortening):

# one line """Decorator to skip a test if not running under CPython or lacking imp.create_dynamic()."""

or

# one line with separate quotation mark lines """ Decorator to skip a test if not running under CPython or lacking imp.create_dynamic(). """

or

# split into multiple lines (my preference) """A decorator to skip tests that rely on imp.create_dynamic(). Not all Python implementations necessarily provide imp.create_dynamic(). This decorators causes decorated tests to be skipped through the normal unittest mechanism. """
meth = support.cpython_only(meth)
return unittest.skipIf(not hasattr(imp, 'create_dynamic'),
'imp.create_dynamic() required')(meth)
Copy link
Member

@ericsnowcurrently ericsnowcurrently Sep 1, 2017

Choose a reason for hiding this comment

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

FWIW, as a reader it's easy to miss something when more than one thing is happening on a line. It might be more clear to break this up:

deco = unittest.skipIf(not hasattr(imp, 'create_dynamic'), 'imp.create_dynamic() required') return deco(meth)

or even

supported = hasattr(imp, 'create_dynamic') deco = unittest.skipIf(not supported, 'imp.create_dynamic() required') return deco(meth)


@unittest.skipIf(_thread is None, '_thread module is required')
Expand Down Expand Up @@ -318,6 +324,16 @@ def test_load_source(self):
with self.assertRaisesRegex(ValueError, 'embedded null'):
imp.load_source(__name__, __file__ + "\0")

@requires_create_dynamic
def test_issue31315(self):
# There shouldn't be an assertion failure in imp.create_dynamic(),
# when spec.name is not a string.
class BadSpec:
name = 42
Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 2, 2017

Choose a reason for hiding this comment

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

Use something more realistic. A bytes object or None.
origin = 'foo'
Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 2, 2017

Choose a reason for hiding this comment

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

If origin is not a string SystemError can be raised. This should be fixed too.
Copy link
Contributor Author

@orenmn orenmn Sep 9, 2017

Choose a reason for hiding this comment

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

what code causes a SystemError?
I got a TypeError: bad argument type for built-in operation both on my Windows and Ubuntu.
Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 19, 2017

Choose a reason for hiding this comment

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

Your are right. I thought _PyUnicode_AsUnicode() raises SystemError, but it raises TypeError. No additional check is needed.
with self.assertRaises(TypeError):
imp.create_dynamic(BadSpec())


class ReloadTests(unittest.TestCase):

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2017-09-01-00-40-58.bpo-31315.ZX20bl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix an assertion failure in imp.create_dynamic(), when spec.name is not a
string. Patch by Oren Milman.
5 changes: 5 additions & 0 deletions Python/importdl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
if (name_unicode == NULL) {
return NULL;
}
if (!PyUnicode_Check(name_unicode)) {
PyErr_SetString(PyExc_TypeError,
"spec.name must be a string");
goto error;
}

name = get_encoded_name(name_unicode, &hook_prefix);
if (name == NULL) {
Expand Down