Skip to content

Commit

Permalink
add error for shadowing
Browse files Browse the repository at this point in the history
  • Loading branch information
@hauntsaninja
hauntsaninja committed Sep 11, 2024
1 parent 6ce3a67 commit 09f6e00
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 72 deletions.
1 change: 1 addition & 0 deletions Include/internal/pycore_moduleobject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ extern void _PyModule_Clear(PyObject *);
extern void _PyModule_ClearDict(PyObject *);
extern int _PyModuleSpec_IsInitializing(PyObject *);
extern int _PyModuleSpec_GetFileOrigin(PyObject *, PyObject **);
extern int _PyModule_IsPossiblyShadowing(PyObject *);

extern int _PyModule_IsExtension(PyObject *obj);

Expand Down
122 changes: 72 additions & 50 deletions Lib/test/test_import/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,49 +786,61 @@ def test_issue105979(self):
str(cm.exception))

def test_script_shadowing_stdlib(self):
with os_helper.temp_dir() as tmp:
with open(os.path.join(tmp, "fractions.py"), "w", encoding='utf-8') as f:
f.write("import fractions\nfractions.Fraction")

expected_error = (
rb"AttributeError: module 'fractions' has no attribute 'Fraction' "
rb"\(consider renaming '.*fractions.py' since it has the "
rb"same name as the standard library module named 'fractions' "
rb"and the import system gives it precedence\)"
script_errors = [
(
"import fractions\nfractions.Fraction",
rb"AttributeError: module 'fractions' has no attribute 'Fraction'"
),
(
"from fractions import Fraction",
rb"ImportError: cannot import name 'Fraction' from 'fractions'"
)
]

popen = script_helper.spawn_python(os.path.join(tmp, "fractions.py"), cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

popen = script_helper.spawn_python('-m', 'fractions', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
for script, error in script_errors:
with os_helper.temp_dir() as tmp:
with open(os.path.join(tmp, "fractions.py"), "w", encoding='utf-8') as f:
f.write(script)

popen = script_helper.spawn_python('-c', 'import fractions', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
expected_error = error + (
rb" \(consider renaming '.*fractions.py' since it has the "
rb"same name as the standard library module named 'fractions' "
rb"and the import system gives it precedence\)"
)

# and there's no error at all when using -P
popen = script_helper.spawn_python('-P', 'fractions.py', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'')
popen = script_helper.spawn_python(os.path.join(tmp, "fractions.py"), cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

tmp_child = os.path.join(tmp, "child")
os.mkdir(tmp_child)
popen = script_helper.spawn_python('-m', 'fractions', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

# test the logic with different cwd
popen = script_helper.spawn_python(os.path.join(tmp, "fractions.py"), cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
popen = script_helper.spawn_python('-c', 'import fractions', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

popen = script_helper.spawn_python('-m', 'fractions', cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'') # no error
# and there's no error at all when using -P
popen = script_helper.spawn_python('-P', 'fractions.py', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'')

popen = script_helper.spawn_python('-c', 'import fractions', cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'') # no error
tmp_child = os.path.join(tmp, "child")
os.mkdir(tmp_child)

# test the logic with different cwd
popen = script_helper.spawn_python(os.path.join(tmp, "fractions.py"), cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

popen = script_helper.spawn_python('-m', 'fractions', cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'') # no error

popen = script_helper.spawn_python('-c', 'import fractions', cwd=tmp_child)
stdout, stderr = popen.communicate()
self.assertEqual(stdout, b'') # no error

def test_package_shadowing_stdlib_module(self):
with os_helper.temp_dir() as tmp:
Expand Down Expand Up @@ -863,27 +875,37 @@ def test_package_shadowing_stdlib_module(self):
self.assertRegex(stdout, b"module 'fractions' has no attribute 'shadowing_module'")

def test_script_shadowing_third_party(self):
script_errors = [
(
"import numpy\nnumpy.array",
rb"AttributeError: module 'numpy' has no attribute 'array'"
),
(
"from numpy import array",
rb"ImportError: cannot import name 'array' from 'numpy'"
)
]
with os_helper.temp_dir() as tmp:
with open(os.path.join(tmp, "numpy.py"), "w", encoding='utf-8') as f:
f.write("import numpy\nnumpy.array")
for script, error in script_errors:
with open(os.path.join(tmp, "numpy.py"), "w", encoding='utf-8') as f:
f.write(script)

expected_error = (
rb"AttributeError: module 'numpy' has no attribute 'array' "
rb"\(consider renaming '.*numpy.py' if it has the "
rb"same name as a third-party module you intended to import\)\s+\Z"
)
expected_error = error + (
rb" \(consider renaming '.*numpy.py' if it has the "
rb"same name as a third-party module you intended to import\)\s+\Z"
)

popen = script_helper.spawn_python(os.path.join(tmp, "numpy.py"))
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
popen = script_helper.spawn_python(os.path.join(tmp, "numpy.py"))
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

popen = script_helper.spawn_python('-m', 'numpy', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
popen = script_helper.spawn_python('-m', 'numpy', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

popen = script_helper.spawn_python('-c', 'import numpy', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)
popen = script_helper.spawn_python('-c', 'import numpy', cwd=tmp)
stdout, stderr = popen.communicate()
self.assertRegex(stdout, expected_error)

def test_script_maybe_not_shadowing_third_party(self):
with os_helper.temp_dir() as tmp:
Expand Down
14 changes: 9 additions & 5 deletions Objects/moduleobject.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,8 @@ _PyModuleSpec_GetFileOrigin(PyObject *spec, PyObject **p_origin)
return 1;
}

static int
_is_module_possibly_shadowing(PyObject *origin)
int
_PyModule_IsPossiblyShadowing(PyObject *origin)
{
// origin must be a unicode subtype
// Returns 1 if the module at origin could be shadowing a module of the
Expand Down Expand Up @@ -985,7 +985,7 @@ _Py_module_getattro_impl(PyModuleObject *m, PyObject *name, int suppress)
goto done;
}

int is_possibly_shadowing = _is_module_possibly_shadowing(origin);
int is_possibly_shadowing = _PyModule_IsPossiblyShadowing(origin);
if (is_possibly_shadowing < 0) {
goto done;
}
Expand All @@ -1011,7 +1011,10 @@ _Py_module_getattro_impl(PyModuleObject *m, PyObject *name, int suppress)
}
else {
int rc = _PyModuleSpec_IsInitializing(spec);
if (rc > 0) {
if (rc < 0) {
goto done;
}
else if (rc > 0) {
if (is_possibly_shadowing) {
assert(origin);
// For third-party modules, only mention the possibility of
Expand All @@ -1037,7 +1040,8 @@ _Py_module_getattro_impl(PyModuleObject *m, PyObject *name, int suppress)
mod_name, name);
}
}
else if (rc == 0) {
else {
assert(rc == 0);
rc = _PyModuleSpec_IsUninitializedSubmodule(spec, name);
if (rc > 0) {
PyErr_Format(PyExc_AttributeError,
Expand Down
88 changes: 71 additions & 17 deletions Python/ceval.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2801,6 +2801,7 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name)
mod_name_or_unknown = mod_name;
}
// mod_name is no longer an owned reference
assert(mod_name_or_unknown);
assert(mod_name == NULL || mod_name == mod_name_or_unknown);

origin = NULL;
Expand All @@ -2818,27 +2819,80 @@ _PyEval_ImportFrom(PyThreadState *tstate, PyObject *v, PyObject *name)
if (_PyModuleSpec_GetFileOrigin(spec, &origin) < 0) {
goto done;
}
if (origin == NULL) {

int is_possibly_shadowing = _PyModule_IsPossiblyShadowing(origin);
if (is_possibly_shadowing < 0) {
goto done;
}
int is_possibly_shadowing_stdlib = 0;
if (is_possibly_shadowing) {
PyObject *stdlib_modules = PySys_GetObject("stdlib_module_names");
if (stdlib_modules && PyAnySet_Check(stdlib_modules)) {
is_possibly_shadowing_stdlib = PySet_Contains(stdlib_modules, mod_name_or_unknown);
if (is_possibly_shadowing_stdlib < 0) {
goto done;
}
}
}

if (is_possibly_shadowing_stdlib) {
assert(origin);
errmsg = PyUnicode_FromFormat(
"cannot import name %R from %R (unknown location)",
name, mod_name_or_unknown
"cannot import name %R from %R "
"(consider renaming %R since it has the same "
"name as the standard library module named %R "
"and the import system gives it precedence)",
name, mod_name_or_unknown, origin, mod_name_or_unknown
);
goto done_with_errmsg;
}

int rc = _PyModuleSpec_IsInitializing(spec);
if (rc < 0) {
Py_DECREF(mod_name_or_unknown);
Py_DECREF(origin);
return NULL;
else {
int rc = _PyModuleSpec_IsInitializing(spec);
if (rc < 0) {
goto done;
}
else if (rc > 0) {
if (is_possibly_shadowing) {
assert(origin);
// For third-party modules, only mention the possibility of
// shadowing if the module is being initialized.
errmsg = PyUnicode_FromFormat(
"cannot import name %R from %R "
"(consider renaming %R if it has the same name "
"as a third-party module you intended to import)",
name, mod_name_or_unknown, origin
);
}
else if (origin) {
errmsg = PyUnicode_FromFormat(
"cannot import name %R from partially initialized module %R "
"(most likely due to a circular import) (%S)",
name, mod_name_or_unknown, origin
);
}
else {
errmsg = PyUnicode_FromFormat(
"cannot import name %R from partially initialized module %R "
"(most likely due to a circular import)",
name, mod_name_or_unknown
);
}
}
else {
assert(rc == 0);
if (origin) {
errmsg = PyUnicode_FromFormat(
"cannot import name %R from %R (%S)",
name, mod_name_or_unknown, origin
);
}
else {
errmsg = PyUnicode_FromFormat(
"cannot import name %R from %R (unknown location)",
name, mod_name_or_unknown
);
}
}
}
const char *fmt =
rc ?
"cannot import name %R from partially initialized module %R "
"(most likely due to a circular import) (%S)" :
"cannot import name %R from %R (%S)";

errmsg = PyUnicode_FromFormat(fmt, name, mod_name_or_unknown, origin);

done_with_errmsg:
/* NULL checks for errmsg and mod_name done by PyErr_SetImportError. */
Expand Down

0 comments on commit 09f6e00

Please sign in to comment.