Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move File.Exists and File.ReadAllBytes to Common #53168

Closed
wants to merge 4 commits into from

Conversation

adamsitnik
Copy link
Member

I wanted to start working on File.OpenHandle which is part of #24847 (comment)

and then I realized that we have a copy of File.Exists and File.ReadAllBytes in System.Private.CoreLib which was promised to keep in sync with System.File.IO:

namespace Internal.IO
{
//
// Subsetted clone of System.IO.File for internal runtime use.
// Keep in sync with https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.FileSystem.
//
internal static partial class File

The problem is that it was not kept in sync. The differences:

I moved it to Common so we have a single implementation and no code duplication now.

@adamsitnik adamsitnik added this to the 6.0.0 milestone May 24, 2021
@ghost
Copy link

ghost commented May 24, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

I wanted to start working on File.OpenHandle which is part of #24847 (comment)

and then I realized that we have a copy of File.Exists and File.ReadAllBytes in System.Private.CoreLib which was promised to keep in sync with System.File.IO:

namespace Internal.IO
{
//
// Subsetted clone of System.IO.File for internal runtime use.
// Keep in sync with https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.FileSystem.
//
internal static partial class File

The problem is that it was not kept in sync. The differences:

I moved it to Common so we have a single implementation and no code duplication now.

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.0

@jkotas
Copy link
Member

jkotas commented May 24, 2021

lack of TrimEndingDirectorySeparator for File.Exists
lack of support for file systems that don't report file length for non-empty files for File.ReadAllBytes

CoreLib uses these two APIs for specific purpose where we do not need any of this. You can add a comment to CoreLib copy that this is missing instead of increasing the amount of duplicated code.

Instead of this, it may be better to fold System.IO.FileSystem into CoreLib (contributes to #2138). @marek-safar asked me about it a while ago. System.IO.FileSystem has a dependency on Linq that would need to be cleaned up before it can be done.

@jkotas
Copy link
Member

jkotas commented May 24, 2021

I moved it to Common so we have a single implementation and no code duplication now.

We still have the code duplicated in binaries, and more of it than before.

@adamsitnik
Copy link
Member Author

@jkotas I like the idea of folding System.IO.FileSystem into System.Private.CoreLib. It would definitely make my life easier. I am going to give it a try.

@adamsitnik adamsitnik closed this May 24, 2021
@carlossanlop
Copy link
Member

@jkotas Why move the whole FileSystem namespace to CoreLib? If Linq has dependencies to FileSystem types, can't we instead move only the types that are needed? For example, FileStream and Path live in Corelib and are typeforwarded to FileSystem.

I'm interested in knowing the reasoning to move the whole namespace. cc @ericstj

@jkotas
Copy link
Member

jkotas commented May 24, 2021

I think that the file and directory enumeration APIs that exist in System.IO.FileSystem.dll really belong to CoreLib.

Note that we have a parallel implementation of file and directory enumeration APIs in CoreCLR PAL (https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/file/find.cpp). We are not able to do C# rewrite of the logic that depends on these since the managed file and directory enumeration APIs do not live in CoreLib. Once we move the the managed file and directory enumeration APIs to CoreLib, we will be one stop closer to be able to rewrite a bunch more runtime code in C#.

Linq

Use of Linq in System.IO as anti-pattern. We try to avoid use Linq at the lower levels of the stack since it has large performance overhead. I would be good to get rid of Linq in System.IO regardless.

@adamsitnik adamsitnik deleted the fileHandleOpen branch May 27, 2021 11:02
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants