Repository: tetratelabs/wazero
Pull request #973 (2022-12-29 03:55:36)

fs: splits unlink and rmdir from remove
This splits unlink and rmdir from remove, as it is not only more precise in GOOS=js, but it is also needed to implement wasi. I verified this works by running go unit tests with logging.

```
==> go.syscall/js.valueCall(fs.open(name=/tmp/TestErrIsNotExist1062486353,flags=,perm=----------))
<== (err=<nil>,fd=10)
==> go.syscall/js.valueCall(fs.fstat(fd=10))
<== (err=<nil>,stat={isDir=true,mode=-rwx------,size=96,mtimeMs=1672285985206})
==> go.syscall/js.valueCall(fs.readdir(name=/tmp/TestErrIsNotExist1062486353))
<== (err=<nil>,dirents=&{[001]})
==> go.syscall/js.valueCall(fs.unlink(path=/tmp/TestErrIsNotExist1062486353/001))
<== (err=is a directory,ok=true)
==> go.syscall/js.valueCall(fs.rmdir(path=/tmp/TestErrIsNotExist1062486353/001))
<== (err=<nil>,ok=false)
```


codefromthecrypt: heh windows again, but luckily I have my laptop ready

Patch:
diff --git a/internal/gojs/fs.go b/internal/gojs/fs.go
index 06a1275d..8e983cdd 100644
--- a/internal/gojs/fs.go
+++ b/internal/gojs/fs.go
@@ -34,7 +34,7 @@ var (
 		addFunction(custom.NameFsWrite, &jsfsWrite{}).
 		addFunction(custom.NameFsReaddir, &jsfsReaddir{}).
 		addFunction(custom.NameFsMkdir, &jsfsMkdir{}).
-		addFunction(custom.NameFsRmdir, &jsfsUnlink{}).
+		addFunction(custom.NameFsRmdir, &jsfsRmdir{}).
 		addFunction(custom.NameFsUnlink, &jsfsUnlink{})
 
 	// TODO: stub all these with syscall.ENOSYS
@@ -438,9 +438,29 @@ func syscallMkdir(mod api.Module, name string, perm uint32) (uint32, error) {
 	return fsc.Mkdir(name, fs.FileMode(perm))
 }
 
+// jsfsRmdir implements the following
+//
+//	_, err := fsCall("rmdir", path) // syscall.Rmdir
+type jsfsRmdir struct{}
+
+// invoke implements jsFn.invoke
+func (*jsfsRmdir) invoke(ctx context.Context, mod api.Module, args ...interface{}) (interface{}, error) {
+	name := args[0].(string)
+	callback := args[1].(funcWrapper)
+
+	ok, err := syscallRmdir(mod, name)
+	return callback.invoke(ctx, mod, goos.RefJsfs, err, ok) // note: error first
+}
+
+// syscallRmdir is like syscall.Rmdir
+func syscallRmdir(mod api.Module, name string) (interface{}, error) {
+	fsc := mod.(*wasm.CallContext).Sys.FS()
+	err := fsc.Rmdir(name)
+	return err != nil, err
+}
+
 // jsfsUnlink implements the following
 //
-//	_, err := fsCall("rmdir", path)
 //	_, err := fsCall("unlink", path) // syscall.Unlink
 type jsfsUnlink struct{}
 
diff --git a/internal/gojs/syscall.go b/internal/gojs/syscall.go
index 1b0df54a..4aed3140 100644
--- a/internal/gojs/syscall.go
+++ b/internal/gojs/syscall.go
@@ -367,6 +367,7 @@ func (e *syscallErr) Error() string {
 var (
 	ebadf     = &syscallErr{"EBADF"}
 	einval    = &syscallErr{"EBADF"}
+	eisdir    = &syscallErr{"EISDIR"}
 	eexist    = &syscallErr{"EEXIST"}
 	enoent    = &syscallErr{"ENOENT"}
 	enosys    = &syscallErr{"ENOSYS"}
@@ -385,6 +386,8 @@ func mapJSError(err error) *syscallErr {
 		return ebadf
 	case errors.Is(err, syscall.EINVAL), errors.Is(err, fs.ErrInvalid):
 		return einval
+	case errors.Is(err, syscall.EISDIR):
+		return eisdir
 	case errors.Is(err, syscall.ENOTEMPTY):
 		return enotempty
 	case errors.Is(err, syscall.EEXIST), errors.Is(err, fs.ErrExist):
diff --git a/internal/sys/fs.go b/internal/sys/fs.go
index d9d3d695..d8ce5e90 100644
--- a/internal/sys/fs.go
+++ b/internal/sys/fs.go
@@ -315,16 +315,6 @@ func (c *FSContext) Mkdir(name string, perm fs.FileMode) (newFD uint32, err erro
 	return
 }
 
-// Unlink is like syscall.Unlink.
-func (c *FSContext) Unlink(name string) (err error) {
-	if wfs, ok := c.fs.(writefs.FS); ok {
-		name = c.cleanPath(name)
-		return wfs.Remove(name)
-	}
-	err = syscall.ENOSYS
-	return
-}
-
 // OpenFile is like syscall.Open and returns the file descriptor of the new file or an error.
 func (c *FSContext) OpenFile(name string, flags int, perm fs.FileMode) (newFD uint32, err error) {
 	create := flags&os.O_CREATE != 0
@@ -351,6 +341,16 @@ func (c *FSContext) OpenFile(name string, flags int, perm fs.FileMode) (newFD ui
 	return newFD, nil
 }
 
+// Rmdir is like syscall.Rmdir.
+func (c *FSContext) Rmdir(name string) (err error) {
+	if wfs, ok := c.fs.(writefs.FS); ok {
+		name = c.cleanPath(name)
+		return wfs.Rmdir(name)
+	}
+	err = syscall.ENOSYS
+	return
+}
+
 func (c *FSContext) StatPath(name string) (fs.FileInfo, error) {
 	f, err := c.openFile(name)
 	if err != nil {
@@ -360,6 +360,16 @@ func (c *FSContext) StatPath(name string) (fs.FileInfo, error) {
 	return f.Stat()
 }
 
+// Unlink is like syscall.Unlink.
+func (c *FSContext) Unlink(name string) (err error) {
+	if wfs, ok := c.fs.(writefs.FS); ok {
+		name = c.cleanPath(name)
+		return wfs.Unlink(name)
+	}
+	err = syscall.ENOSYS
+	return
+}
+
 func (c *FSContext) openFile(name string) (fs.File, error) {
 	return c.fs.Open(c.cleanPath(name))
 }
diff --git a/internal/writefs/syscall.go b/internal/writefs/syscall.go
new file mode 100644
index 00000000..8d4fd1d0
--- /dev/null
+++ b/internal/writefs/syscall.go
@@ -0,0 +1,20 @@
+//go:build !windows
+
+package writefs
+
+import "syscall"
+
+func adjustMkdirError(err error) error {
+	return err
+}
+
+func adjustRmdirError(err error) error {
+	return err
+}
+
+func adjustUnlinkError(err error) error {
+	if err == syscall.EPERM {
+		return syscall.EISDIR
+	}
+	return err
+}
diff --git a/internal/writefs/syscall_windows.go b/internal/writefs/syscall_windows.go
new file mode 100644
index 00000000..0252a8dc
--- /dev/null
+++ b/internal/writefs/syscall_windows.go
@@ -0,0 +1,42 @@
+package writefs
+
+import (
+	"io/fs"
+	"syscall"
+)
+
+const (
+	// ERROR_ACCESS_DENIED is a Windows error returned by syscall.Unlink
+	// instead of syscall.EPERM
+	ERROR_ACCESS_DENIED = syscall.Errno(5)
+
+	// ERROR_ALREADY_EXISTS is a Windows error returned by os.Mkdir
+	// instead of syscall.EEXIST
+	ERROR_ALREADY_EXISTS = syscall.Errno(183)
+
+	// ERROR_DIRECTORY is a Windows error returned by syscall.Rmdir
+	// instead of syscall.ENOTDIR
+	ERROR_DIRECTORY = syscall.Errno(267)
+)
+
+func adjustMkdirError(err error) error {
+	// os.Mkdir wraps the syscall error in a path error
+	if pe, ok := err.(*fs.PathError); ok && pe.Err == ERROR_ALREADY_EXISTS {
+		pe.Err = syscall.EEXIST // adjust it
+	}
+	return err
+}
+
+func adjustRmdirError(err error) error {
+	if err == ERROR_DIRECTORY {
+		return syscall.ENOTDIR
+	}
+	return err
+}
+
+func adjustUnlinkError(err error) error {
+	if err == ERROR_ACCESS_DENIED {
+		return syscall.EISDIR
+	}
+	return err
+}
diff --git a/internal/writefs/writefs.go b/internal/writefs/writefs.go
index 874805d9..000431ce 100644
--- a/internal/writefs/writefs.go
+++ b/internal/writefs/writefs.go
@@ -3,6 +3,8 @@ package writefs
 import (
 	"io/fs"
 	"os"
+	"path"
+	"syscall"
 )
 
 // FS is a fs.FS which can also create new files or directories.
@@ -21,9 +23,25 @@ type FS interface {
 	// system.
 	Mkdir(name string, perm fs.FileMode) error
 
-	// Remove is similar to os.Remove, except the path is relative to this file
-	// system.
-	Remove(path string) error
+	// Rmdir is similar to syscall.Rmdir, except the path is relative to this
+	// file system.
+	//
+	// # Errors
+	//
+	// The following errors are expected:
+	//   - syscall.EINVAL: `path` is invalid.
+	//   - syscall.ENOENT: `path` doesn't exist.
+	//   - syscall.ENOTDIR: `path` exists, but isn't a directory.
+	Rmdir(path string) error
+
+	// Unlink is similar to syscall.Unlink, except the path is relative to this
+	// file system.
+	//
+	// The following errors are expected:
+	//   - syscall.EINVAL: `path` is invalid.
+	//   - syscall.ENOENT: `path` doesn't exist.
+	//   - syscall.EISDIR: `path` exists, but is a directory.
+	Unlink(path string) error
 }
 
 func DirFS(dir string) FS {
@@ -42,7 +60,7 @@ func (dir dirFS) OpenFile(name string, flag int, perm fs.FileMode) (fs.File, err
 	if !fs.ValidPath(name) {
 		return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
 	}
-	return os.OpenFile(string(dir)+"/"+name, flag, perm)
+	return os.OpenFile(path.Join(string(dir), name), flag, perm)
 }
 
 // Mkdir implements FS.Mkdir
@@ -50,13 +68,30 @@ func (dir dirFS) Mkdir(name string, perm fs.FileMode) error {
 	if !fs.ValidPath(name) {
 		return &fs.PathError{Op: "mkdir", Path: name, Err: fs.ErrInvalid}
 	}
-	return os.Mkdir(string(dir)+"/"+name, perm)
+
+	err := os.Mkdir(path.Join(string(dir), name), perm)
+
+	return adjustMkdirError(err)
 }
 
-// Remove implements FS.Remove
-func (dir dirFS) Remove(path string) error {
-	if !fs.ValidPath(path) {
-		return &fs.PathError{Op: "remove", Path: path, Err: fs.ErrInvalid}
+// Rmdir implements FS.Rmdir
+func (dir dirFS) Rmdir(name string) error {
+	if !fs.ValidPath(name) {
+		return syscall.EINVAL
 	}
-	return os.Remove(string(dir) + "/" + path)
+
+	err := syscall.Rmdir(path.Join(string(dir), name))
+
+	return adjustRmdirError(err)
+}
+
+// Unlink implements FS.Unlink
+func (dir dirFS) Unlink(name string) error {
+	if !fs.ValidPath(name) {
+		return syscall.EINVAL
+	}
+
+	err := syscall.Unlink(path.Join(string(dir), name))
+
+	return adjustUnlinkError(err)
 }


Test patch:
diff --git a/internal/gojs/testdata/writefs/main.go b/internal/gojs/testdata/writefs/main.go
index 7dcb4634..92b27481 100644
--- a/internal/gojs/testdata/writefs/main.go
+++ b/internal/gojs/testdata/writefs/main.go
@@ -5,9 +5,11 @@ import (
 	"log"
 	"os"
 	"path"
+	"syscall"
 )
 
 func Main() {
+	// Create a test directory
 	dir := path.Join(os.TempDir(), "dir")
 	err := os.Mkdir(dir, 0o700)
 	if err != nil {
@@ -16,6 +18,7 @@ func Main() {
 	}
 	defer os.Remove(dir)
 
+	// Create a test file in that directory
 	file := path.Join(dir, "file")
 	err = os.WriteFile(file, []byte{}, 0o600)
 	if err != nil {
@@ -33,9 +36,25 @@ func Main() {
 		}
 	}
 
-	// should fail due to non-empty dir
-	if err = os.Remove(dir); err == nil {
-		log.Panicln("shouldn't be able to remove", dir)
+	// Test removing a non-empty empty directory
+	if err = syscall.Rmdir(dir); err != syscall.ENOTEMPTY {
+		log.Panicln("unexpected error", err)
+	}
+
+	// Test unlinking a file
+	if err = syscall.Rmdir(file); err != syscall.ENOTDIR {
+		log.Panicln("unexpected error", err)
+	}
+	if err = syscall.Unlink(file); err != nil {
+		log.Panicln("unexpected error", err)
+	}
+
+	// Test removing an empty directory
+	if err = syscall.Unlink(dir); err != syscall.EISDIR {
+		log.Panicln("unexpected error", err)
+	}
+	if err = syscall.Rmdir(dir); err != nil {
+		log.Panicln("unexpected error", err)
 	}
 
 	// shouldn't fail
diff --git a/internal/writefs/writefs_test.go b/internal/writefs/writefs_test.go
index 1ec928bd..ad7341de 100644
--- a/internal/writefs/writefs_test.go
+++ b/internal/writefs/writefs_test.go
@@ -1,10 +1,12 @@
 package writefs
 
 import (
+	"errors"
 	"io/fs"
 	"os"
 	"path"
 	"runtime"
+	"syscall"
 	"testing"
 	"testing/fstest"
 
@@ -18,7 +20,7 @@ var testFiles = map[string]string{
 	"sub/sub/test.txt": "greet sub sub dir\n",
 }
 
-func TestFS(t *testing.T) {
+func TestDirFS_TestFS(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		// This abstraction is a toe-hold, but we'll have to sort windows with
 		// our ideal filesystem tester.
@@ -38,54 +40,94 @@ func TestFS(t *testing.T) {
 	}
 }
 
-func TestMkDir(t *testing.T) {
+func TestDirFS_MkDir(t *testing.T) {
 	dir := t.TempDir()
 
 	testFS := DirFS(dir)
 
 	name := "mkdir"
+	realPath := path.Join(dir, name)
 
 	t.Run("doesn't exist", func(t *testing.T) {
 		require.NoError(t, testFS.Mkdir(name, fs.ModeDir))
-		stat, err := os.Stat(path.Join(dir, name))
+		stat, err := os.Stat(realPath)
 		require.NoError(t, err)
 		require.Equal(t, name, stat.Name())
 		require.True(t, stat.IsDir())
 	})
 
-	t.Run("exists", func(t *testing.T) {
-		require.Error(t, testFS.Mkdir(name, fs.ModeDir))
+	t.Run("dir exists", func(t *testing.T) {
+		err := testFS.Mkdir(name, fs.ModeDir)
+		require.Equal(t, syscall.EEXIST, errors.Unwrap(err))
+	})
+
+	t.Run("file exists", func(t *testing.T) {
+		require.NoError(t, os.Remove(realPath))
+		require.NoError(t, os.Mkdir(realPath, 0o700))
+
+		err := testFS.Mkdir(name, fs.ModeDir)
+		require.Equal(t, syscall.EEXIST, errors.Unwrap(err))
 	})
 }
 
-func TestRemove(t *testing.T) {
+func TestDirFS_Rmdir(t *testing.T) {
 	dir := t.TempDir()
 
 	testFS := DirFS(dir)
 
-	name := "remove"
+	name := "rmdir"
+	realPath := path.Join(dir, name)
 
 	t.Run("doesn't exist", func(t *testing.T) {
-		require.Error(t, testFS.Remove(name))
+		err := testFS.Rmdir(name)
+		require.Equal(t, syscall.ENOENT, err)
 	})
 
 	t.Run("dir exists", func(t *testing.T) {
-		realPath := path.Join(dir, name)
-		err := os.Mkdir(realPath, 0o700)
-		require.NoError(t, err)
+		require.NoError(t, os.Mkdir(realPath, 0o700))
 
-		require.NoError(t, testFS.Remove(name))
-		_, err = os.Stat(realPath)
+		require.NoError(t, testFS.Rmdir(name))
+		_, err := os.Stat(realPath)
 		require.Error(t, err)
 	})
 
 	t.Run("file exists", func(t *testing.T) {
-		realPath := path.Join(dir, name)
-		err := os.WriteFile(realPath, []byte{}, 0o600)
-		require.NoError(t, err)
+		require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
+
+		err := testFS.Rmdir(name)
+		require.Equal(t, syscall.ENOTDIR, err)
+
+		require.NoError(t, os.Remove(realPath))
+	})
+}
+
+func TestDirFS_Unlink(t *testing.T) {
+	dir := t.TempDir()
+
+	testFS := DirFS(dir)
+
+	name := "unlink"
+	realPath := path.Join(dir, name)
+
+	t.Run("doesn't exist", func(t *testing.T) {
+		err := testFS.Unlink(name)
+		require.Equal(t, syscall.ENOENT, err)
+	})
+
+	t.Run("dir exists", func(t *testing.T) {
+		require.NoError(t, os.Mkdir(realPath, 0o700))
+
+		err := testFS.Unlink(name)
+		require.Equal(t, syscall.EISDIR, err)
+
+		require.NoError(t, os.Remove(realPath))
+	})
+
+	t.Run("file exists", func(t *testing.T) {
+		require.NoError(t, os.WriteFile(realPath, []byte{}, 0o600))
 
-		require.NoError(t, testFS.Remove(name))
-		_, err = os.Stat(realPath)
+		require.NoError(t, testFS.Unlink(name))
+		_, err := os.Stat(realPath)
 		require.Error(t, err)
 	})
 }