How do you guys feel about variable shadowing?

I'm playing with implementing pull streams in #racket after talking to @punkmonk. I'm using a combination of reading the docs and the source to figure out how something works.

The source mentioned above is very terse:

module.exports = function filter (test) {
  //regexp
  test = tester(test)
  return function (read) {
    return function next (end, cb) {
      var sync, loop = true
      while(loop) {
        loop = false
        sync = true
        read(end, function (end, data) {
          if(!end && !test(data))
            return sync ? loop = true : next(end, cb)
          cb(end, data)
        })
        sync = false
      }
    }
  }
}

Notice the variable shadowing where function next has an argument called end while the inner nameless function inside read() also has an argument called end that effectively shadows the previous one with the same name. This means that there are two separate variables with the same name in this function. I find it a bit harder to read when this happens, and often need to stop to think what end I'm thinking about when trying to understand what is going on.

I don't think this is a pro or against situation, I just wanted to see if other people here find these shadows a bit confusing as well.

I never use it, so I’m not used to expect a value to change during its life cycle.

It is effectively a new variable with it's own little allocation with the same name as other nearby variable. It is not a rewrite of the value of a single variable, which would be easier to reason about. Basically, that inner end could be named something else and everything would behave exactly the same, it would just be more readable.

This is not a bug, this is working as intended. It just trips me now and then. I've accidentally done the same stuff before...

@andrestaltz📱 just to be clear, I'm not talking about bugs. There is no bug, it is just a readability issue that makes it easy to misjudge the value of a variable in your head.

There is sometimes a convention to have the outer next function use abort as the variable name instead of end.
e.g. the following from pull-stream-examples:

function map (mapper) {
  //a sink function: accept a source
  return function (read) {
    //but return another source!
    return function (abort, cb) {
      read(abort, function (err, data) {
        //if the stream has ended, pass that on.
        if(err) return cb(err)
        //apply a mapping to that data
        cb(null, mapper(data))
      })
    }
  }
}

or this from patchfoo:

function catchTextError() {
  return function (read) {
    var ended
    return function (abort, cb) {
      if (ended) return cb(ended)
      read(abort, function (end, data) {
        if (!end || end === true) return cb(end, data)
        ended = true
        cb(null, end.stack + '\n')
      })
    }
  }
}

Part of what makes the referenced code from pull-stream/throughts/filter harder to follow I think is that it includes protection against stack growth and callbacks potentially being called synchronously. Related: dezalgo - and the package looper mentioned by @dominic in %sBL+I3p... - or pull-looper. That is why there is a while loop in there when it could otherwise apparently be written more simply.