[RFC] 'walkDir' now has a new 'checkDir' flag, to mimic behaviour of other languages (#13642)

Co-authored-by: narimiran

changelog.md

@@ -7,7 +7,7 @@
   on GCC's `__builtin_sadd_overflow` family of functions. (Clang also
   supports these). Some versions of GCC lack this feature and unfortunately
   we cannot detect this case reliably. So if you get compilation errors like
-  "undefined reference to '__builtin_saddll_overflow'" compile your programs
+  "undefined reference to `__builtin_saddll_overflow`" compile your programs
   with `-d:nimEmulateOverflowChecks`.
 
 
@@ -34,8 +34,8 @@
 - `options` now treats `proc` like other pointer types, meaning `nil` proc variables
   are converted to `None`.
 - `relativePath("foo", "foo")` is now `"."`, not `""`, as `""` means invalid path
-  and shouldn't be conflated with `"."`; use -d:nimOldRelativePathBehavior to restore the old
+  and shouldn't be conflated with `"."`; use -d:nimOldRelativePathBehavior to
-  behavior
+  restore the old behavior
 - `joinPath(a,b)` now honors trailing slashes in `b` (or `a` if `b` = "")
 - `times.parse` now only uses input to compute its result, and not `now`:
   `parse("2020", "YYYY", utc())` is now `2020-01-01T00:00:00Z` instead of
@@ -44,6 +44,12 @@
 - `httpcore.==(string, HttpCode)` is now deprecated due to lack of practical
   usage. The `$` operator can be used to obtain the string form of `HttpCode`
   for comparison if desired.
+- `os.walkDir` and `os.walkDirRec` now have new flag, `checkDir` (default: false).
+  If it is set to true, it will throw if input dir is invalid instead of a noop
+  (which is the default behaviour, as it was before this change),
+  `os.walkDirRec` only throws if top-level dir is invalid, but ignores errors for
+  subdirs, otherwise it would be impossible to resume iteration.
+
 
 ### Breaking changes in the compiler
 

compiler/scriptconfig.nim

@@ -65,7 +65,7 @@ proc setupVM*(module: PSym; cache: IdentCache; scriptName: string;
     if defined(nimsuggest) or graph.config.cmd == cmdCheck:
       discard
     else:
-      os.removeDir getString(a, 0)
+      os.removeDir(getString(a, 0), getBool(a, 1))
   cbos removeFile:
     if defined(nimsuggest) or graph.config.cmd == cmdCheck:
       discard

lib/pure/os.nim

@@ -2028,8 +2028,8 @@ proc staticWalkDir(dir: string; relative: bool): seq[
                   tuple[kind: PathComponent, path: string]] =
   discard
 
-iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path: string] {.
+iterator walkDir*(dir: string; relative = false, checkDir = false):
-  tags: [ReadDirEffect].} =
+  tuple[kind: PathComponent, path: string] {.tags: [ReadDirEffect].} =
   ## Walks over the directory `dir` and yields for each directory or file in
   ## `dir`. The component type and full path for each item are returned.
   ##
@@ -2038,7 +2038,6 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
   ## Example: This directory structure::
   ##   dirA / dirB / fileB1.txt
   ##        / dirC
-  ##        / fileA1.txt
   ##        / fileA2.txt
   ##
   ## and this code:
@@ -2069,7 +2068,10 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
     elif defined(windows):
       var f: WIN32_FIND_DATA
       var h = findFirstFile(dir / "*", f)
-      if h != -1:
+      if h == -1:
+        if checkDir:
+          raiseOSError(osLastError(), dir)
+      else:
         defer: findClose(h)
         while true:
           var k = pcFile
@@ -2087,7 +2089,10 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
             else: raiseOSError(errCode.OSErrorCode)
     else:
       var d = opendir(dir)
-      if d != nil:
+      if d == nil:
+        if checkDir:
+          raiseOSError(osLastError(), dir)
+      else:
         defer: discard closedir(d)
         while true:
           var x = readdir(d)
@@ -2122,7 +2127,7 @@ iterator walkDir*(dir: string; relative=false): tuple[kind: PathComponent, path:
 
 iterator walkDirRec*(dir: string,
                      yieldFilter = {pcFile}, followFilter = {pcDir},
-                     relative = false): string {.tags: [ReadDirEffect].} =
+                     relative = false, checkDir = false): string {.tags: [ReadDirEffect].} =
   ## Recursively walks over the directory `dir` and yields for each file
   ## or directory in `dir`.
   ##
@@ -2159,14 +2164,20 @@ iterator walkDirRec*(dir: string,
   ## * `walkDir iterator <#walkDir.i,string>`_
 
   var stack = @[""]
+  var checkDir = checkDir
   while stack.len > 0:
     let d = stack.pop()
-    for k, p in walkDir(dir / d, relative = true):
+    for k, p in walkDir(dir / d, relative = true, checkDir = checkDir):
       let rel = d / p
       if k in {pcDir, pcLinkToDir} and k in followFilter:
         stack.add rel
       if k in yieldFilter:
         yield if relative: rel else: dir / rel
+    checkDir = false
+      # We only check top-level dir, otherwise if a subdir is invalid (eg. wrong
+      # permissions), it'll abort iteration and there would be no way to
+      # continue iteration.
+      # Future work can provide a way to customize this and do error reporting.
 
 proc rawRemoveDir(dir: string) {.noNimScript.} =
   when defined(windows):
@@ -2181,13 +2192,13 @@ proc rawRemoveDir(dir: string) {.noNimScript.} =
   else:
     if rmdir(dir) != 0'i32 and errno != ENOENT: raiseOSError(osLastError(), dir)
 
-proc removeDir*(dir: string) {.rtl, extern: "nos$1", tags: [
+proc removeDir*(dir: string, checkDir = false) {.rtl, extern: "nos$1", tags: [
   WriteDirEffect, ReadDirEffect], benign, noNimScript.} =
   ## Removes the directory `dir` including all subdirectories and files
   ## in `dir` (recursively).
   ##
   ## If this fails, `OSError` is raised. This does not fail if the directory never
-  ## existed in the first place.
+  ## existed in the first place, unless `checkDir` = true
   ##
   ## See also:
   ## * `tryRemoveFile proc <#tryRemoveFile,string>`_
@@ -2197,10 +2208,13 @@ proc removeDir*(dir: string) {.rtl, extern: "nos$1", tags: [
   ## * `copyDir proc <#copyDir,string,string>`_
   ## * `copyDirWithPermissions proc <#copyDirWithPermissions,string,string>`_
   ## * `moveDir proc <#moveDir,string,string>`_
-  for kind, path in walkDir(dir):
+  for kind, path in walkDir(dir, checkDir = checkDir):
     case kind
     of pcFile, pcLinkToFile, pcLinkToDir: removeFile(path)
-    of pcDir: removeDir(path)
+    of pcDir: removeDir(path, true)
+      # for subdirectories there is no benefit in `checkDir = false`
+      # (unless perhaps for edge case of concurrent processes also deleting
+      # the same files)
   rawRemoveDir(dir)
 
 proc rawCreateDir(dir: string): bool {.noNimScript.} =

lib/system/nimscript.nim

@@ -30,7 +30,7 @@ proc listDirsImpl(dir: string): seq[string] {.
   tags: [ReadIOEffect], raises: [OSError].} = builtin
 proc listFilesImpl(dir: string): seq[string] {.
   tags: [ReadIOEffect], raises: [OSError].} = builtin
-proc removeDir(dir: string) {.
+proc removeDir(dir: string, checkDir = true) {.
   tags: [ReadIOEffect, WriteIOEffect], raises: [OSError].} = builtin
 proc removeFile(dir: string) {.
   tags: [ReadIOEffect, WriteIOEffect], raises: [OSError].} = builtin
@@ -204,10 +204,10 @@ proc listFiles*(dir: string): seq[string] =
   result = listFilesImpl(dir)
   checkOsError()
 
-proc rmDir*(dir: string) {.raises: [OSError].} =
+proc rmDir*(dir: string, checkDir = false) {.raises: [OSError].} =
   ## Removes the directory `dir`.
   log "rmDir: " & dir:
-    removeDir dir
+    removeDir(dir, checkDir = checkDir)
     checkOsError()
 
 proc rmFile*(file: string) {.raises: [OSError].} =

tests/newconfig/tfoo.nims

@@ -24,8 +24,10 @@ doAssert(existsEnv("dummy") == false)
 
 # issue #7283
 putEnv("dummy", "myval")
-doAssert(existsEnv("dummy") == true)
+doAssert(existsEnv("dummy"))
 doAssert(getEnv("dummy") == "myval")
+delEnv("dummy")
+doAssert(existsEnv("dummy") == false)
 
 # issue #7393
 let wd = getCurrentDir()
@@ -64,6 +66,8 @@ else:
   assert toDll("nim") == "libnim.so"
 
 rmDir("tempXYZ")
+doAssertRaises(OSError):
+  rmDir("tempXYZ", checkDir = true)
 assert dirExists("tempXYZ") == false
 mkDir("tempXYZ")
 assert dirExists("tempXYZ") == true
@@ -81,8 +85,3 @@ when false:
 
 rmDir("tempXYZ")
 assert dirExists("tempXYZ") == false
-
-putEnv("dummy", "myval")
-doAssert(existsEnv("dummy") == true)
-delEnv("dummy")
-doAssert(existsEnv("dummy") == false)

tests/stdlib/tos.nim

@@ -163,13 +163,19 @@ block walkDirRec:
 
   removeDir("walkdir_test")
 
-when not defined(windows):
+block: # walkDir
-  block walkDirRelative:
+  doAssertRaises(OSError):
-    createDir("walkdir_test")
+    for a in walkDir("nonexistant", checkDir = true): discard
-    createSymlink(".", "walkdir_test/c")
+  doAssertRaises(OSError):
-    for k, p in walkDir("walkdir_test", true):
+    for p in walkDirRec("nonexistant", checkDir = true): discard
-      doAssert k == pcLinkToDir
+
-    removeDir("walkdir_test")
+  when not defined(windows):
+    block walkDirRelative:
+      createDir("walkdir_test")
+      createSymlink(".", "walkdir_test/c")
+      for k, p in walkDir("walkdir_test", true):
+        doAssert k == pcLinkToDir
+      removeDir("walkdir_test")
 
 block normalizedPath:
   doAssert normalizedPath("") == ""