gh-102931: Use explicit exception when shutil._copytree should group exceptions

[berland]

If not, it is caught as a shutil.Error for which it is assumed to be a recursive error, resulting in splitting of the error string as

shutil.Error: ['<', 'D', 'i', 'r', 'E', 'n', 't', 'r', 'y', ' ', ...,
               's', 'a', 'm', 'e', ' ', 'f', 'i', 'l', 'e']

• Issue: shutil.copytree emits nonsensical error message for symlinks #102931

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[berland]

This is based on @eric-wieser's work in #102827 but with tests adde and a different approach to resolution.

[eric-wieser]

I think you need the same handler for SpecialFileError, which is also instantiated with a string.

[eric-wieser]

(and possibly the other errors too, I haven't attempted to trace them all).

Using ExceptionGroup here might make things cleaner, but is probably backwards-incompatible.

[berland]

I will try to reproduce that with a test.

[berland]

I could not make a scenario that would yield this error (a splitted string in the error message) when a SpecialFileError is triggered. There is already a test function that checks the error string when a SpecialFileError is provoked with a pipe

cpython/Lib/test/test_shutil.py

Line 999 in ffece55

def test_copytree_named_pipe(self):

which already passes and that behaviour is not changed in this PR. In that case there is recursion involved, and then the existing code works. The bug with splitted strings only occurs when there is no recursion.

[ZeroIntensity]

Some high-level comments.

[eric-wieser]

Would it instead be better to use:

if type(e) is Error:
  errors.extend(err.args[0])
else:
  errors.append((srcname, dstname, str(err)))

which perhaps is more robust

[berland]

Yes, it is more robust if there are more bugs that we have not found. It is best if we could find them too and add a test case for them.

If it is impossible to trigger other exceptions in the outermost recursion layer, I would prefer the code to be explicit and only catch what is attainable.

[eric-wieser]

I would prefer the code to be explicit and only catch what is attainable.

Agreed; perhaps then we should add

class ErrorGroup(Error): pass

and throw/catch this when grouping the exceptions. That way it is explicit that this merging behavior is actually going to do the right thinking, rather than hoping the exception is the one we think it is.

[berland]

Thanks, that looked good in the code! Fixed

[eric-wieser]

Error extends OSError which is already caught below, so there is no need to catch this any more.

[berland]

Yup. Fixed!

[eric-wieser]

Thanks! It would be cool to use ExceptionGroup here, but that would probably be backwards incompatible so probably isn't possible.

[berland]

@eric-wieser is there anything I can do to make this enter "core review"?

[python-cla-bot]

All commit authors signed the Contributor License Agreement.