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.

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

Generated function argument names conflict with inlined JS #452

Open
bbugh opened this issue May 31, 2021 · 7 comments
Open

Generated function argument names conflict with inlined JS #452

bbugh opened this issue May 31, 2021 · 7 comments
Labels

Comments

@bbugh
Copy link
Contributor

bbugh commented May 31, 2021

The generated function arguments m, n, o can conflict with the actual JS values (particularly n which is not uncommon as a variable name), causing weird errors.

Given a module:

module Example {
  fun show(something : a, list: b, value: c) {
    `
    (() => {
      for (let n = 0; n < #{list}.length; n++) {
        console.log(#{list}[n])
      }
    })()
    `
  }
}

This generates:

const AH = new(class extends _M {
  b(n, m, o) {
    return ((() => {
          for (let n = 0; n < m.length; n++) {
            console.log(m[n])
          }
        })());
  }
});

The n defined in the JavaScript code is shadowing the n defined as the minified argument name.

It seems like these generated arguments should be ones that cannot conflict, perhaps with an index/uuid like n9208, or __0__, __1__, etc..

@gdotdesign
Copy link
Member

It seems like these generated arguments should be ones that cannot conflict, perhaps with an index/uuid like n9208, or __0__, __1__, etc..

That would increase the size of the generate JS a lot since there are more variables generated from Mint code then from inlined code.

I usually prefix the inlined variables with $ in cases like this.


The proper solution to this would be something which Crystal has: https://crystal-lang.org/reference/syntax_and_semantics/macros/fresh_variables.html

Something like this:

module Example {
  fun show(something : a, list: b, value: c) {
    `
    (() => {
      for (let #{%n} = 0; #{%n} < #{list}.length; #{%n}++) {
        console.log(#{list}[#{%n}])
      }
    })()
    `
  }
}

and would generate this (next available variable name):

const AH = new(class extends _M {
  b(n, m, o) {
    return ((() => {
      for (let p = 0; p < m.length; p++) {
        console.log(m[p])
      }
    })());
  }
});

Implementation wise all occurrences of a variable (%n) need to compile to the same JS variable in the scope of the inline JS not in the scope of all inline JS.

@bbugh
Copy link
Contributor Author

bbugh commented May 31, 2021

Another situation resulted in arguments generated as i, which was funny because I renamed it to i to avoid n. 😅

Thanks for the explanation. I can add a warning to the docs to avoid single letter variable names.

@Sija
Copy link
Member

Sija commented Jun 1, 2021

I'd vote for autoprefixed variable names (interpolated ones). Otherwise it's gonna be tricky as hell to debug such issues, and fresh variables doesn't seem to give any advantage over autoprefixing, and OTOH they introduce another concept needed to be learned.

@gdotdesign
Copy link
Member

I'd vote for autoprefixed variable names (interpolated ones).

I don't understand what you mean by that.

@Sija
Copy link
Member

Sija commented Jun 1, 2021

I meant that the variable names should be prefixed to avoid clashing with already defined (usually single-lettered) variables.

@gdotdesign
Copy link
Member

gdotdesign commented Jun 1, 2021

I meant that the variable names should be prefixed to avoid clashing with already defined (usually single-lettered) variables.

If we prefix every variable that the compiler produces it will significantly increase the size of the compiled code. Fresh variables are the proper way of fixing it, since they will take up a variable name slot.

OTOH they introduce another concept needed to be learned.

I think it needs to be documented properly - both fresh variables and manual prefixing - and we will be fine.

@Sija
Copy link
Member

Sija commented Jun 1, 2021

Fresh variables are the proper way of fixing it, since they will take up a variable name slot.

hmm, I still don't see this as a solution, since, theoretically every interpolated variable name within the inline JS body could clash with already defined ones, so in order to be safe you'd need to always use fresh variables, and if that's the case why not do this automatically and always be sure we're not colliding. Even using a $ character as a prefix would help here - at a cost of additional 1 character per variable in the output js, but if that's the price for the safety - and only for inlined JS - it might be worth paying it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants