The main issue with this approach is the whole optional method thing; the moment you use this helper all additional FS methods are lost. To do it "correctly", you end up having to do what httpsnoop had to do to deal with the optional interface problem with ResponseWriter, or if the ErrNotSupported value becomes standard, make io/fs somehow implement every possible method (because you couldn't just embed the result of a new fs.WithPrefix and add on without losing the extra methods entirely). This all was brought up during the design of io/fs on Reddit and the issue thread.

For net/http, this doesn't matter, since http.FS only uses Open, but for more generic uses of io/fs, this might matter more. I mentioned in a followup comment that I could achieve the same thing with an AddPrefix counterpart to StripPrefix in net/http (#41191 (comment)), which might work better, but doesn't solve the overall problem of "I can't subtree an FS".

I'm still not certain if this is a "problem" with how static serving works in net/http (the awkward set of needing all of the prefix strippers to modify Request), or the general problem that the design of io/fs doesn't lend itself to subtreeing if you want retain efficiency. Maybe the later isn't 100% true, though, since who knows how any given FS would really want to handle prefixes.

In the comments on the io/fs proposal, as I understood it, the consensus was that the "right" way to deal with the optional interfaces was to implement them all and then return ErrNotSupported if the underlying FS didn't have the method in question.

Forgive me, I meant ErrUnsupported (#41198), not ErrNotSupported (which is a different thing...). Note that while ErrUnsupported is accepted, it's not implemented and not used in io/fs.

@zikaeroh The general recommendation for optional methods is to add them all and fall-back to the appropriate helper in io/fs if you can't implement them specifically. That helper should then do the correct thing (i.e. type-assert OR fallback-implementation OR return ErrUnsupported). That it was not necessary to use ErrUnsupported (or equivalent) in io/fs so far is not a reason it couldn't be used.

If there is an optional interface for which this approach doesn't work, details would be very useful (probably in a separate bug) because it would point to a serious design issue which should be discussed before go 1.16 is released. I don't know of any such instance yet, but that doesn't mean it doesn't exist. In particular, I don't see any problems in interactions with optional interfaces I know about and a hypothetical StripPrefixFS.

I think this proposal is a good idea and I would very much like to see it happen. To me, this is a fundamental primitive of composability for file systems and I would like to see it in io/fs - just like io contains MultiReader, TeeReader and other fundamental composability primitives of I/O-streams.

/cc @rsc

My initial instinct was that this would be useful because it could add more safety when trying to "sandbox" parts of the filesystem. But then I remembered that in the io/fs draft design .. is forbidden already. In theory this seems like a pretty easy to implement proposal (string concatenation and all the stuff needed to handle optional methods, right?) Is there some subtle reason this should be in the standard lib?

(apologies for poorly formatted example; written directly in browser.)

type WPFS struct {
   inner  FS
   prefix string
}

func (fs WPFS) Open(p string) (File, error) {
   return fs.inner.Open(inner.prefix + "/" + p)
}

// same thing but for ReadFile, Stat, ReadDir, Glob, and maybe Rename, OpenFile?

func WithPrefix(f FS, p string) WPFS {
   return WPFS{f, p}
}

Is there some subtle reason this should be in the standard lib?

No, not a subtle reason. The non-subtle reason is what I mentioned above: It's a fundamental primitive of composition, so making it available is akin to other top-level functions in the io package, for example.

An alternative name could be Chroot

When we talked about adding this during the proposal discussion on Reddit, I was thinking it could be called just fs.Sub, as in sub-tree. Chroot is a bit too obscure (but accurate!), and WithPrefix is maybe too much about the mechanics (and maybe inaccurate! The argument to Open is without the prefix).

I was thinking we could put this off until the next release and get more experience with io/fs, but I agree that it is a critical piece to have to use with embedding.

Does anyone object to adding func Sub(fsys FS, dir string) FS to io/fs and also the corresponding SubFS interface?

What should the SubFS interface look like? Sub(string) (FS, error)? Will it work with the not implemented error (I forget which name won for that)? SGTM as long as it returns some error.

I agree that the interface should return an error, because if, for example, dir does not exist, it makes sense to report that sooner rather than later. And if the method returns an error, so should fs.Sub itself.
However, I don't think the method would ever need to return ErrNotImplemented (or somesuch) because it can always safely fall back to fs.Sub.

It's a lot more awkward to call Sub compared to http.StripPrefix if it returns an error.
And it's easy to check if you want the error: call Stat(".") on the result.
I'm leaning toward leaving the error off.

@carlmjohnson, no there is no "not implemented". If an implementation wants to provide a Sub but doesn't know how, it can call fs.Sub.

Based on the discussion, this seems like a likely accept, and for Go 1.16 so that it is part of the initial FS API.

@rsc I'm thinking about an io/fs using openat and similar syscalls. The simplest implementation of that would just use a single uintptr of the dirfd. Sub would then just open the directory and return the resulting file-descriptor. ISTM that if Sub can't return an error, you need additional bookkeeping information and extra checks to make sure the dirfd is actually valid and you still have to actually do something with the error returned by open. Not a total dealbreaker, but IMO returning an error from Sub is cleaner in this example.

@icholy No, you pass the wrapped file-system to fs.Sub, not yourself.

I think the proposal is useful, but I think it should be documented that this is not a security features, as even just reading a symlinked file allows to escape the filesystem root (in fact, at some point we might want to add a fs.Jail in the future with security implications, but that's for another proposal).

If fs.Jail is a thing, maybe it matters less that fs.Sub has no error checking. fs.Jail could complain that you don't have permission to use a directory in the first place, etc. Having them separate would also mean that fs.Sub wouldn't have to clean all paths of .. on each and every use, which might be nice for performance.

But if fs.Sub is less secure than fs.Jail, would we want to encourage its use in http.FS?

http.Dir has the same caveat. I think that's fine.

No change in consensus, so accepted.

I went to implement this, and it does seem like we need the error result if only for diagnosing invalid arguments properly.
So that's what the CL has.

Change https://golang.org/cl/274856 mentions this issue: io/fs: add Sub