mint icon indicating copy to clipboard operation
mint copied to clipboard

Generated function argument names conflict with inlined JS

Open bbugh opened this issue 3 years ago • 7 comments

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..

bbugh avatar May 31 '21 14:05 bbugh

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.

gdotdesign avatar May 31 '21 14:05 gdotdesign

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.

bbugh avatar May 31 '21 14:05 bbugh

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.

Sija avatar Jun 01 '21 10:06 Sija

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

I don't understand what you mean by that.

gdotdesign avatar Jun 01 '21 15:06 gdotdesign

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

Sija avatar Jun 01 '21 15:06 Sija

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.

gdotdesign avatar Jun 01 '21 15:06 gdotdesign

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.

Sija avatar Jun 01 '21 16:06 Sija