bpo-32932: More revealing error message when non-str objects in all by zhangyangyu · Pull Request #5848 · python/cpython (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits11 Checks0 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
| if (skip_leading_underscores && PyUnicode_Check(name)) { |
|---|
| if (!PyUnicode_Check(name)) { |
| PyErr_Format(PyExc_TypeError, |
| "Item in __all__ must be str, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to include the module name as in Python version?
If skip_leading_underscores is true, the source of names is __dict__, not __all__.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it would be, but I hesitate. Is v always a module and I can use PyModule_GetName then? Or using __name__? __name__ could be altered.
| break; |
|---|
| } |
| PyErr_Format(PyExc_TypeError, |
| "%s in %U.%s must be str, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%U will fail if modname is not a string.
| @@ -111,6 +111,27 @@ def test_from_import_missing_attr_path_is_canonical(self): |
|---|
| self.assertIn(cm.exception.name, {'posixpath', 'ntpath'}) |
| self.assertIsNotNone(cm.exception) |
| def test_from_import_star_invalid_type(self): |
| TESTFNnoat = TESTFN[1:] |
| source = TESTFNnoat + os.extsep + "py" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no guaranties that TESTFN without the first character is a valid identifier or that it doesn't conflict with the name of a standard module. It would be safer to use TESTFN as the name of a directory name where put the Python file with predefined name (or different names for different cases). Or even use test.support.temp_dir().
| sys.path.insert(0, os.curdir) |
|---|
| try: |
| with open(source, "w") as f: |
| f.write("__all__ = [1]") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps None or a bytes object are more realistic examples.
| f.write("__all__ = [1]") |
|---|
| with self.assertRaisesRegex( |
| TypeError, f"{TESTFNnoat}.__all__ must be str"): |
| exec(f"from {TESTFNnoat} import *") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be safer to pass locals and globals dicts? And you can check that the import doesn't have effect on them.
| exec(f"from {TESTFNnoat} import *") |
|---|
| unload(TESTFNnoat) |
| with open(source, "w") as f: |
| f.write("import sys\nsys.modules[__name__].__dict__[1] = 1") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whouldn't be easier to use just globals()?
| def test_from_import_star_invalid_type(self): |
|---|
| TESTFNnoat = TESTFN[1:] |
| source = TESTFNnoat + os.extsep + "py" |
| sys.path.insert(0, os.curdir) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there is a special purposed helper in test.support or test.test_importlib.util for temporary modifying sys.path and safely clean up.
| @@ -111,6 +111,26 @@ def test_from_import_missing_attr_path_is_canonical(self): |
|---|
| self.assertIn(cm.exception.name, {'posixpath', 'ntpath'}) |
| self.assertIsNotNone(cm.exception) |
| def test_from_import_star_invalid_type(self): |
| with _ready_to_import() as (name, path): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, there was a ready helper in this file! Nice.
| break; |
|---|
| } |
| PyErr_Format(PyExc_TypeError, |
| "%s in %S.%s must be str, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%S can emit a warning or fail if modname is bytes. :(
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it's right. We could limit __name__ to str here. It's just another check. But the warning or failure won't cause the program to crash and they are hints for the programmer there is something not good in your program: do you really want __name__ a bytes object? Otherwise to avoid the warning or failure, any place using PyObject_Str should come after a comparison like !PyBytes_Check. _handle_fromlist could fail either.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a consenting adults thing where if you go so far as to set an object's name to a bytes object then you're already asking for trouble.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this true also for non-str objects in __all__?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but one is more possible than another. Anyway, adding a check here isn't too difficult so I'm fine asking for a guard.
| f.write("__all__ = [b'invalid_type']") |
|---|
| globals = {} |
| with self.assertRaisesRegex( |
| TypeError, f"{name}.__all__ must be str" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string is a regular expression, . should be escaped. And since name is just "spam", it is safe to substitute it. But in general case you could need to escape all metacharacters in it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll change it.
Okay, adding a check is not hard, so let's add it.
| } |
|---|
| else { |
| PyErr_Format(PyExc_TypeError, |
| "%s in %S.%s must be str, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%U can be used instead of %S.
| } |
|---|
| if (!PyUnicode_Check(modname)) { |
| PyErr_Format(PyExc_TypeError, |
| "__name__ must be a string, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message can be unclear. What if add "module" before "__name__"?
| @@ -4859,12 +4859,12 @@ import_all_from(PyObject *locals, PyObject *v) |
|---|
| } |
| if (!PyUnicode_Check(modname)) { |
| PyErr_Format(PyExc_TypeError, |
| "__name__ must be a string, not %.100s", |
| "module.__name__ must be a string, not %.100s", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to replace . with a space?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I don't get the point.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because "module" is not a name of any object.
But it was just a question. I don't know what is better in English. If . looks better to you, keep it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either is fine in practice, but leaving . out would technically be more accurate (since we're using "module" as an ordinary English word, rather than as a variable name)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!