Skip to content

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
@adamsitnik adamsitnik requested review from carlossanlop and jozkee May 24, 2021 10:44
@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

@adamsitnik adamsitnik requested a review from stephentoub May 24, 2021 10:51
@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
Contributor

@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