Lazy load variables from called mixins

[steven-pribilinskiy]

Following code will throw a variable @a is undefined error, which is strange due to the nature of lazy-loaded variables

.m1() { @b: @a; }
.m2() { @a: #aaa; }
.m1();
.m2();

yet everything is fine when the variables are accessed consequently

.m2() { @a: #aaa; }
.m1() { @b: @a; }
.m1();
.m2();

Please clarify

[seven-phases-max]

Yes, this was raised before in #1399 (comment).
I.e. currently compiler always evaluates mixin content on expansion using current context even if (some of) this content is not really used in the mixin itself and thus is a subject for lazy-loading...

I don't know if this can be fixed at all... so not yet adding bug label. But since there's no dedicated ticket for this issue let's have it as the one (though I guess there're mixin expansion order related issues to be referenced here further).

[SomMeri]

Lazy loading normally works because less pretends that all variables are defined at the same time - with later redefinition winning. It is easy to achieve with normal variables, because we can collect them all before using them.

It is harder for variables defined in called mixins, because variables defined by other mixins can be used in guards and as parameters. So the order in which we evaluate mixins may influence which mixin will be called and what kind of variables it returns.

The result depends on mixins evaluation order:

.mixin(@aaa) when (@aaa<10), (@bbb=dummy) {
  @aaa: 111;
}
.mixin(@aaa) when (@aaa>10), (@bbb=dummy) {
  @aaa: 1;
}
.selector {
  .mixin(@aaa);
  .mixin(@aaa);
  value: @aaa;
}
@aaa: dummy;
@bbb: dummy;

In practice, mixins need to be evaluated in some order and the easier the rule is the better. I can imagine more complicated algorithm then just running through them top down or bottom up, but it probably is not worth the effort. We risk getting into cycles and less sheets uncompilable due to guard/parameter dependencies or simply making grammar harder to use when user variables clash.

However, if there is simple solution I just do not see, I will be happy to change opinion and implement it.

[rjgotten]

Lazy loading normally works because less pretends that all variables are defined at the same time - with later redefinition winning. It is easy to achieve with normal variables, because we can collect them all before using them.

It is harder for variables defined in called mixins, because variables defined by other mixins can be used in guards and as parameters. So the order in which we evaluate mixins may influence which mixin will be called and what kind of variables it returns.

To be blunt: this is one more reason to bury the 'variable bubbling' feature behind a legacy config flag and add proper return values to mixins. Return values would have to be assigned to actual variables known at compile time in the parent. That means they can be collected and make use of the same implicit re-ordering 'trick'.

It'd make a world of difference for static code analysis tools as well, because you'd be able to reliably determine existence of variables in scope, where you currently cannot: thanks to the dynamic nature of mixins (evaluation order, mixin guards, etc.) with variable bubbling you never know if (and which version of) a variable gets exposed.

[seven-phases-max]

this is one more reason to bury the 'variable bubbling' feature behind a legacy

Disabling a feature is simply equal to a suggestion like: "do not use such code".

It'd make a world of difference for static code analysis tools as well

This does not sound like something that is worth killing even a single tiny and rare use-case (one of many). It's like "VS Intellisense can't fully recognize this code so let's just remove it from the language", oops.

Return values would have to be assigned to actual variables known at compile time in the parent.

Which simply means we need (native Less language) functions not really "mixins returning values" (though this does not mean that the function definition syntax may not be similar or even the same as of mixins).

But functions won't cancel the need for mixins exposing all if their content to the caller scope (not just properties). As a user I don't really care of other users problems ("Intellisense? pfff... what the heck is it?" ;) and I'm not ready to give up some of my code (like this for example) just to make others to feel more comfortable with their IDEs (yes, I know the same result can be achieved in various ways by other methods w/o such mixins - but I don't care, I just like it the way it already is).

Splitting language into dialects (i.e. "legacy" option) is also a questionable idea - first of all and yet again it's really just the same thing as "do not use such code", but what's more important it multiplies maintenance and support efforts... double tests, double examples, double docs, and all these annoying "what options you compile your code with?" (but I remember I already expressed this opinion for this suggestion in some of earlier issues).

[matthew-dean]

Hmm.... I would see this as a genuine issue, based on our documentation.

But it is a perplexing problem to address. The only thing I can think of is to not immediately throw an error for undefined variables, but return a type of "variable promise" that retains the originating lines. If Less finishes evaluating that enclosing scope (in this case the root) and the promise is unfulfilled, then throw an error. But if it finds a value in the enclosing scope(s) by the end, then you'd assign the value and then probably have to reevaluate the entire tree, substituting the (now) known value for that promise.

Evaluating the tree twice (or more, depending on if assigning the value left other promises unresolved) wouldn't be efficient, but it seems like it would fit the idea of lazy loading, which is to say it evaluates based on when the variable is available, instead of what we actually do, which is what others have described.

Of course, I don't know if that would work at all. It's likely @seven-phases-max will say, "But have you considered X?" which I probably haven't.

@rjgotten I don't know about removing things into legacy, because I'm not sure return vars are the answer either. In a way, that increases complexity in a different way. Right now the caller and callee scope just merge vars, and that's easy to use, even if it's a headache for us, or causes these rare cases that can't be resolved.

I would be more in favor or a (small) language feature that could give more predictable or static behaviors as you suggest, rather than terminate an existing behavior, because it seems, for the most part, it hasn't been confusing to people. (Incidentally, a detached ruleset has completely different behaviors from mixins, and local vars are private, if I recall correctly. That might be something we could expand upon.)

[seven-phases-max]

Just in case a workaround for the initial example is:

foo {
    a: @a;

    .m1() { @b: @a; }
    .m2() { @a: #aaa; }
    .m1();
    .m2(); 
}

@a: I΄m really unused;

Thus "lazy-evaluation" does actually work properly for such code and it's just some kind of bug and/or side-effect where compiler still requires a variable to appear in certain scope even if it never uses this scope to evaluate anything (I feel like there's probably related issue elsewhere with similar example, I can't find it right now though).

---

This obviously does not cancel anything for @SomMeri example, but just shows that those two are actually slightly different examples and the initial one can potentially be solved. (While that of #2543 (comment) can't of course).