[CS2] Nodes: Missing declarations for generated variables in parameter defaults

[connec]

Note: this is for the 2 branch only.

(a = foo() ? bar()) ->

// Generated by CoffeeScript 2.0.0-alpha
(function(a = (ref = foo()) != null ? ref : bar()) {});

Note that ref is used despite there being no var ref anywhere.

A simple patch to fix this is

diff --git a/src/nodes.coffee b/src/nodes.coffee
index 08b0b4e..65fc7cf 100644
--- a/src/nodes.coffee
+++ b/src/nodes.coffee
@@ -2005,10 +2005,11 @@ exports.Code = class Code extends Base
     modifiers.push '*'        if @isGenerator
 
     signature = [@makeCode '(']
+    paramO    = extend o, scope: o.scope.parent
     for param, i in params
       signature.push @makeCode ', ' if i
       signature.push @makeCode '...' if haveSplatParam and i is params.length - 1
-      signature.push param.compileToFragments(o)...
+      signature.push param.compileToFragments(paramO)...
     signature.push @makeCode ')'
 
     body = @body.compileWithDeclarations o unless @body.isEmpty()

I'm not sure though if such functions should also be compiled in a safety wrapper, though

(function () {
  var ref
  return (function(a = (ref = foo()) != null ? ref : bar()) {});
})()

[connec]

Alternatively we could fall back to the old handling in such cases, which would give us a better scope for these temporary variables.

(function(a) {
  var ref
  if (a === undefined) {
    a = (ref = foo()) != null ? ref : bar()
  }
})

[connec]

A final option for this:

(function(a = (() => {
    var ref
    return (ref = foo()) != null ? ref : bar()
  })()) {})

I reckon falling back is the nicest option.

Regardless, to fix this we need to be able to detect when an expression will need to be cached. Hopefully Node#isComplex will be up to the task.

[GeoffreyBooth]

When would this really happen? I don’t see when I would put a whole expression in a function parameters list.

[connec]

I guess it's less about when it would happen, and the fact that it's allowed to happen incorrectly (i.e. the compiler generates invalid Javascript).

I can't remember the context I discovered it in, but I suspect it was whilst generating contrived examples for class tests 😄

Anyway, I suspect that fixing this would be easier and more maintainable than properly disallowing it (i.e. making it not parse and/or validating the content of Param nodes). Also it is allowed in JS.

[connec]

The patch for this is almost identical to the one used to fix #4426 - in both cases it feels like there's a missing scope (parameter default scope, in this case, and class body scope, in the case of #4426).

Of course, syntactically, it's impossible to declare variables in parameter defaults or class bodies, so we have to choose whether to hoist such declarations to the enclosing scope (potentially polluting it with a bunch of temporary variables), or declare them in an IIFE wrapper (causing 'ugly' output, and potentially impacting performance).

My feeling is that we should wrap classes, and hoist parameter defaults.

i = 0
class B extends A
  "#{++i}": (a = foo() ? bar()) -> super()
# ...
# B = (function () {
#   var name, ref
#   return class B extends A {
#     [name = `${++i}`] (a = (ref = foo()) != null ? ref : bar()) {
#       super[ref]()
#     }
#   }
# })()

Ultimately most of these cases are pretty esoteric, but I think it's important that we maintain the consistency of the language (i.e. everything is an expression).

[GeoffreyBooth]

You’re the expert on hoisting, do you feel like taking this on? I would only add that another goal is to output idiomatic ES whenever possible, such as outputting ES parameter default values with the parameter and not in the function body. We don’t want to go too far into handling edge cases that we generate convoluted (and potentially buggy) JavaScript when it isn’t necessary.

[GeoffreyBooth]

I’m looking at this again, and I think that perhaps there isn’t actually a bug here. Consider when we modify your original example:

// (a = foo() ? bar()) ->
(function(a = (ref = foo()) != null ? ref : bar()) {});

to:

// (a = foo() ? bar()) -> ref
(function(a = (ref1 = foo()) != null ? ref1 : bar()) {
  var ref1;
  return ref;
});

(I’m on the destructuring branch.) Clearly the compiler is tracking the scope of the generated variable ref, and it’s tracked as a parameter-sope variable like a. So just as we don’t (and shouldn’t) get a var a line inside the function, there also need not nor should not be a var ref/var ref1 line. Both ref/ref1 and a are declared by virtue of being function parameters, just like a in this very minimal example:

// (a) -> a
(function(a) {
  return a;
});

Am I missing something here, or are we okay?

[connec]

So it looks like ref is being declared now, which fixes any runtime errors. - See below update.

On the broader point of declaring it in the function or not... Since these refs are free variables, I don't think there's a problem declaring them in the outer scope (besides that you could end up with lots of temporary variables in your outer scope).

I think the main concern for me was that this is a change from CS1, wherein such ref variables would be in the function body, which feels like a better location for them. The cost of doing it this way is we'd have to detect parameter defaults that need refs (.shouldCache) and demote them to the function body.

[GeoffreyBooth]

Well they were in the function body in CS1 because almost anything complicated or unusual in function parameters got pushed into the function body. Now with CS2 trying to honor default values and destructuring in the parameters as opposed to the body, we necessarily will have some complicated stuff up there. I don't think we should be redeclaring variables in a function body that we've already declared as function parameters.

I'm leaning toward closing this issue, unless there's something I'm missing?

[connec]

OK, the behaviour was a bit more complicated than I initially thought. However, consider the following CoffeeScript:

'use strict'
a = ->
b = -> 1
c = (d = a() ? b()) ->
c()

This is currently compiled to:

// Generated by CoffeeScript 2.0.0-alpha1
var a, b, c;

a = function() {};

b = function() {
  return 1;
};

c = function(d = (ref = a()) != null ? ref : b()) {};

c();

Notice that ref is not declared anywhere. This will appear to work if run with node regularly, however if run in strict mode we get:

c = function(d = (ref = a()) != null ? ref : b()) {};
                      ^

ReferenceError: ref is not defined

[connec]

The solution I proposed in the first post was ensuring that ref was declared in the enclosing scope (e.g. with a, b, and c in the example above).

An easier and (imo) better solution would be to demote shouldCache expressions from the parameter list, and compile them in the body as before.

[GeoffreyBooth]

An easier and (imo) better solution would be to demote shouldCache expressions from the parameter list, and compile them in the body as before.

I’m fine with this, though we would have to make sure that not all destructured parameters are shouldCache—but that destructured params that need it, like ({ a = b() }) ->, still are.

[connec]

Yes, I agree. That'll be a fun one 😛

[GeoffreyBooth]

I did a little digging into this one. It turns out that ref is getting defined if the function has a body. And in such cases, the ? operation appears to work. Example:

foo = -> null
bar = -> 33
f = (a = foo() ? bar()) -> a
f() # 33

---

var bar, f, foo;
// ...
f = function(a = (ref = foo()) != null ? ref : bar()) {
  var ref;
  return a;
};

But since ref is declared inside the body, this still fails in strict mode. Working on a fix.

[GeoffreyBooth]

See #4640. I tried to do an implementation that compiled the parameter “prematurely,” just to figure out if it caused the creation of any generated variables and if so, to shift it into the function body; but I couldn’t find a safe way to do so. So in #4640 I went for a solution closer to your original suggestion: if the compilation of a function parameter results in new generated variables, I just shift those variables from the function scope into the parent scope. Since the function scope is already a child of the parent scope, I don’t need to worry about name collisions.

So that solves this immediate issue, and I guess we can tackle the broader implications of expressions in function parameters when the next issue arises. But for now I’d like to stick with the simpler solution (assuming it works as I think it does) because the generated JS is closer to the author’s intent, as the expression is in the function parameter just like in the original CoffeeScript.

[GeoffreyBooth]

Fixed by #4640.