xoofx / zio

A cross-platform abstract/virtual filesystem framework with many built-ins filesystems for .NET
BSD 2-Clause "Simplified" License
827 stars 61 forks source link

Implicit string conversion to/from UPath is subtle & error prone #75

Open agocke opened 1 year ago

agocke commented 1 year ago

Just hit this:

var fs = new SubFileSystem(new PhysicalFileSystem(), realPath));

I assume the proper way to do this is:

var physicalFs = new PhysicalFileSystem();
var fs = new SubFileSystem(physicalFs, physicalFs.ConvertPathFromInternal(realPath)));

If there weren't an implicit conversion, I probably would have guessed that. Overall, it seems like keeping UPaths and strings a bit separate is a good idea. Especially when working with subpaths like this, the chances that you'll confuse one for the other is pretty high.

xoofx commented 1 year ago

Agreed, that's one of the gray area of the API. Not allowing implicit conversion from string to UPath would have lead to annoying explicit cast in many places (more than the creation of filesystems but all methods provided by IFileSystem) so made this choice of simplicity vs correctness, but the downside is that there are a few cases like the one you point out that are not protected against a misusages (and the XML doc is not saying anything about this). Not sure I would want to change this after all these years this API has been released though...

agocke commented 1 year ago

An analyzer might work here, along with an option to enable it. Not taking a breaking change makes sense.

In the meantime I might try an expedient route and see if the BannedApiAnalyzer could catch this.

agocke commented 1 year ago

Aside: the / operator override is clever. 😄