From 363567c37d0c85d8db214c090797305cc2cef100 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Thu, 5 Dec 2024 16:41:39 -0800 Subject: [PATCH 01/15] gh-127651: Use __file__ in diagnostics if origin is missing See the left hand side in https://github.com/python/cpython/pull/123929/files#diff-c22186367cbe20233e843261998dc027ae5f1f8c0d2e778abfa454ae74cc59deL2840-L2849 I missed this in part because I use debug builds for dev which I think don't freeze modules --- Lib/test/test_import/__init__.py | 8 ++++++++ Python/ceval.c | 13 +++++++++++++ 2 files changed, 21 insertions(+) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index c52b7f3e09bea1..9053b04d128833 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -807,6 +807,14 @@ def test_issue105979(self): self.assertIn("Frozen object named 'x' is invalid", str(cm.exception)) + def test_frozen_module_from_import_error(self): + with self.assertRaises(ImportError) as cm: + from os import this_will_never_exist + self.assertRegex( + str(cm.exception), + r"cannot import name 'this_will_never_exist' from 'os' \(.*Lib[\\/]os\.py\)" + ) + def test_script_shadowing_stdlib(self): script_errors = [ ( diff --git a/Python/ceval.c b/Python/ceval.c index 6795a160506231..c505f110a5d80b 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2903,6 +2903,19 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name) } else { assert(rc == 0); + if (origin == NULL) { + // Fall back to __file__ for diagnostics if we don't have + // an origin that is a location + origin = PyModule_GetFilenameObject(v); + if (origin == NULL) { + if (!PyErr_ExceptionMatches(PyExc_SystemError)) { + goto done; + } + // PyModule_GetFilenameObject raised "module filename missing" + _PyErr_Clear(tstate); + } + assert(origin == NULL || PyUnicode_Check(origin)); + } if (origin) { errmsg = PyUnicode_FromFormat( "cannot import name %R from %R (%S)", From 69be54a6a372503055183a6b7d5f0004f64d4d72 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 6 Dec 2024 01:09:47 +0000 Subject: [PATCH 02/15] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst new file mode 100644 index 00000000000000..b6e2a66bc4c532 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst @@ -0,0 +1 @@ +When raising ImportError for missing symbols in ``from`` imports, use ``__file__`` in the error message if `__spec__.origin` is not a location From e5bab4c2861edae5a69edb81d5ab830f928b19f9 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Thu, 5 Dec 2024 17:21:52 -0800 Subject: [PATCH 03/15] fix news entry --- .../2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst index b6e2a66bc4c532..92b18b082eb30e 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-06-01-09-40.gh-issue-127651.80cm6j.rst @@ -1 +1 @@ -When raising ImportError for missing symbols in ``from`` imports, use ``__file__`` in the error message if `__spec__.origin` is not a location +When raising :exc:`ImportError` for missing symbols in ``from`` imports, use ``__file__`` in the error message if ``__spec__.origin`` is not a location From 0688e77cd86a5d73f29fd50e14fe62095bf23654 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Sun, 8 Dec 2024 23:29:23 -0800 Subject: [PATCH 04/15] use os.__file__ in test --- Lib/test/test_import/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 9053b04d128833..8b775a2c73f648 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -15,6 +15,7 @@ import os import py_compile import random +import re import shutil import stat import subprocess @@ -812,7 +813,7 @@ def test_frozen_module_from_import_error(self): from os import this_will_never_exist self.assertRegex( str(cm.exception), - r"cannot import name 'this_will_never_exist' from 'os' \(.*Lib[\\/]os\.py\)" + f"cannot import name 'this_will_never_exist' from 'os' \\({re.escape(os.__file__)}\\)" ) def test_script_shadowing_stdlib(self): From 7c30a5d760a167d62dbcd8b708db551f4ecadad0 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Sun, 8 Dec 2024 23:37:42 -0800 Subject: [PATCH 05/15] add test for error path --- Lib/test/test_import/__init__.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 8b775a2c73f648..75472de6de565a 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -816,6 +816,23 @@ def test_frozen_module_from_import_error(self): f"cannot import name 'this_will_never_exist' from 'os' \\({re.escape(os.__file__)}\\)" ) + with os_helper.temp_dir() as tmp: + with open(os.path.join(tmp, "main.py"), "w", encoding='utf-8') as f: + f.write(""" +import os +os.__file__ = 123 +os.__spec__.has_location = False +from os import this_will_never_exist +""") + + expected_error = ( + b"cannot import name 'this_will_never_exist' " + b"from 'os' \\(unknown location\\)" + ) + popen = script_helper.spawn_python(os.path.join(tmp, "main.py"), cwd=tmp) + stdout, stderr = popen.communicate() + self.assertRegex(stdout, expected_error) + def test_script_shadowing_stdlib(self): script_errors = [ ( From 9a3dbbfadc188ce4213e07a32f1848dc1a4e69de Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Sun, 8 Dec 2024 23:49:06 -0800 Subject: [PATCH 06/15] add test for missing __file__ --- Lib/test/test_import/__init__.py | 33 +++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 75472de6de565a..d532b506823f41 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -816,22 +816,33 @@ def test_frozen_module_from_import_error(self): f"cannot import name 'this_will_never_exist' from 'os' \\({re.escape(os.__file__)}\\)" ) - with os_helper.temp_dir() as tmp: - with open(os.path.join(tmp, "main.py"), "w", encoding='utf-8') as f: - f.write(""" + + scripts = [ + """ import os +os.__spec__.has_location = False os.__file__ = 123 +from os import this_will_never_exist +""", + """ +import os os.__spec__.has_location = False +del os.__file__ from os import this_will_never_exist -""") +""" + ] + for script in scripts: + with self.subTest(script=script), os_helper.temp_dir() as tmp: + with open(os.path.join(tmp, "main.py"), "w", encoding='utf-8') as f: + f.write(script) - expected_error = ( - b"cannot import name 'this_will_never_exist' " - b"from 'os' \\(unknown location\\)" - ) - popen = script_helper.spawn_python(os.path.join(tmp, "main.py"), cwd=tmp) - stdout, stderr = popen.communicate() - self.assertRegex(stdout, expected_error) + expected_error = ( + b"cannot import name 'this_will_never_exist' " + b"from 'os' \\(unknown location\\)" + ) + popen = script_helper.spawn_python(os.path.join(tmp, "main.py"), cwd=tmp) + stdout, stderr = popen.communicate() + self.assertRegex(stdout, expected_error) def test_script_shadowing_stdlib(self): script_errors = [ From 33e3fb6ffa0605c04bf52f6d4c36958293930fcd Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 01:21:22 -0800 Subject: [PATCH 07/15] use mortal objects --- Lib/test/test_import/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index d532b506823f41..6ac1a59e0c0afe 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -821,7 +821,7 @@ def test_frozen_module_from_import_error(self): """ import os os.__spec__.has_location = False -os.__file__ = 123 +os.__file__ = [] from os import this_will_never_exist """, """ @@ -1105,7 +1105,7 @@ class substr(str): except AttributeError as e: print(str(e)) -fractions.__spec__.origin = 0 +fractions.__spec__.origin = [] try: fractions.Fraction except AttributeError as e: @@ -1129,7 +1129,7 @@ class substr(str): except ImportError as e: print(str(e)) -fractions.__spec__.origin = 0 +fractions.__spec__.origin = [] try: from fractions import Fraction except ImportError as e: From fb1d3819d2094002f1e47f977c07c8db9e1c2411 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 01:29:39 -0800 Subject: [PATCH 08/15] remove regex use --- Lib/test/test_import/__init__.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 6ac1a59e0c0afe..64054e56df0e2c 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -15,7 +15,6 @@ import os import py_compile import random -import re import shutil import stat import subprocess @@ -811,9 +810,9 @@ def test_issue105979(self): def test_frozen_module_from_import_error(self): with self.assertRaises(ImportError) as cm: from os import this_will_never_exist - self.assertRegex( + self.assertIn( + f"cannot import name 'this_will_never_exist' from 'os' ({os.__file__})", str(cm.exception), - f"cannot import name 'this_will_never_exist' from 'os' \\({re.escape(os.__file__)}\\)" ) From 247565e3031cd3938d43bac118592a0929903d02 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 01:31:18 -0800 Subject: [PATCH 09/15] remove tmp dir --- Lib/test/test_import/__init__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 64054e56df0e2c..c6aab21022c049 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -831,15 +831,12 @@ def test_frozen_module_from_import_error(self): """ ] for script in scripts: - with self.subTest(script=script), os_helper.temp_dir() as tmp: - with open(os.path.join(tmp, "main.py"), "w", encoding='utf-8') as f: - f.write(script) - + with self.subTest(script=script): expected_error = ( b"cannot import name 'this_will_never_exist' " b"from 'os' \\(unknown location\\)" ) - popen = script_helper.spawn_python(os.path.join(tmp, "main.py"), cwd=tmp) + popen = script_helper.spawn_python("-c", script) stdout, stderr = popen.communicate() self.assertRegex(stdout, expected_error) From 2bd25a8162705ffd6d4676d12c1e73c5c9f07203 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 01:49:49 -0800 Subject: [PATCH 10/15] move call earlier --- Python/ceval.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index c505f110a5d80b..f42e4fc3394bd9 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2859,6 +2859,20 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name) } } + if (origin == NULL) { + // Fall back to __file__ for diagnostics if we don't have + // an origin that is a location + origin = PyModule_GetFilenameObject(v); + if (origin == NULL) { + if (!PyErr_ExceptionMatches(PyExc_SystemError)) { + goto done; + } + // PyModule_GetFilenameObject raised "module filename missing" + _PyErr_Clear(tstate); + } + assert(origin == NULL || PyUnicode_Check(origin)); + } + if (is_possibly_shadowing_stdlib) { assert(origin); errmsg = PyUnicode_FromFormat( @@ -2903,19 +2917,6 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name) } else { assert(rc == 0); - if (origin == NULL) { - // Fall back to __file__ for diagnostics if we don't have - // an origin that is a location - origin = PyModule_GetFilenameObject(v); - if (origin == NULL) { - if (!PyErr_ExceptionMatches(PyExc_SystemError)) { - goto done; - } - // PyModule_GetFilenameObject raised "module filename missing" - _PyErr_Clear(tstate); - } - assert(origin == NULL || PyUnicode_Check(origin)); - } if (origin) { errmsg = PyUnicode_FromFormat( "cannot import name %R from %R (%S)", From 8a5f76ad9d2ea8d2ddcae4347ad4c657a730515c Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 01:54:00 -0800 Subject: [PATCH 11/15] test with sys --- Lib/test/test_import/__init__.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index c6aab21022c049..cb429a016839a1 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -815,6 +815,14 @@ def test_frozen_module_from_import_error(self): str(cm.exception), ) + expected_error = ( + b"cannot import name 'this_will_never_exist' " + b"from 'sys' (unknown location)" + ) + popen = script_helper.spawn_python("-c", "from sys import this_will_never_exist") + stdout, stderr = popen.communicate() + self.assertIn(expected_error, stdout) + scripts = [ """ @@ -834,11 +842,11 @@ def test_frozen_module_from_import_error(self): with self.subTest(script=script): expected_error = ( b"cannot import name 'this_will_never_exist' " - b"from 'os' \\(unknown location\\)" + b"from 'os' (unknown location)" ) popen = script_helper.spawn_python("-c", script) stdout, stderr = popen.communicate() - self.assertRegex(stdout, expected_error) + self.assertIn(expected_error, stdout) def test_script_shadowing_stdlib(self): script_errors = [ From c80120ec1d518faf2e2df7b3b863ffa8a5d57a15 Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 02:01:48 -0800 Subject: [PATCH 12/15] errmsg null check --- Python/ceval.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index f42e4fc3394bd9..fabdb60507ce2f 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2933,9 +2933,11 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name) } done_with_errmsg: - /* NULL checks for errmsg, mod_name, origin done by PyErr_SetImportError. */ - _PyErr_SetImportErrorWithNameFrom(errmsg, mod_name, origin, name); - Py_DECREF(errmsg); + if (errmsg != NULL) { + /* NULL checks for errmsg, mod_name, origin done by _PyErr_SetImportErrorWithNameFrom */ + _PyErr_SetImportErrorWithNameFrom(errmsg, mod_name, origin, name); + Py_DECREF(errmsg); + } done: Py_XDECREF(origin); From e8da759597909e647ca04c4b321674461a6b5baa Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 02:22:25 -0800 Subject: [PATCH 13/15] improve test --- Lib/test/test_import/__init__.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index cb429a016839a1..38188729869667 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -814,15 +814,12 @@ def test_frozen_module_from_import_error(self): f"cannot import name 'this_will_never_exist' from 'os' ({os.__file__})", str(cm.exception), ) - - expected_error = ( - b"cannot import name 'this_will_never_exist' " - b"from 'sys' (unknown location)" + with self.assertRaises(ImportError) as cm: + from sys import this_will_never_exist + self.assertIn( + "cannot import name 'this_will_never_exist' from 'sys' (unknown location)", + str(cm.exception), ) - popen = script_helper.spawn_python("-c", "from sys import this_will_never_exist") - stdout, stderr = popen.communicate() - self.assertIn(expected_error, stdout) - scripts = [ """ From d5aa69cfd33ecd650ee67d4514403930b0c5d511 Mon Sep 17 00:00:00 2001 From: Shantanu <12621235+hauntsaninja@users.noreply.github.com> Date: Mon, 9 Dec 2024 02:23:05 -0800 Subject: [PATCH 14/15] Update Python/ceval.c Co-authored-by: Serhiy Storchaka --- Python/ceval.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/ceval.c b/Python/ceval.c index fabdb60507ce2f..5eda033eced628 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2934,7 +2934,7 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name) done_with_errmsg: if (errmsg != NULL) { - /* NULL checks for errmsg, mod_name, origin done by _PyErr_SetImportErrorWithNameFrom */ + /* NULL checks for mod_name and origin done by _PyErr_SetImportErrorWithNameFrom */ _PyErr_SetImportErrorWithNameFrom(errmsg, mod_name, origin, name); Py_DECREF(errmsg); } From 560a23c7082e2cbeb2ab0f51dbc155a0a126670c Mon Sep 17 00:00:00 2001 From: Shantanu Jain Date: Mon, 9 Dec 2024 02:27:20 -0800 Subject: [PATCH 15/15] more origin test --- Lib/test/test_import/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 38188729869667..83efbc1e25e77a 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -833,6 +833,12 @@ def test_frozen_module_from_import_error(self): os.__spec__.has_location = False del os.__file__ from os import this_will_never_exist +""", + """ +import os +os.__spec__.origin = [] +os.__file__ = [] +from os import this_will_never_exist """ ] for script in scripts: