gh-102931: Use explicit exception when shutil._copytree should group exceptions by berland · Pull Request #128309 · python/cpython

berland · 2024-12-28T12:18:16Z

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 · 2024-12-28T12:18:19Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-12-28T12:18:22Z

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 · 2024-12-28T12:21:46Z

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

eric-wieser · 2024-12-28T22:29:54Z

                copy_function(srcobj, dstname)
-        # catch the Error from the recursive copytree so that we can
-        # continue with other files
+        except SameFileError as err:


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

(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.

I will try to reproduce that with a test.

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 · 2024-12-29T16:47:36Z

+        except SameFileError as err:
+            errors.append((srcname, dstname, str(err)))
        except Error as err:
            errors.extend(err.args[0])


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

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.

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.

Thanks, that looked good in the code! Fixed

eric-wieser · 2025-01-02T12:16:57Z

        except Error as err:
-            errors.extend(err.args[0])
+            errors.append((srcname, dstname, str(err)))


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

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 · 2025-10-14T13:18:31Z

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

python-cla-bot · 2025-10-14T13:18:57Z

All commit authors signed the Contributor License Agreement.

picnixz · 2026-01-10T19:03:26Z

+        try:
+            shutil.copytree(src_dir, target_dir, dirs_exist_ok=True)
+            self.fail("shutil.Error should have been raised")
+        except Error as error:


Use self.assertRaisesRegex instead of try-except please or work with the context manager.

I don't think self.assertRaisesRegex is appropriate here, since we're checking a tuple-valued args.

Oh right. Then just assertRaises

picnixz · 2026-01-10T19:03:49Z

    pass

+class ErrorGroup(Error):
+    """Raised when multiple exceptions have been caught"""


Suggested change

"""Raised when multiple exceptions have been caught"""

"""Raised when multiple exceptions have been caught."""

Thanks for your review @picnixz , this should now be fixed. Will squash the commits later, but kept the review-fix as separate commit for now.

Don't bother squashing them. We do it ourselves at the end. Squashing commits is not always good since we do incremental reviews. Only force-pushes when (1) there is no review yet (2) there was a real issue with the history

This resolves a bug where a non-recursive error is assumed to be a list of exceptions and presenting the error as: ``` shutil.Error: ['<', 'D', 'i', 'r', 'E', 'n', 't', 'r', 'y', ' ', ..., 's', 'a', 'm', 'e', ' ', 'f', 'i', 'l', 'e'] ```

github-actions · 2026-04-20T12:23:11Z

This PR is stale because it has been open for 30 days with no activity.

bedevere-app Bot mentioned this pull request Dec 28, 2024

SMTP #102391

Closed

bedevere-app Bot added the awaiting review label Dec 28, 2024

berland changed the title ~~gh-102391: Catch SameFileError explicitly in _copytree~~ gh-102931: Catch SameFileError explicitly in _copytree Dec 28, 2024

bedevere-app Bot mentioned this pull request Dec 28, 2024

shutil.copytree emits nonsensical error message for symlinks #102931

Open

ZeroIntensity self-requested a review December 28, 2024 14:22

eric-wieser reviewed Dec 28, 2024

View reviewed changes

ZeroIntensity reviewed Dec 29, 2024

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2024-12-28-16-25-42.gh-issue-102931.55o5kb.rst Outdated

Comment thread Lib/test/test_shutil.py Outdated

Comment thread Lib/test/test_shutil.py

Comment thread Lib/shutil.py Outdated

berland force-pushed the fix_copy_itself_exc_error branch from b8dc15b to 470b7bd Compare December 29, 2024 15:49

eric-wieser reviewed Dec 29, 2024

View reviewed changes

berland changed the title ~~gh-102931: Catch SameFileError explicitly in _copytree~~ gh-102931: Use explicit exception when _copytree should group exceptions Jan 2, 2025

berland changed the title ~~gh-102931: Use explicit exception when _copytree should group exceptions~~ gh-102931: Use explicit exception when shutil._copytree should group exceptions Jan 2, 2025

berland force-pushed the fix_copy_itself_exc_error branch 2 times, most recently from 6e9603b to 101bf86 Compare January 2, 2025 06:27

eric-wieser reviewed Jan 2, 2025

View reviewed changes

eric-wieser approved these changes Jan 3, 2025

View reviewed changes

bedevere-app Bot added awaiting core review and removed awaiting review labels Jan 3, 2025

berland force-pushed the fix_copy_itself_exc_error branch from baff88d to 533be29 Compare January 3, 2025 06:15

berland requested a review from giampaolo as a code owner October 14, 2025 13:18

picnixz reviewed Jan 10, 2026

View reviewed changes

berland added 2 commits January 12, 2026 12:16

Address review comments from picnixz

da49c09

berland force-pushed the fix_copy_itself_exc_error branch from 3d03a5f to da49c09 Compare January 12, 2026 11:16

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 20, 2026

	"""Raised when multiple exceptions have been caught"""
	"""Raised when multiple exceptions have been caught."""

Uh oh!

Conversation

berland commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 28, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Dec 28, 2024

Uh oh!

berland commented Dec 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berland Dec 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

berland commented Oct 14, 2025

Uh oh!

python-cla-bot Bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

berland commented Dec 28, 2024 •

edited

Loading

ghost commented Dec 28, 2024 •

edited by ghost

Loading

berland Dec 29, 2024 •

edited

Loading

eric-wieser Jan 2, 2025 •

edited

Loading

python-cla-bot Bot commented Oct 14, 2025 •

edited

Loading

picnixz Jan 10, 2026 •

edited

Loading