add error-checked version of walkDir

[a-mr]

Alternative solution w.r.t. #13011, #12960.
New iterator walkDirErr with error-checking, use like this:

for kind, path, error in walkDirErr("dirA"):
  if error == OSErrorCode(0):
    echo path & " of kind " & $kind
  else:
    echo path & " GET INFO FAILED: " & osErrorMsg(error)

Other iterators remain unchecked.
Tested manually on Linux & Windows.

[juancarlospaco]

Being Nim a typed lang with overload and all, I dont think we need to add functionErrorname.
if err != nil: ... feels too Go lang.

[Araq]

I like this idea much better than #13011

[Araq]

Needs a .since annotation and a changelog entry.

[juancarlospaco]

What about tryWalkDir name to be consistent with the rest of the std lib ?.

[timotheecour]

will walkDirRec get similar treatment?

[Araq]

Indeed, please rename it to tryWalkDir.

[demotomohiro]

When findFirstFile or opendir failed (because dir is invalid path or no permission), walkDirErr yield dir.
But when there is no error, it doesn't yield dir.
How about to add pcInputPath to PathComponent enum and yield it in that case?
I can check if opening input dir is failed by comparing path and dir, but just checking error and kind is faster.

Can you write signle yield per when branch code?
https://nim-lang.org/docs/manual.html#iterators-and-the-for-statement-first-class-iterators

the body of a for loop over an inline iterator is inlined into each yield statement appearing in the iterator code, so ideally the code should be refactored to contain a single yield when possible to avoid code bloat.

[Araq]

How about to add pcInputPath to PathComponent enum and yield it in that case?

That would break code, code can contain exhaustive case statements, in fact, I generally encourage people to do that.

will walkDirRec get similar treatment?

No, we only need one non-recursive iterator as the building block for custom walks that can live outside the stdlib.

[a-mr]

For discussion: an updated version, slightly more generalized + some fixes:

for step in tryWalkDir(dir):
  case step.kind
  of wiOpenDir:
    if step.openStatus != odOpenOk:
      echo "directory " & dir & " could not be opened " &
           $step.openStatus & " / " & osErrorMsg(step.code)
  of wiEntryOk: echo "found " & step.path & " of kind " & $step.entryType
  of wiEntryBad: echo "bad symlink " & step.path & " with error " &
                      osErrorMsg(step.code)
  of wiInterrupted:
    echo "traversal interrupted with error: " & osErrorMsg(step.code)

[dom96]

I left a few comments, but I don't think this should be accepted into the stdlib for various reasons:

• I don't see this seeing enough use to be included in the stdlib
• The API for this should be modelled via something more generic, like a Either[T, E] type or something instead of this custom WalkStep object.
• The code being introduced in this PR is convoluted and hard to read, which will make it hard to maintain

[a-mr]

The solution of using closure iterators would be much simpler and more elegant (return tuple[OpenDirStatus, iterator]), but I'm not yet sure how to manage resources for them.
It seems that directory handles, captured by closure iterators are never freed.
A naive attempt to define a hook for the handles does not even pass compilation:

proc `=destroy`(m: var ptr DIR) =
  echo "Destruct"

Error: signature for '=destroy' must be proc[T: object](x: var T)

I can try using wrapper object for the handle.
There is a recent discussion on topic but it's without conclusion: Will --gc:arc address finalizers for closure iterator variables?

[Araq]

I don't see this seeing enough use to be included in the stdlib

I've asked for an implementation like this because our current means are insufficient for handling errors.

[a-mr]

It looks a bit bulky but there is a bright side: recursive version is straightforward for implementation, because WalkStep values can be passed upstream easily. Here is an analogue for walkDirRec from stdlib:

iterator tryWalkDirRec*(dir: string,
                        yieldFilter = {pcFile}, followFilter = {pcDir},
                        relative = false):
                       WalkStep {.tags: [ReadDirEffect].} =
  var stack = @[""]
  while stack.len > 0:
    let d = stack.pop()
    for s in tryWalkDir(dir / d, relative = true):
      let rel = d / s.path
      case s.kind
      of wsEntryOk:
        if s.entryType in {pcDir, pcLinkToDir} and
           s.entryType in followFilter:
          stack.add rel
        if s.entryType in yieldFilter:
          var step = s
          step.path = if relative: rel else: dir / rel
          yield step
      of wsEntryBad, wsInterrupted, wsOpenDir:
        var step = s
        step.path = if relative: rel else: dir / rel
        yield step

[timotheecour]

multiple yield statements in inline iterators cause code bloat and must be avoided, as explained here: #10553

see #10557 (which was closed but is relevant) for an example of how to convert multiple yield into a single yield

both current PR and #13163 (comment) suffer from this

One possibility would be to use a 1st class iterator instead of inline iterator, given cost of file system operations, the overhead of 1st class iterators would be negligible

[a-mr]

it was rewritten to use single yield. I'm using multiple yield only in examples for the sake of clarity.

[Araq]

This definitely goes in the right direction, but:

1. The control flow is unstructured. Too many break and continue statements make it hard to reason about. I personally also hate defer.
2. No less than 4 different enums for iterating over a dir? OpenDirStatus, WalkStepKind, OSErrorCode , PathComponent?

I understand that iterating over directories properly in 2020 involves some black magic, but this seems overly excessive.

[a-mr]

I combined OpenDirStatus into WalkStepKind. And I tried to simplify the code further, with limited success though. It's hard to do that with inline iterator using single yield statement.
It seems I've reached my limit here :-/

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

[timotheecour]

I combined OpenDirStatus into WalkStepKind. And I tried to simplify the code further, with limited success though. It's hard to do that with inline iterator using single yield statement.
It seems I've reached my limit here :-/

filewalks (nim-lang/fusion#32) does this; IMO filewalks belongs squarely in stdlib.

[a-mr]

IMHO nim-results should be a part of stdlib for the tasks like this PR.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.