Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@tomasfarias

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Thanks for your time @tomasfarias, and welcome to CPython! 😎

I assume that you've seen the bot's message about the CLA?

This PR should also have a NEWS entry. Something like:

Update :exc:`TypeError` messages for :meth:`os.path.join` to include :class:`os.PathLike` objects as acceptable input types.

I'd say this falls under the category of enhancement rather than documentation, since it's providing an enhancement to an error message rather than making any documentation changes.

The current uses of genericpath._check_arg_types() are by the .join(), .relpath() and .commonpath() methods of the ntpath and posixpath modules. However, unlike the other methods, both of the .join() methods call _check_arg_types() with the original paths, without first passing them through os.fspath().

That makes the error message of _check_arg_types() potentially inaccurate after this change, since it could potentially be passed os.PathLike objects and raise a very confusing exception.

I suggest adding a check for os.PathLike objects to _check_arg_types() to make it less susceptible to future bugs. With that, the new error message will more certainly be correct. (I wouldn't be worried about performance here, since this is only used in error cases to give more informative exceptions.)

@taleinat

I suggest adding a check for os.PathLike objects to _check_arg_types() to make it less susceptible to future bugs

Would something like this be adequate?

def _check_arg_types(funcname, *args):
    hasstr = hasbytes = haspathlike = False
    for s in args:
        if isinstance(s, str):
            hasstr = True
        elif isinstance(s, bytes):
            hasbytes = True
        elif isinstance(s, os.PathLike):
            haspathlike = True
        else:
            ...

I believe the conditional check below would also be needed, to ensure that os.PathLike instances aren't mixed with bytes:

    if haspathlike and hasbytes:
        raise TypeError("Can't mix os.PathLike and bytes in path components") from None

Currently, if you try to mix os.PathLike path and with a bytes literal, you get a potentially misleading error message:

>>> a = pathlib.PurePath('dir')
>>> b = b'test.py'
>>> os.path.join(a, b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/posixpath.py", line 94, in join
    genericpath._check_arg_types('join', a, *p)
  File "/usr/lib/python3.7/genericpath.py", line 151, in _check_arg_types
    raise TypeError("Can't mix strings and bytes in path components") from None
TypeError: Can't mix strings and bytes in path components

I believe this occurs because in os.path.join(), os.PathLike instances are converted to strings when they are passed to os.fspath().

A check shouldn't be needed for hasstr and haspathlike, because os.PathLike instances can be mixed with strings:

>>> a = pathlib.PurePath('dir')
>>> b = "test.py"
>>> os.path.join(a, b)
'dir/test.py'

Edit: the current error message is even more misleading the over way around:

>>> a = 'dir'
>>> b = pathlib.PurePath('test.py')
>>> os.path.join(a, b)
'dir/test.py'
>>> a = b'dir'
>>> os.path.join(a, b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/posixpath.py", line 94, in join
    genericpath._check_arg_types('join', a, *p)
  File "/usr/lib/python3.7/genericpath.py", line 149, in _check_arg_types
    (funcname, s.__class__.__name__)) from None
TypeError: join() argument must be str or bytes, not 'PurePosixPath'

@aeros, you're right, adding that check like that to _check_arg_types() becomes awkward due to the additional check for mixed strings and bytes. So, to answer:

Would something like this be adequate?

IMO, no.

I suggest changing _check_arg_types() to use for s in map(os.fspath, args):, which should just work.

@taleinat

I suggest changing _check_arg_types() to use for s in map(os.fspath, args):, which should just work.

I agree, that would likely be a better solution. Especially in the long term if any other valid types are added in the future that convert to strings with os.fspath() (that aren't subclasses of os.PathLike).

I've been afk the last couple of days so I'm just now reading the discussion. Thank you all for your comments.

Is there a point to raising the first TypeError in _check_arg_types if we're using for s in map(os.fspath, args)? Since at least in the first example I brought up os.fspath will end up raising the TypeError before _check_arg_types has a chance to run the else block.

I agree with the comment, just wondering if also deleting the else block would be a positive change.

Edit: the exception message from os.fspath mentions os.PathLike objects but doesn't add the function name like _check_arg_types currently does: TypeError: expected str, bytes or os.PathLike object, not NoneType

Actually I think the for s in map(os.fspath, args) solution is not ideal: after testing I realize nothing is handling the exception raised by os.fspath inside _check_arg_types and this leads to printing two tracebacks, which is confusing.

A possible solution is to have join pass the arguments through os.fspath first, like the other functions do: paths = tuple(map(os.fspath, args))

Also thought about using a try/except to wrap the loop in _check_arg_types, but I do not like that idea as it would be duplicating the try/except blocks in join, relpath and commonpath.

@rhettinger: Please replace # with GH- in the commit message next time. Thanks!

Thanks @tomasfarias for the PR, and @rhettinger for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

GH-17249 is a backport of this pull request to the 3.8 branch.

Congrats on your first CPython contribution @tomasfarias! 🍾

Looking forward to seeing more from you in the future.

@rhettinger, did you follow the conversation here? The PR as merged doesn't appear to properly handle edge cases in posixpath.join() and ntpath.join(). If you see otherwise, could you explain?