Evaluating @arguments

[Justineo]

Hi guys.

For the following Less code:

.mixin(@param) {
  @x: @arguments;

  .output(@whatever) {
    x: @x;
  }

  .output(unexpected);
}

.test {
  .mixin(expected);
}

I would expect the rendered CSS to be like this:

.test {
  x: expected;
}

But actually it is:

.test {
  x: unexpected;
}

I know that Less lazy loads variables but this still seems weird to me. Shouldn't things like @arguments and @rest be evaluated before it goes into deeper scope?

[seven-phases-max]

I added "Bug" label at first, but I'm quite confused now. Notice that it not so different from the second example in Lazy Loading. Also it's not limited to only @arguments variable and/or ~"" stuff. It' s for any variable inside .output:

.mixin(@param) {
  @x: @arguments @param;
  .output(@param) {
    x: @x;
  }

  .output(unexpected);
}

.test {
  .mixin(expected); // unexpected unexpected
}

Just in case I also tested earlier versions (down to 1.4.2) and it's always been like this.

---

P.S.
I guess the most confusing part is probably that if we move the @x definition out of the .mixin it starts to look like the variable is evaluated immediately, e.g.:

@x: @arguments @param; // Error: @arguments is undefined

.mixin(@param) {
  .output(@param) {
    x: @x;
  }

  .output(unexpected);
}

However if you add those undefined variables there they yet again trigger the deepest evaluation back (so in the previous example it actually complains about unused variables :)

@arguments:    foo;
@param:        bar;
@x: @arguments @param;

.mixin(@param) {
  .output(@param) {
    x: @x; // no, not foo and bar
  }

  .output(unexpected);
}

[Justineo]

Yes it seems to be consistent with lazy loading but still weird...
In common cases when we use @arguments, we are actually expecting it will be evaluated within current scope. In my actual code I can refactor a little bit to avoid this but the logic here is quite confusing.

[Justineo]

@seven-phases-max Like you said it's not limited to @arguments. But in other cases I can avoid the unexpected behavior by naming parameters in deeper scopes different from variables from upper scopes. @arguments is automatically generated in each scope and so this kind of workaround won't work.

[seven-phases-max]

@Justineo

Since I mentioned this issue in #2433 (comment), I feel like I need to provide an alternative answer now.

Regardless of how we'll decide to treat that (change or not) the root of the problem in the example is of course the use of the caller variable in the called mixin, in Less it's a perfect way to shoot yourself in the foot. Such cases always should be taken with care and avoided when possible (the problem is that there're too many directly contradictory scope handling expectations and already implemented rules so it will always have "issues").

So yes, even if we decide to change it for @arguments it's still better to refactor your code to pass caller variables to the mixin explicitly (just to feel yourself safer :), e.g.:

.mixin(@param) {
    .output(@x, @whatever) {
        x: @x;
    }

    .output(@arguments, unexpected);
}

.test {
    .mixin(expected);
}

[rjgotten]

More confirmation for what has been on my mind for some time now:
LESS's attempts at being clever with lazy evaluation and its often nonsensical and non-orthogonal/non-uniform scoping rules are not a good thing.

Of late there have been far too many instances of people shooting themselves in the foot or the compiler containing actual bugs, all in major part due to trying to cope with the quirks and exceptions in these rules. Perhaps it's time to make work of missing crucial features such as lambdas and function return values and just switch to passing by parameter, closure scope or return value only: neat, nice and above all predictable.

As for a migration path and backwards compatibility: we already have strict-units and strict-math. There is no reason we cannot have strict-scope as well.

[seven-phases-max]

@rjgotten

Well, strictly speaking, Less scoping rules are uniform and strict. Even the most famous #1316 (that applies here as well) is not caused by actually different (or some "clever") scoping rules but is only a result of different "use" and "meaning" of plain rulesets and parametric mixins (I explained it in details here).
The only thing that may be considered "clever" in this context is "variables expanded by mixin do not override existing variables" but it's pretty much issue-free feature (and it's not like "it works this way in this case and another way in that case").

Perhaps it's time to make work of missing crucial features such as lambdas and function return values

Ready to make a PR for all this? ;) OK, just killing. But in this context I'd like to remind that initially Less is meant to be not programming language at all (in classical sense of this term, just like CSS isn't). And while recently it's got some of such functionality, expecting all these not-even-considered-initially features (callbacks, lamdas, functions, OOP classes) to appear instantly and fit like a charm out-of-the-box into existing facilities is rather strange.

P.S.

There is no reason we cannot have strict-scope as well.

There's actually, you'll wonder to find out how few Less users use mixins and are aware of scope (or care of these things at all, for 90% of users Less code is just "bunch of global variables + nesting" that's it). So implementing those different (from current) scoping rules enabled by some option (which is basically equal to having two different languages in the same compiler) sounds like a total overkill (that thing will be simply unmaintainable).
Or in other words, an approach of "implement what I need, redesign what I don't like, throw away what I don't need, provide an option to disable all that so it won't break existing code and it's all solved" just won't work. Small incremental changes/fixes/improvements is all we have. The only alternative is developing another language and its compiler from scratch.

[rjgotten]

an approach of "implement what I need, redesign what I don't like, throw away what I don't need, provide an option to disable all that so it won't break existing code and it's all solved" just won't work.

Well; lambdas and real mixin return values are general additions that don't change existing behavior. The scoping rules are the only real change: as far as I've been able to tell, mostly everything is already using normal closure scope, but pulls in stuff from other scopes in addition to that. A strict-scope switch would only turn that behavior off and nothing else.

Ready to make a PR for all this? ;)

Sorry; kind of busy with @import (plugin) already. ;-)

[seven-phases-max]

Yes, sure, lambdas and mixin return values won't be affected by and/or won't change scoping rules, hence strict-scope option won't make any sense for them anyway... So my comment is mostly for those feature-request/issues that scope-priority related issues.

A strict-scope switch would only turn that behavior off and nothing else.

Just turning caller scope off? OK, makes sense then... to solve this one (Sorry, I was thinking you also mean other scope issues).

[seven-phases-max]

After some thinking I'm about closing this (feel free to reopen).
As already mentioned the result is expected because of lazy-evaluation (aka lazy-loading), the value of @arguments is evaluated when x: @x is rendered (and not when @arguments is assigned to @x) hence the resulting value is .output argument instead of .mixin. No caller/parent precedence quirks are actually involved here.

And while it would seem to be natural to make an exception for @arguments because of its special meaning, in general this would actually mean introducing that uncertain and inconsistent evaluation hack (also mentioned above) and bringing just more chaos in.
So in conclusion: "lazy-evaluation" is "lazy-evaluation". No exceptions.

---

For the particular example of the initial post this means it should not rely on the special @arguments meaning, but use explicit variable instead, e.g.:

.mixin(@parameters...) {
    @x: @parameters;

    .output(@whatever) {
        x: @x;
    }

    .output(unexpected);
}

.test {
    .mixin(expected);
}

[seven-phases-max]

Closing as expected behaviour.