bpo-32932: More revealing error message when non-str objects in all #5848

zhangyangyu · 2018-02-24T13:49:44Z

https://bugs.python.org/issue32932

serhiy-storchaka · 2018-02-24T14:01:03Z

Python/ceval.c

-        if (skip_leading_underscores && PyUnicode_Check(name)) {
+        if (!PyUnicode_Check(name)) {
+            PyErr_Format(PyExc_TypeError,
+                         "Item in __all__ must be str, not %.100s",


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__.

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.

serhiy-storchaka · 2018-02-26T08:55:21Z

Python/ceval.c

+                break;
+            }
+            PyErr_Format(PyExc_TypeError,
+                         "%s in %U.%s must be str, not %.100s",


%U will fail if modname is not a string.

serhiy-storchaka · 2018-02-26T09:01:02Z

Lib/test/test_import/__init__.py

@@ -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"


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().

serhiy-storchaka · 2018-02-26T09:02:11Z

Lib/test/test_import/__init__.py

+        sys.path.insert(0, os.curdir)
+        try:
+            with open(source, "w") as f:
+                f.write("__all__ = [1]")


Perhaps None or a bytes object are more realistic examples.

serhiy-storchaka · 2018-02-26T09:05:35Z

Lib/test/test_import/__init__.py

+                f.write("__all__ = [1]")
+            with self.assertRaisesRegex(
+                    TypeError, f"{TESTFNnoat}.__all__ must be str"):
+                exec(f"from {TESTFNnoat} import *")


Wouldn't be safer to pass locals and globals dicts? And you can check that the import doesn't have effect on them.

serhiy-storchaka · 2018-02-26T09:06:59Z

Lib/test/test_import/__init__.py

+                exec(f"from {TESTFNnoat} import *")
+            unload(TESTFNnoat)
+            with open(source, "w") as f:
+                f.write("import sys\nsys.modules[__name__].__dict__[1] = 1")


Whouldn't be easier to use just globals()?

serhiy-storchaka · 2018-02-26T09:10:53Z

Lib/test/test_import/__init__.py

+    def test_from_import_star_invalid_type(self):
+        TESTFNnoat = TESTFN[1:]
+        source = TESTFNnoat + os.extsep + "py"
+        sys.path.insert(0, os.curdir)


Perhaps there is a special purposed helper in test.support or test.test_importlib.util for temporary modifying sys.path and safely clean up.

serhiy-storchaka · 2018-02-26T17:04:18Z

Lib/test/test_import/__init__.py

@@ -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):


Oh, there was a ready helper in this file! Nice.

serhiy-storchaka · 2018-02-26T17:07:40Z

Python/ceval.c

+                break;
+            }
+            PyErr_Format(PyExc_TypeError,
+                         "%s in %S.%s must be str, not %.100s",


%S can emit a warning or fail if modname is bytes. :(

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.

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.

Isn't this true also for non-str objects in __all__?

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.

serhiy-storchaka · 2018-02-26T17:13:42Z

Lib/test/test_import/__init__.py

+                f.write("__all__ = [b'invalid_type']")
+            globals = {}
+            with self.assertRaisesRegex(
+                TypeError, f"{name}.__all__ must be str"


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.

Yes. I'll change it.

…e32932

zhangyangyu · 2018-03-24T06:38:59Z

Okay, adding a check is not hard, so let's add it.

serhiy-storchaka · 2018-03-24T07:25:06Z

Python/ceval.c

+            }
+            else {
+                PyErr_Format(PyExc_TypeError,
+                             "%s in %S.%s must be str, not %.100s",


%U can be used instead of %S.

serhiy-storchaka · 2018-03-24T07:28:23Z

Python/ceval.c

+            }
+            if (!PyUnicode_Check(modname)) {
+                PyErr_Format(PyExc_TypeError,
+                             "__name__ must be a string, not %.100s",


This message can be unclear. What if add "module" before "__name__"?

serhiy-storchaka · 2018-03-24T08:23:44Z

Python/ceval.c

@@ -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",


Wouldn't be better to replace . with a space?

why? I don't get the point.

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.

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)

More revealing error message when non-str objects in __all__

f6e1db7

zhangyangyu added type-feature A feature request or enhancement awaiting review labels Feb 24, 2018

zhangyangyu requested a review from serhiy-storchaka February 24, 2018 13:49

the-knights-who-say-ni added the CLA signed label Feb 24, 2018

bedevere-bot added awaiting merge and removed awaiting review labels Feb 24, 2018

zhangyangyu added 3 commits February 24, 2018 21:51

add NEWS entry

eadbb44

use sys.modules key as name not mod.__name__

7793818

format rst

bdbd0ea

serhiy-storchaka reviewed Feb 24, 2018

View reviewed changes

address Serhiy's comments

f1b8f91

zhangyangyu requested a review from a team February 24, 2018 16:27

serhiy-storchaka reviewed Feb 26, 2018

View reviewed changes

addresses Serhiy's comments again :-)

f07e5dc

serhiy-storchaka reviewed Feb 26, 2018

View reviewed changes

escape variables

c13c7de

brettcannon approved these changes Mar 23, 2018

View reviewed changes

zhangyangyu added 2 commits March 24, 2018 14:34

check modname

da79ff1

Merge branch 'issue32932' of github.com:zhangyangyu/cpython into issu…

90676ae

…e32932

serhiy-storchaka approved these changes Mar 24, 2018

View reviewed changes

more fix

e326e49

serhiy-storchaka reviewed Mar 24, 2018

View reviewed changes

leave dot out

995012e

zhangyangyu merged commit d8b291a into python:master Mar 24, 2018

bedevere-bot removed the awaiting merge label Mar 24, 2018

zhangyangyu deleted the issue32932 branch March 24, 2018 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32932: More revealing error message when non-str objects in all #5848

bpo-32932: More revealing error message when non-str objects in all #5848

zhangyangyu commented Feb 24, 2018 •

edited by bedevere-bot

Loading

serhiy-storchaka Feb 24, 2018

zhangyangyu Feb 24, 2018 •

edited

Loading

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

serhiy-storchaka Feb 26, 2018

zhangyangyu Feb 27, 2018 •

edited

Loading

brettcannon Mar 23, 2018

serhiy-storchaka Mar 23, 2018

brettcannon Mar 23, 2018

serhiy-storchaka Feb 26, 2018

zhangyangyu Feb 27, 2018

zhangyangyu commented Mar 24, 2018

serhiy-storchaka Mar 24, 2018

serhiy-storchaka Mar 24, 2018

serhiy-storchaka Mar 24, 2018

zhangyangyu Mar 24, 2018

serhiy-storchaka Mar 24, 2018

ncoghlan Mar 24, 2018

zhangyangyu Mar 24, 2018

bpo-32932: More revealing error message when non-str objects in __all__ #5848

bpo-32932: More revealing error message when non-str objects in __all__ #5848

Conversation

zhangyangyu commented Feb 24, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

zhangyangyu Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpo-32932: More revealing error message when non-str objects in all #5848

bpo-32932: More revealing error message when non-str objects in all #5848

zhangyangyu commented Feb 24, 2018 •

edited by bedevere-bot

Loading

zhangyangyu Feb 24, 2018 •

edited

Loading

zhangyangyu Feb 27, 2018 •

edited

Loading