Decaffeinate compiles wrong code if variable is used and first assigned in child-scope

[SimonSimCity]

This is code I found while decaffeinating the project https://github.com/vsivsi/meteor-job-collection:

class JobCollectionBase
  _idsOfDeps: () ->
    dependsQuery = []

    antsArray = []
    @find(
    ).forEach (d) -> antsArray.push(i) for i in d.depends unless i in antsArray
    if antsArray.length > 0
      dependsQuery.push
        _id:
          $in: antsArray
    return dependsQuery

https://decaffeinate-project.org/repl/#?useCS2=true&useJSModules=true&loose=false&evaluate=true&stage=full&code=class%20JobCollectionBase%0A%20%20_idsOfDeps%3A%20()%20-%3E%0A%20%20%20%20dependsQuery%20%3D%20%5B%5D%0A%0A%20%20%20%20antsArray%20%3D%20%5B%5D%0A%20%20%20%20%40find(%0A%20%20%20%20).forEach%20(d)%20-%3E%20antsArray.push(i)%20for%20i%20in%20d.depends%20unless%20i%20in%20antsArray%0A%20%20%20%20if%20antsArray.length%20%3E%200%0A%20%20%20%20%20%20dependsQuery.push%0A%20%20%20%20%20%20%20%20_id%3A%0A%20%20%20%20%20%20%20%20%20%20%24in%3A%20antsArray%0A%20%20%20%20return%20dependsQuery%0A
I get this output:

class JobCollectionBase {
  _idsOfDeps() {
    const dependsQuery = [];

    const antsArray = [];
    this.find(
    ).forEach(function(d) {
      if (!Array.from(antsArray).includes(i)) {
        return (() => {
          const result = [];
          for (let i of Array.from(d.depends)) {
            result.push(antsArray.push(i));
          }
          return result;
        })();
      }
    });
    if (antsArray.length > 0) {
      dependsQuery.push({
        _id: {
          $in: antsArray
        }
      });
    }
    return dependsQuery;
  }
}

Here's what I expect it to be instead:

class JobCollectionBase {
  _idsOfDeps() {
    const dependsQuery = [];

    const antsArray = [];
    this.find(
    ).forEach(function(d) {
      let i;
      if (!Array.from(antsArray).includes(i)) {
        const result = [];
        for (i of Array.from(d.depends)) {
          result.push(antsArray.push(i));
        }
        return result;
      }
    });
    if (antsArray.length > 0) {
      dependsQuery.push({
        _id: {
          $in: antsArray
        }
      });
    }
    return dependsQuery;
  }
}

The key is the line of declaration of the variable i. In the code generated by this project, it's instantiated in a child-scope of where it's declared by the code generated by coffeescript itself. I've modified the output of coffeescript to be more like the code coming from this project to make it easier to spot the difference.

Feel free to change the title of this issue - I just had to come up with something ...

[alangpierce]

Makes sense! To be clear, this code is buggy. This code:

antsArray.push(i) for i in d.depends unless i in antsArray

probably meant to be this:

antsArray.push(i) for i in d.depends when i not in antsArray

In a CoffeeScript array comprehension, you can't use unless or if as a filter; you need to use when. As-is, the code is interpreted as (antsArray.push(i) for i in d.depends) unless (i in antsArray), so the i in antsArray is deciding whether or not to skip the whole loop. i is always undefined because it was never assigned at that point, so presumably it's not in antsArray, so the loop always runs.

You may want to fix that code if you can, but certainly decaffeinate should replicate exactly the CoffeeScript behavior, so the right approach is to use var for the declaration or to declare it earlier.

[SimonSimCity]

I say to declare it earlier - which is the output I get from the coffeescript compiler.

I found out on this out after I decaffeinated the package linked above and some of the tests they had started failing - so it was kind-of expected behavior, at least for the developer who wrote it and the corresponding tests 😊