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

Jump to bottom

io/fs: add more specific documentation about valid paths #44168

Open
elagergren-spideroak opened this issue Feb 8, 2021 · 2 comments
Open

io/fs: add more specific documentation about valid paths #44168

elagergren-spideroak opened this issue Feb 8, 2021 · 2 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@elagergren-spideroak
Copy link

elagergren-spideroak commented Feb 8, 2021 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.16rc1 darwin/amd64

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/xyz/gopath/bin"
GOCACHE="/Users/xyz/Library/Caches/go-build"
GOENV="/Users/xyz/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/xyz/gopath/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/elagergren/gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/xyz/sdk/go1.16rc1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/xyz/sdk/go1.16rc1/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16rc1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/private/tmp/b58/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/2b/74fz3jhd4wz4vnbf4z7ywzww0000gp/T/go-build3417337290=/tmp/go-build -gno-record-gcc-switches -fno-common"

We are early adopters of io/fs: we'd been wanting to move to a VFS implementation and the original proposal gave us the push to do so. So far, it has been a good decision. It's allowed us to stub out the file system implementation for tests (SlowFS, CorruptedFS, etc.), to more easily test bugs by replaying application state from ZIP files, and to transparently encrypt files.

One pain point, however, is knowing what constitutes a valid file path for a particular FS. With the os package, we could make a reasonable set of assumptions. For example, rejecting or not creating paths with problematic or illegal characters like null bytes or rejecting or not creating excessively long paths. But with io/fs, we can't. It's impossible to know whether the FS is os.DirFS, a remote object store, or an in-memory file system.

There seem to be two possibly correct answers:

  1. A FS must accept any path for which ValidPath returns true and therefore translate it accordingly
  2. It's impossible to know and therefore the API using io/fs must document what it expects out of the FS

The implementation of os.DirFS seems to assume (2), but it's not clearly documented. ValidPath should more clearly document its assumptions.

Relates to #44166.
@elagergren-spideroak
Copy link
Author

elagergren-spideroak commented Feb 8, 2021 •
edited

Personally, I worry that (2) will cause too much friction.

I've found that io/fs causes me to document a multitude of supported auxiliary interfaces (see snippet below). Adding path element conventions on top of that increases the wall of text, which makes it that much more frustrating to use the API.

Further, it means users can't just drop in their preferred FS like you can with other common standard library interfaces (io.Reader, etc.). Instead, they'll have to add a glue layer that translates my path element scheme—whatever it may be—to their desired FS. For example, if I document that my API uses colons in path elements then a user using os.DirFS will have to strip those out on Windows.

One of Go's strengths is making small, composable interfaces that you can rely on. The classic example is io.Reader: its documentation is exacting, and if you see Read([]byte) (int, error) you can reasonably assume what the method is going to do. But, if the documentation were vague—perhaps not documenting that (0, nil) is valid but discouraged or that it can return n !=0 if err == io.EOF—then it would be much more difficult to use: what does a particular result mean on Unix vs Windows? Does it matter that I'm reading from a memory buffer or socket? Etc.

// NewT creates a T that uses the FS to store its
// state.
//
// FS provides the minimum functionality required
// for a read-only T.
//
// To allow the T to modify its state, the FS must
// implement one or more of the following interfaces:
//
//    CreateFS
//      Allows the T to create additional files
//    RemoveFS
//      Allows the T to remove files that are no longer
//      needed.
//    [...]
//
func NewT(fsys fs.FS) T

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 8, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Feb 8, 2021

CC @rsc.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@dmitshur @gopherbot @seankhliao @elagergren-spideroak