Directory.rename should not delete the target if it exists

[brianquinlan]

Change

Currently Directory.rename() has two undesirable properties on Windows:

1. if the target path is an existing directory then it is deleted
2. if the target path matches the current path, then the directory is deleted

I propose that we remove both of those behaviors i.e. it is an no-op if the src and target paths are identical and it is an error if the target path is an existing directory.

See https://dart-review.googlesource.com/c/sdk/+/219501

Rationale

This change will bring the semantics of Directory.rename() on Windows inline with the semantics on every other platform - except that it will be an error to rename to a existing empty directory, which is allowed on POSIX systems.

This is inline with the behavior offered in other programming languages:
https://en.cppreference.com/w/cpp/filesystem/rename
https://docs.python.org/3/library/os.html#os.rename
https://docs.oracle.com/javase/7/docs/api/java/io/File.html#renameTo(java.io.File)
https://doc.rust-lang.org/std/fs/fn.rename.html

Impact

It is possible (but not likely IMO) that some dart Windows-only programs rely on this behavior but it is more likely that existing programs have a large unintended footgun and this will remove it.

Mitigation

Users can first remove the target directory before rename i.e.

Directory(target).deleteSync()
Directory(src).renameSync(target)

[a-siva]

/cc @mit-mit @devoncarew

[devoncarew]

Thanks for filing the breaking change request; I'll track down approvals (as per https://github.com/dart-lang/sdk/blob/master/docs/process/breaking-changes.md#step-2-approval).

[devoncarew]

FWIW, this seems like a reasonable change to me.

[brianquinlan]

Hey @devoncarew , you'll comment again on this issue when I have approval, right?

[devoncarew]

Thanks, yup. I'm still running down some details of our breaking change process.

I think we'll want approval from AngularDart (and google3 impact), Flutter, and Dart generally. @grouma, @Hixie, and @vsmenon - can you review and approve? I suspect the impact of the change will be relatively low, and will be specific to Windows. Thanks!

[mit-mit]

It looks like we have the same 'overwrite' behaviour for file renames?
https://api.flutter.dev/flutter/dart-io/File/File.html

Do we want to change them both?

And do we want to consider still overwriting, perhaps via a extra optional parameter, e.g.:
.rename(String newFileName, {bool replace = false}) ?

[brianquinlan]

Hey @mit-mit ,

1. File.rename() has consistent behavior across platforms (AFAIC) and that behavior matches the documentation i.e. if the target path is a file, it is deleted, if the target path is a directory then the rename fails. The further that we move from the semantics offered by POSIX/Windows, the more difficult it will be to have a correct implementation. This also matches what other programming languages do e.g. C++, Python.

2. We can't add new arguments to rename without breaking subclasses.

[grouma]

This should have no impact on AngularDart since it requires dart:io which is incompatible. Note however that AngularDart customers may use this API for backend scripts but that use case should be covered by @vsmenon's analysis.

[vsmenon]

lgtm

[brianquinlan]

So am I good to go?

[devoncarew]

We should get feedback from @Hixie (though I don't suspect this will impact Flutter apps).

[Hixie]

This change seems positive. In general, the less we silently delete, the better. Also, the more these I/O operations can approximate an atomic operation, the more reliable they will be in general.

[devoncarew]

Thanks all! @brianquinlan, I think you can consider this change approved. From https://github.com/dart-lang/sdk/blob/master/docs/process/breaking-changes.md, you want to send an announce email to announce@dartlang.org (I know you had some trouble sending to that list; I'm happy to send one if you get a bounce). After some time for comments you'll want to work through the 'Execution' steps. Thanks!

[brianquinlan]

I've made the announcement and added an update to the change log to my CL. I'll wait until mid next week to submit.

[brianquinlan]

Fixed in 0e5f3f4