File APIs inconsistently handle paths ending in `.` on windows in 3.4.3

[jakemac53]

See this gist https://gist.github.com/jakemac53/845b60847dbda53f93aeed8056d5fbd2.

Assuming the file does exist with that name (I think it is invalid, but it is possible to create at least on the command line), on 3.2.0 and 3.4.0 and greater we do consistently always claim the file does not exist. I am not sure if this in intended behavior or not, but at least it is consistent.

However, starting with at least 3.4.0, the first file copy (using the absolute file URI) succeeds, even though exists() returned false.

I can't easily test this beyond 3.4.3 on my windows box, as I am not set up to do custom builds.

[jakemac53]

Discovered when investigating dart-lang/test#2243.

[lrhn]

Seems to be the absolute path which breaks copy (and copySync) for me. The following shows the same error:

import "dart:io";
void main() {
    var test = File(r'test.');
    try {
      test.copySync(r'copyTest.txt');
      print("Success");
    } catch (e) {
      print("$e");
    }
    test = test.absolute;
    try {
      test.copySync(r'copyTest.txt'); // Fails.
      print("Success");
    } catch (e) {
      print("$e");
    }
}

Also works with .\test. and ..\testdata\test., so it's not any preceding path, only an absolute one, that breaks it.

[aam]

https://dart-review.googlesource.com/c/sdk/+/356680 is the culprit

[aam]

Per https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#win32-file-namespaces on Windows you can't have (trailing) periods in filenames unless you prefix the filename with '\\?\'.
Since we do prefix filenames with '\?' before doing a copy

sdk/runtime/bin/file_win.cc

Line 816 in ca3e67a

const auto old_path = ToWinAPIFilePath(old_name, /*force_long_prefix=*/true);

, that changes meaning of those (trailing) periods.
We could strip trailing periods before adding a prefix

diff --git a/runtime/bin/file_win.cc b/runtime/bin/file_win.cc
index 66cadbedf1b..bf2c0c058e1 100644
--- a/runtime/bin/file_win.cc
+++ b/runtime/bin/file_win.cc
@@ -408,10 +408,14 @@ static std::unique_ptr<wchar_t[]> ToWinAPIPath(const char* utf8_path,
     return absolute_path;
   }

-  // Add prefix and replace forward slashes with backward slashes.
+  // Add prefix, remove trailing periods, replace forward slashes with
+  // backward slashes.
   auto result =
       std::make_unique<wchar_t[]>(kLongPathPrefixLength + path_length + 1);
   wcsncpy(result.get(), kLongPathPrefix, kLongPathPrefixLength);
+  if (is_file) {
+    while (path_length > 0 && absolute_path[path_length - 1] == '.') {
+      --path_length;
+    }
+  }
   for (int i = 0; i < path_length; i++) {
     result.get()[kLongPathPrefixLength + i] =
         absolute_path[i] == L'/' ? L'\\' : absolute_path[i];

cc @mraleph

[mraleph]

@aam I don't think the proposed change is enough. Rules around trailing dots apply to all path components, not just the trailing one. Consider path test.\test. for example.

I think we have a choice to make:

1. We could add better support for path where components include trailing dots - this would require rewriting ToWinAPIFilePath to avoid using GetFullPathNameW which currently eats those dots and then forcing \\?\ prefix if any components contain ..
2. We can silently apply Win32 path normalization rules in ToWinAPIFilePath and strip trailing dots from all path components (so that test.\test. becomes test\test). Maybe just using PathCchCanonicalizeEx would do the trick?
3. We could do a breaking change and make dart:io throw an error when path contains a component with a trailing dot.

I am in doubt that we actually want option 2 - because it silently leads to a very surprising behavior, especially when people are trying to write portable code. I think we should go for either 1 or 3. I am leaning towards 1.

Any opinions?