Add Optional JavaScript 'let' variables #13851

juancarlospaco · 2020-04-02T22:29:09Z

Add compile-time optional JavaScript let variable declarations.
https://caniuse.com/let
Use -d:jsLet for let variable declarations, otherwise var is used.
Simple code, 0 lines added, everything is done at compile-time.
This may not be perfect but using codegenDecl everywhere is not better.

I try compiling Karax examples and they work just Ok. NodeJs also works Ok.

We can try enabling it on Nightly or Betas, if no bug are reported for a while it can be switched to default, or just leave it as optional, is a nice option to have for Frontend people.

ghost · 2020-04-02T22:37:59Z

Are you sure that this won't break things?
"let allows you to declare variables that are limited to a scope of a block statement, or expression on which it is used, unlike the var keyword, which defines a variable globally, or locally to an entire function regardless of block scope."

Araq · 2020-04-03T09:27:59Z

What's the point? JS JIT engines should have no problem optimizing var the same way they optimize let.

timotheecour · 2020-04-06T01:38:24Z

new defined symbols must start with nim prefix: jsLet => nimJsLet
I wish we could introduce a test in CI that looks for new defined symbols in the diff and checks that
let is cleaner since it doesn't have global scope (less pollution etc; could be a an even bigger problem for .inject symbols? but I haven't checked whether this would introduce undesirable change in semantics
how about --experimental:jslet which allows localizing it

juancarlospaco · 2020-04-06T02:26:22Z

@Araq Problem is that is easily reversible:
JS JIT engines should have no problem optimizing let the same way they optimize var. 🤷‍♂️

var is kinda global, let is scoped, making the compiler generate better code step-by-step.

JavaScript user base is the biggest basically, attract them here may be good for Nim.

@timotheecour Maybe .inject can have something like (pseudocode):

when defined(js) and defined(nimJsLet): 
  {.warning: "JavaScript target uses 'let', injected variables are not global".}

Help prove me wrong, break it, I also try to break it, but it seems that Nim codegen is good,
and do not relies too much on mutating global variables.

Araq · 2020-04-06T10:02:41Z

Help prove me wrong, break it, I also try to break it, but it seems that Nim codegen is good,
and do not relies too much on mutating global variables.

So simply enable it all the time.

timotheecour · 2020-04-08T02:57:46Z

changelog.md

@@ -10,10 +10,11 @@

 ## Compiler changes

+- Specific warnings can now be turned into errors via `--warningAsError[X]:on|off`.
+- The `define` and `undef` pragmas have been de-deprecated.
 - JavaScript backend adds compile-time optional `let` variable declarations,


weirdly worded, how about:
JavaScript backend now uses let instead of var declarations; for previous behavior, compile with -d:nimJsVar; see also https://caniuse.com/let

timotheecour · 2020-04-08T03:01:19Z

changelog.md

@@ -12,6 +12,9 @@

 - Specific warnings can now be turned into errors via `--warningAsError[X]:on|off`.
 - The `define` and `undef` pragmas have been de-deprecated.
+- JavaScript backend adds compile-time optional `let` variable declarations,
+  compile Nim with `-d:nimJsVar` for `var` and without for `let`,


instead how about using the standard flag --legacy which is exactly for this purpose;
we already have: --legacy:allowSemcheckedAstModification|checkUnsignedConversions

we'd also now have:
--legacy:jsUseVar or --legacy:jsvar

timotheecour · 2020-04-08T03:04:35Z

compiler/jsgen.nim

@@ -1655,15 +1655,15 @@ proc genVarInit(p: PProc, v: PSym, n: PNode) =
    varCode: string
    varName = mangleName(p.module, v)
    useReloadingGuard = sfGlobal in v.flags and p.config.hcrOn
-
+  const jsVarDecl = when defined(nimJsVar): "var" else: "let"


this makes no sense. Why do you need to recompile nim (aka different dialects) just to change nim's behavior?
instead either use isDefined(conf, "nimJsVar") or, better, use the suggestion i gave above with --legacy:jsUseVar

I like it compile time, because either it always works or never works,
and I dont see people switching back to var if compiler uses let already.
But ok.

timotheecour · 2020-04-09T00:25:05Z

compiler/jsgen.nim

  if v.constraint.isNil:
    if useReloadingGuard:
-      lineF(p, "var $1;$n", varName)
+      if jsVarDecl: lineF(p, "var $1;$n", varName) else: lineF(p, "let $1;$n", varName)


here + elsewhere, please make it more DRY:
introduce this once: let jsdecl = if jsVarDecl: "var" else: "let"
then use it everywhere relevant eg:

lineF(p, "$1 $2;$n", [jsdecl, varName])

code repetition = WET (all acronyms of which apply)

timotheecour · 2020-04-09T00:26:42Z

compiler/jsgen.nim

      lineF(p, "if ($1 === undefined) {$n", varName)
      varCode = $varName
      inc p.extraIndent
    else:
-      varCode = "var $2"
+      varCode = if jsVarDecl: "var $2" else: "let $2"


varCode = jsdecl & " $2"

timotheecour · 2020-04-09T00:28:11Z

compiler/jsgen.nim

@@ -1673,8 +1673,12 @@ proc genVarInit(p: PProc, v: PSym, n: PNode) =
  if n.kind == nkEmpty:
    if not isIndirect(v) and
      v.typ.kind in {tyVar, tyPtr, tyLent, tyRef, tyOwned} and mapType(p, v.typ) == etyBaseIndex:
-      lineF(p, "var $1 = null;$n", [varName])
-      lineF(p, "var $1_Idx = 0;$n", [varName])
+      if jsVarDecl:


ditto: no branching needed:

lineF(p, "$1 $2 = null;$n", [jsdecl, varName]) lineF(p, "$1 $2_Idx = 0;$n", [jsdecl,varName])

timotheecour · 2020-04-09T00:28:27Z

compiler/jsgen.nim

@@ -1702,7 +1706,11 @@ proc genVarInit(p: PProc, v: PSym, n: PNode) =
      else:
        if targetBaseIndex:
          let tmp = p.getTemp
-          lineF(p, "var $1 = $2, $3 = $1[0], $3_Idx = $1[1];$n",
+          if jsVarDecl:


timotheecour · 2020-04-09T00:53:26Z

the failure for tests/js/tfieldchecks.nim indicates this needs more care. It's great that we have a test suite that catches this. I think the idea is sound but we need to be more careful, after fixing it please add a test showing multiple cases of interest to make sure things don't break
after your PR result still generates var

proc fun1(): int = result = 11
  doAssert fun1() == 11

after your PR some constructs (eg for loops) still generate var instead of let eg:

let a = @[1,2]
echo a

reduced case that fails after your PR:

when true:
  block: # works without block
    var flag = false
    proc wrap(): int =
      discard flag
      result = 12
    doAssert wrap() == 12

juancarlospaco added 2 commits April 2, 2020 18:46

Add Optional JavaScript codegen 'let' variable declaration

3601607

Add Optional JavaScript codegen 'let' variable declaration

1d62fa0

Peer Review Feedback #13851 (comment)

35d73e3

timotheecour reviewed Apr 8, 2020

View reviewed changes

juancarlospaco added 2 commits April 8, 2020 18:29

https://github.com/nim-lang/Nim/pull/13851#discussion_r405229935

233af79

ReSync

22fe4ad

timotheecour reviewed Apr 9, 2020

View reviewed changes

juancarlospaco closed this Apr 15, 2020

timotheecour mentioned this pull request Apr 15, 2020

misc nim js timotheecour/Nim#95

Open

1 task

timotheecour added the stale Staled PR/issues; remove the label after fixing them label Jun 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Optional JavaScript 'let' variables #13851

Add Optional JavaScript 'let' variables #13851

juancarlospaco commented Apr 2, 2020 •

edited

Loading

ghost commented Apr 2, 2020

Araq commented Apr 3, 2020

timotheecour commented Apr 6, 2020 •

edited

Loading

juancarlospaco commented Apr 6, 2020

Araq commented Apr 6, 2020

timotheecour Apr 8, 2020 •

edited

Loading

timotheecour Apr 8, 2020 •

edited

Loading

timotheecour Apr 8, 2020

juancarlospaco Apr 8, 2020

timotheecour Apr 9, 2020 •

edited

Loading

timotheecour Apr 9, 2020

timotheecour Apr 9, 2020

timotheecour Apr 9, 2020

timotheecour commented Apr 9, 2020 •

edited

Loading

Add Optional JavaScript 'let' variables #13851

Add Optional JavaScript 'let' variables #13851

Conversation

juancarlospaco commented Apr 2, 2020 • edited Loading

ghost commented Apr 2, 2020

Araq commented Apr 3, 2020

timotheecour commented Apr 6, 2020 • edited Loading

juancarlospaco commented Apr 6, 2020

Araq commented Apr 6, 2020

timotheecour Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

timotheecour Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

timotheecour Apr 8, 2020

Choose a reason for hiding this comment

juancarlospaco Apr 8, 2020

Choose a reason for hiding this comment

timotheecour Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

timotheecour Apr 9, 2020

Choose a reason for hiding this comment

timotheecour Apr 9, 2020

Choose a reason for hiding this comment

timotheecour Apr 9, 2020

Choose a reason for hiding this comment

timotheecour commented Apr 9, 2020 • edited Loading

juancarlospaco commented Apr 2, 2020 •

edited

Loading

timotheecour commented Apr 6, 2020 •

edited

Loading

timotheecour Apr 8, 2020 •

edited

Loading

timotheecour Apr 8, 2020 •

edited

Loading

timotheecour Apr 9, 2020 •

edited

Loading

timotheecour commented Apr 9, 2020 •

edited

Loading