Update native File.Exists

[JaynieBai]

Fixes #4272

Context

The root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL

Changes Made

Rewrite the WindowsFileSystem.FileExists implementation like this:

#if NETFRAMEWORK
            return Microsoft.IO.File.Exists(path);
#else
            return File.Exists(path);
#endif

Testing

ProjectItemSpecTooLong()

Notes

[AR-May]

@JaynieBai Could you comment on this PR and fill the description?

[JaynieBai]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[f-alizada]

Overall looking good!
One comment left probably nit

[ladipro]

It looks like the root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL. Here it is:

msbuild/src/Framework/NativeMethods.cs

Lines 1725 to 1731 in 053feb0

	[SupportedOSPlatform("windows")]
	internal static bool FileExistsWindows(string fullPath)
	{
	WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
	bool success = GetFileAttributesEx(fullPath, 0, ref data);
	return success && (data.fileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0;
	}

BCL API accepts paths like ..\..\..\..\..\..\..\..\..\..\..\..\..\mydir\myfile while ours does not. I think we should make sure that the fix is done at the right layer (or if it needs to be done at all). What are our guarantees when it comes to such paths? Are there other places where we currently have a similar Windows-only problem? Does the proposed fix of always normalizing paths with GetFullPathNoThrow have a negative perf impact on Linux and Mac where we likely don't need it?

@JaynieBai, can you please check why exactly FileSystems.Default.FileExists(projectPath) is failing without the proposed fix? Is it because the argument is longer than MAX_PATH, because it contains unbalanced ..'s, or because it contains any ..'s at all?

[JaynieBai]

It looks like the root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL. Here it is:

msbuild/src/Framework/NativeMethods.cs

Lines 1725 to 1731 in 053feb0

	[SupportedOSPlatform("windows")]
	internal static bool FileExistsWindows(string fullPath)
	{
	WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
	bool success = GetFileAttributesEx(fullPath, 0, ref data);
	return success && (data.fileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0;
	}

BCL API accepts paths like ..\..\..\..\..\..\..\..\..\..\..\..\..\mydir\myfile while ours does not. I think we should make sure that the fix is done at the right layer (or if it needs to be done at all). What are our guarantees when it comes to such paths? Are there other places where we currently have a similar Windows-only problem? Does the proposed fix of always normalizing paths with GetFullPathNoThrow have a negative perf impact on Linux and Mac where we likely don't need it?

@JaynieBai, can you please check why exactly FileSystems.Default.FileExists(projectPath) is failing without the proposed fix? Is it because the argument is longer than MAX_PATH, because it contains unbalanced ..'s, or because it contains any ..'s at all?

I Find FileSystems.Default.FileExists calls the unmanage function GetFileAttributesEx. So write the following code and find when file is prefixed with different length of "..\". The outputs of File.Exists and GetFileAttributesEx are different. Not sure the reason now. @f-alizada Could you have a look?

static void Main(string[] args)
{
    int[] numbers = new int[] {7, 8, 50, 57, 101, 250 };
    for(int j=0 ; j<numbers.Length; j++)
    {
        string file = null;
        for (int i = 0; i < numbers[j]; i++)
        {
            file += "..\\";
        }
        file += "Users\\file.tmp";
        var test = File.Exists(file);
        Console.WriteLine("FileLength:" + file.Length);
        Console.WriteLine("File.Exists Output:" + test);
        WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
        var result = GetFileAttributesEx(file, 0, ref data);
        Console.WriteLine("FileSystems.Default.FileExists Output: " + result);
        if (!result)
        {
            int error = Marshal.GetLastWin32Error();
            Console.WriteLine($"Error {error} occurred while getting file attributes.");
        }
    }
}

[DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern bool GetFileAttributesEx(String name, int fileInfoLevel, ref WIN32_FILE_ATTRIBUTE_DATA lpFileInformation);

/// <summary>
/// Contains information about a file or directory; used by GetFileAttributesEx.
/// </summary>
[StructLayout(LayoutKind.Sequential)]
public struct WIN32_FILE_ATTRIBUTE_DATA
{
    internal int fileAttributes;
    internal uint ftCreationTimeLow;
    internal uint ftCreationTimeHigh;
    internal uint ftLastAccessTimeLow;
    internal uint ftLastAccessTimeHigh;
    internal uint ftLastWriteTimeLow;
    internal uint ftLastWriteTimeHigh;
    internal uint fileSizeHigh;
    internal uint fileSizeLow;
}

.NET Framework Output

FileLength:35
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:38
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:164
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:185
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:317
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:764
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.

.Net Output

FileLength:35
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:38
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:164
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:185
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:317
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:764
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.

[AR-May]

I agree with @ladipro here that we need to make sure a change happens on a correct abstraction layer and isolated only to the failing scenario, given the perf concerns.

@rainersigwald do you have some knowledge why we have a custom Windows-only implementation of File.Exists here?

[JanKrivanek]

Synced with @AR-May on this - our conclusion: The root of the problem is in our custom implementation of FileExists (FileExistsWindows). The suggested course of action (@JaynieBai):

• Compare performance of standard File.Exists and our custom FileExistsWindows (you can use BenchmarkDotNet for this) - and see if there is a significant difference (report the results here)
• If there are no significant differences - let's replace our custom impl. with the standard impl
• If there is a difference - ping the team - we'll need to have different (more specific) bug created for fixing the FileExistsWindows