Repository: python/cpython
Pull request: gh-106046: Improve error message from `os.fspath` if `__fspath__` is set to `None`
Closes #106046

<!-- gh-issue-number: gh-106046 -->
* Issue: gh-106046
<!-- /gh-issue-number -->


Problem statement:
Improve error message from `os.fspath` if `__fspath__` is set to `None`
# Feature or enhancement

A common Python idiom is to set a magic method to `None` in a subclass if you want to disable the behaviour that the magic method enables. If you do so, a nice error message will be given if a user tries to "call" the magic method that's been set to `None`. For example:

```pycon
>>> class Foo(list):
...     __iter__ = None
...
>>> iter(Foo())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'Foo' object is not iterable
>>> class Bar(int):
...     __hash__ = None
...
>>> hash(Bar(42))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'Bar'
```

However, Python doesn't currently do the same thing for `__fspath__`. If a subclass of an `os.PathLike` sets `__fspath__` to `None`, Python gives a bad error message when `os.fspath` is called on instances of that subclass:

```pycon
>>> from pathlib import Path
>>> import os
>>> class Baz(Path):
...     __fspath__ = None
...
>>> os.fspath(Baz())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NoneType' object is not callable
```

# Pitch

It would be nice if Python handled `__fspath__` being set to `None` the same way it handles magic methods such as `__iter__` or `__hash__` being set to `None`. The error message in such cases should be the `expected str, bytes or os.PathLike object, not Baz` error message that the interpreter gives when `os.fspath` is called on an object that has no `__fspath__` method at all.

<!-- gh-linked-prs -->
### Linked PRs
* gh-106082
<!-- /gh-linked-prs -->

Hints from issue discussion:
It also doesn't work for `__bytes__()`:

```python
>>> class C:
...   __bytes__ = None
... 
>>> bytes(C())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NoneType' object is not callable
```
> It also doesn't work for `__bytes__()`

Yeah. I don't think there's any guarantee or promise that Python will do this for every magic method -- but if we have a use case here, then it _could_ be worth it to implement it for `__fspath__`.

Commit message: gh-106046: Improve error message from `os.fspath` if `__fspath__` is set to `None`

Patch to review:
diff --git a/Lib/os.py b/Lib/os.py
index 31b957f13215d5..d8c9ba4b15400a 100644
--- a/Lib/os.py
+++ b/Lib/os.py
@@ -1061,6 +1061,12 @@ def _fspath(path):
         else:
             raise TypeError("expected str, bytes or os.PathLike object, "
                             "not " + path_type.__name__)
+    except TypeError:
+        if path_type.__fspath__ is None:
+            raise TypeError("expected str, bytes or os.PathLike object, "
+                            "not " + path_type.__name__) from None
+        else:
+            raise
     if isinstance(path_repr, (str, bytes)):
         return path_repr
     else:
diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index 8de4ef7270b754..a29806fae8ab54 100644
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -4647,6 +4647,41 @@ def __fspath__(self):
                 return ''
         self.assertFalse(hasattr(A(), '__dict__'))
 
+    def test_fspath_set_to_None(self):
+        class Foo:
+            __fspath__ = None
+
+        class Bar:
+            def __fspath__(self):
+                return 'bar'
+
+        class Baz(Bar):
+            __fspath__ = None
+
+        good_error_msg = (
+            r"((expected)|(should be)) str, bytes or os.PathLike( object)?, not {}".format
+        )
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Foo")):
+            self.fspath(Foo())
+
+        self.assertEqual(self.fspath(Bar()), 'bar')
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Baz")):
+            self.fspath(Baz())
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Foo")):
+            open(Foo())
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Baz")):
+            open(Baz())
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Foo")):
+            os.rename(Foo(), "foooo")
+
+        with self.assertRaisesRegex(TypeError, good_error_msg("Baz")):
+            os.rename(Baz(), "bazzz")
+
 class TimesTests(unittest.TestCase):
     def test_times(self):
         times = os.times()
diff --git a/Misc/NEWS.d/next/Library/2023-06-23-22-52-24.gh-issue-106046.OdLiLJ.rst b/Misc/NEWS.d/next/Library/2023-06-23-22-52-24.gh-issue-106046.OdLiLJ.rst
new file mode 100644
index 00000000000000..ce10a9d81dc64c
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2023-06-23-22-52-24.gh-issue-106046.OdLiLJ.rst
@@ -0,0 +1,2 @@
+Improve the error message from :func:`os.fspath` if called on an object
+where ``__fspath__`` is set to ``None``. Patch by Alex Waygood.
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 694cff19d2286c..81b5b702f7ce88 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -1197,7 +1197,7 @@ path_converter(PyObject *o, void *p)
         PyObject *func, *res;
 
         func = _PyObject_LookupSpecial(o, &_Py_ID(__fspath__));
-        if (NULL == func) {
+        if ((NULL == func) || (func == Py_None)) {
             goto error_format;
         }
         res = _PyObject_CallNoArgs(func);
@@ -1271,11 +1271,11 @@ path_converter(PyObject *o, void *p)
             path->function_name ? path->function_name : "",
             path->function_name ? ": "                : "",
             path->argument_name ? path->argument_name : "path",
-            path->allow_fd && path->nullable ? "string, bytes, os.PathLike, "
-                                               "integer or None" :
-            path->allow_fd ? "string, bytes, os.PathLike or integer" :
-            path->nullable ? "string, bytes, os.PathLike or None" :
-                             "string, bytes or os.PathLike",
+            path->allow_fd && path->nullable ? "str, bytes, os.PathLike, "
+                                               "int or None" :
+            path->allow_fd ? "str, bytes, os.PathLike or int" :
+            path->nullable ? "str, bytes, os.PathLike or None" :
+                             "str, bytes or os.PathLike",
             _PyType_Name(Py_TYPE(o)));
         goto error_exit;
     }
@@ -15430,7 +15430,7 @@ PyOS_FSPath(PyObject *path)
     }
 
     func = _PyObject_LookupSpecial(path, &_Py_ID(__fspath__));
-    if (NULL == func) {
+    if ((NULL == func) || (func == Py_None)) {
         return PyErr_Format(PyExc_TypeError,
                             "expected str, bytes or os.PathLike object, "
                             "not %.200s",


Code review comments:
diff_hunk: @@ -4647,6 +4647,41 @@ def __fspath__(self):
                 return ''
         self.assertFalse(hasattr(A(), '__dict__'))
 
+    def test_fspath_set_to_None(self):
+        class Foo:
+            __fspath__ = None
+
+        class Bar:
+            def __fspath__(self):
+                return 'bar'
+
+        class Baz(Bar):
+            __fspath__ = None
+
+        good_error_msg = (
+            r"((expected)|(should be)) str, bytes or os.PathLike( object)?, not {}".format
path: Lib/test/test_os.py
text: @user1:
```suggestion
            r"(expected|should be) str, bytes or os.PathLike( object)?, not {}".format
```
Don't need that many parentheses