Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
SimonSimCity opened this issue May 2, 2018 · 2 comments
Open

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

SimonSimCity opened this issue May 2, 2018 · 2 comments
Labels
incorrect-output

Comments

@SimonSimCity
Copy link

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 ...
SimonSimCity added a commit to SimonSimCity/meteor-job-collection that referenced this issue May 2, 2018
@SimonSimCity
Carefully compared the output of coffee -c and `decaffeinate-projec…
1037508 
…t.org` and fixed bug in code generated by decaffeinate-project.org

* Bug reported at decaffeinate/decaffeinate#1313 - no follow up needed.
@SimonSimCity SimonSimCity mentioned this issue May 2, 2018
Open
@alangpierce
Copy link
Member

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
Copy link
Author

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 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
incorrect-output
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alangpierce @SimonSimCity