Unify Mixin and DR behaviour (and related Functions stuff)

seven-phases-max commented 8 years ago

As dissucussed in #2767, current DR and Mixin call result (visibility of their internal entities in the caller scope) is somewhat inconsistent. In particular, mixin calls exposes both internal mixins and variables, but DR calls keep its variables hidden:

Mixin:

.my-red-theme() {
    .button() {
        color: red;  
    }

    @color: red;
}

.btn {
    .my-red-theme();
    .button();            // OK
    border-color: @color; // OK
}

DR:


@my-red-theme: {
    .button() {
        color: red;  
    }

    @color: red;
};

.btn {
    @my-red-theme();
    .button();            // OK
    border-color: @color; // Error
}

The proposal is to match both (per #2767 by exposing DR variables too).

The exposed variables ("callee variables") have the lowest priority (i.e. never override caller's local ~~or parent scope variables), thus the change will not break any existing Less code.~~

Edit: My mistake above: callee variables do override caller's parent (incl. global) variables, thus the change is breaking for snippets like:

@my-red-theme: {
    @color: red;
};

@color: blue;

.btn {
    @my-red-theme();
    color: @color; // blue -> red
}

matthew-dean commented 8 years ago

👍

matthew-dean commented 8 years ago

Is there any logic to going the other way in 3.0? Just in terms of simplifying the compiler, it seems like Less sometimes has to do a number of evaluation passes to settle all the variables exposed to different scopes. If you had to design variable scope from scratch in Less, would you do it this way?

matthew-dean commented 8 years ago

Also, I think this was discussed elsewhere, but it might be a good opportunity to change the definition of "Detached Ruleset" to "Anonymous Mixin", with a change like:

// silly example but here ya go
@my-red-theme: (@newcolor) {
    @color: @newcolor;
};

.btn {
    @my-red-theme(red);
    border-color: @color; // red
}

seven-phases-max commented 8 years ago

"Anonymous Mixin"

"Anonymous Ruleset" probably, "mixin" would be somewhat...

matthew-dean commented 8 years ago

I just mean if we were to allow someone to pass in parameters as a mixin (and we're aligning variable behavior with mixins), then it would be an anonymous mixin.

matthew-dean commented 8 years ago

One thing about variables: from this section, http://lesscss.org/features/#mixins-as-functions-feature, this part is baffling to me:

Variables and mixins defined in a mixin are visible and can be used in caller's scope. There is only one exception, a variable is not copied if the caller contains a variable with the same name (that includes variables defined by another mixin call). Only variables present in callers local scope are protected. Variables inherited from parent scopes are overridden.

What is the rationale for this? I didn't realize that a mixin had exceptions where it didn't copy a variable. Isn't it inconsistent that it mixins variables some times and not others? And, as noted by the documentation, it doesn't protect variables from the parent scope.

Also, because of this behavior, you get add-on weirdness like this:

.mixin1() {
  @a: 1;
}
.mixin2() {
  @a: 2;
}
.scope {
  .mixin1();
  .mixin2();
  val: @a;
}
// Output
.scope {
  val: 1;
}

In my mind, this would be simpler to define that mixins either add variables to the scope, or they don't. This exception just makes for a confusing mess of behaviors.

Can you think of a reason why this behavior is justified?

matthew-dean commented 8 years ago

Per @rjgotten's comment here: https://github.com/less/less.js/issues/538#issuecomment-249646504

...I'm wondering if we should not match mixin behavior to DRs, rather than the other way around. That is, isolate variables within the scope of mixins, if you can solve return variables with a declarative syntax. If you can explicitly pass in variables to a mixin, and explicitly return variables, it seems like that would greatly simplify evaluation if the internal variables were isolated, because the mixin would always return the same results, given the same values, regardless of scope.

That is, ~~this would throw an error~~ this would maybe throw an error. There are a collection of scoping rules, and I noticed that DRs, when called, can still see the caller's scope. So maybe mixins/DRs would retain that behavior and not throw an error with the following:

.mixin() {
  bar: @a;
}
.test {
  @a: foo;
  .mixin();   // undefined @a
}

This would not throw an error (variable definitions in scope of mixin definition):

@a: foo;
.mixin() {
  bar: @a;
}
.test {
  .mixin();   // that's fine
}

Like detached rulesets, this would throw an error:

.mixin() {
  @a: foo;
}
.test {
  bar: @a;  // undefined @a
  .mixin();  
}

BUT with @rjgotten's syntax, you could do:

.mixin(): @a {
  @a: foo;
}
.test {
  bar: .mixin();  // we're all good
}

Breaking behavior is not always great. But.... mixin scope and the explanation of caller/callee scope is not a simple concept in Less. My guess is that caller scope is not a relied-upon behavior (maybe), and likely more often than not creates unexpected side-effects. And we're already on the page of recognizing that having different scoping rules for DRs and mixins is not ideal. So maybe let's go the other way, since isolating variable scope is way easier to reason about.

_EDIT: As noted above, we could isolate variables/mixins from returning to the caller scope, but it's possible we can/should retain visibility of caller scope from within the mixin call. I didn't realize when I first wrote this up that that's current DR behavior._

This would also eliminate counter-intuitive patched-on rules like "Variables defined directly in callers scope cannot be overridden. However, variables defined in callers parent scope is not protected and will be overridden." Basically, variables in caller's scope would always be safe.

matthew-dean commented 8 years ago

Just to see how this would affect the documentation, I found that most of the "Mixins as functions" documentation could be eliminated with this change. See: https://github.com/less/less-meta/blob/master/proposal/mixins-as-functions.md

Also, considering what I've seen in the ruleset evaluation step of Less.js, I think this could give a performance boost, because it would reduce the amount of splicing / array flattening that currently happens, as well as variable lookup time (because of fewer variables and fewer loop iterations), whereas increasing variable visibility for DRs would theoretically reduce performance. Also, one advantage of @rjgotten's syntax as opposed to other proposals is that the mixin doesn't have to be evaluated to determine that a value other than a ruleset will be produced. The mixin can be flagged statically when it's defined.

rjgotten commented 7 years ago

the mixin doesn't have to be evaluated to determine that a value other than a ruleset will be produced. The mixin can be flagged statically when it's defined.

I.e. Mixin-as-mixin calls could early-out on all signatures that were flagged by the parser as having an out-parameter. And vice-versa; mixin-as-function calls could early-out on all signatures that were flagged as not having an out-parameter. Both at (virtually) no additional cost.

This and the fact that it plays nicely with Less's declarative nature are exactly the two primary reasons I proposed this particular syntax.

matthew-dean commented 7 years ago

I have an alternate suggestion for this thread.

This happened to come to me while I was working this week on finishing up 3.0 for release. (It's now in alpha if you can please check it out.)

It occurred to me: Instead of being able to call mixins from anywhere and use them as functions (i.e. an "out" value), I think we've already effectively solved this with the discussion in #12 (e.g. https://github.com/less/less-meta/issues/12#issuecomment-230340920)

That is: rather than the "variable return assignment syntax", this is my proposal.

We consider the return value of both Mixins and Detached Rulesets as a "Less List" of rules, which of course will be mixed into the parent ruleset when called, by default.
We can access members of that list (and return their values) by key, which will either be a variable or property, using [].
```
.mixin() {
@color: blue;
bg: red;
}
.element {
color: .mixin['@color'];
background: .mixin['bg']; 
}
```
We can effectively get arbitrary "out values" like this:
```
.square(@val) {
@result: @val * @val;
}
.box {
height: .square(8)['@result'];   // result: 64
}
```
In addition, I was thinking we could treat Less lists similar to PHP arrays, which can be indexed by number or key (or both), and use [] as shorthand for the extract() function. So that you can write the above example as:
```
.square(@val) {
@result: @val * @val;
}
.box {
height: .square(8)[1];  // (Less lists are 1-based)
}
```
(Numerical access is not necessary, just an idea to normalize across lists, such that you could possibly also do extract(@list, 'key') or use @list[3] vs. extract(@list, 3). This could be a later-stage proposal.)

Since Less lists are not mutable, we don't need to add any other kind of array-like manipulation capability. And of course, this allows a great deal of flexibility with name-spacing and variables.

/* ui-library.less */
#lib() {
  .colors() {
    primary: blue;
  }
}

/* custom.less */
@include "ui-library";
// Override library vars
// Note: You can't name-space a mixin like this but it would be convenient
#lib.colors() {
  primary: red;
}

.button {
  background: #lib.colors['primary'];  // background: red;
}

Note, keys are quoted to make it clear that the key is not a local variable, which is still a possibility.

#lib() {
  .colors() {
    primary: blue;
    secondary: green;
  }
}

.section {
  @main-color-here: 'secondary';
  .button {
    background: #lib.colors[@main-color-here];
  }
}

The above pattern could be very powerful, as you could effectively have the same mixin apply & consume different namespaced values. (In other words, the .button class above could be rewritten as a re-usable mixin.)

So that's the main pieces of this proposal. And, just to re-state, we could continue forward with the breaking change of not leaking variables from mixins into the parent, and have variables private like detached rulesets do now. That would greatly simplify the two (and the documentation).

Thoughts welcome.

rjgotten commented 7 years ago

@matthew-dean Thoughts welcome.

One issue I see with this, is that outside consumers calling a mixin will need to have knowledge of the internal details of the mixin (i.e. the variable name they need to extract). That limits re-use, especially if said mixin comes from an @import 4 layers down in the guts of a framework or library.

matthew-dean commented 7 years ago

@rjgotten True, but how is that different than exposing variables into the parent scope? If you're going to use those values, then yes, you need to know the name of the variable / property returned by the mixin. The knowledge needed by the developer does not change. You could also argue that this makes the code more self-documenting. That is: right now, if you use a variable returned by a mixin, it's not clear at all from the point of the mixin call if that variable is from the mixin or just happens to be a variable available from further up the scope chain.

For example:

/* Less 0.x-3.x */
// @import... @import... etc.
.element {
  .mixin;
  value: @arbitraryvar; // from.... where exactly?
}

vs.

/* Less 4.x+ */
// @import... @import... etc.
.element {
  value: .mixin['@arbitraryvar'];  // ah, it's available in this mixin
}

matthew-dean commented 7 years ago

This also isn't that different than, say, needing to know the variable names of a library like Bootstrap 3 in order to override them. It's just (IMHO) more organized and concise.

rjgotten commented 7 years ago

Don't get me wrong: I agree that using index accessors this way is an improvement over the current situation,

I was comparing to the syntax that I proposed earlier, where the variable that serves as the mixin's output is set by the mixin; i.e. , it's encapsulated at that level and callers don't need that bit of knowledge.

matthew-dean commented 7 years ago

@rjgotten

I like the declarative nature of the syntax you proposed, but giving it more thought, I think it runs into two problems.

It's not conducive to exporting more than one value. True, you can assign a variable to contain a list of other values, like...
```
.mixin(@b, @c): @a {
@a: @b @c;
}
```
But then you just need to extract the value from that list again extract(@a, 1). Meaning, it can inadvertently make Less syntax more verbose than the current scenario.
The behavior of multiple matching mixins is not necessarily intuitive, as in:
```
.mixin(): @a {
@a: foo;
}
.mixin(): @b {
@b: bar;
}
.mixin() {
@c: nothing is returned;
}
.element {
value: .mixin();  // ?
}
```
Whereas, even if you have multiple matching mixins with [], you can rely on the cascade to give you a final value.
```
.mixin() {
@a: foo;
}
.mixin() {
@a: bar;
}
.mixin() {
@c: nothing;
}
.element {
value: .mixin['@a'];  // bar
}
```
So, the latter syntax, rather than allowing conflicting exports in the mixin definition, essentially works by evaluating all mixins into a single ruleset (list), and then obeying normal Less variable / property override rules (last value wins). So, I feel like it aligns a bit better with the cascade, and is able to essentially replace current behavior closer to 1-to-1 whereas the declarative export leaves a few gaps.

As in:

// 3.x-
.element {
  .mixin();
  value: @a;  // bar
}
// 4.x+
.element {
  value: .mixin['@a'];  // bar
}

In both cases, all the mixins get evaluated first in the same manner, except in the second case, unwanted variables / properties do not leak into the parent scope.

Know what I mean?

matthew-dean commented 7 years ago

@rjgotten

One thing we could do to maybe have the best of both worlds is to have the behavior of mixins be such that if only a single variable / property (single key) is exported from matching mixins, then the result is that value and not a list with a single member. This behavior is not unprecedented in languages. We can even make Less smart enough to not error out if the developer accesses the value by key even when there's only one value.

In other words (oops, typo'd this the first time):

.mixin(@a) when (@a > 2) {
  @b: true;
}
.mixin(@a) when (@a <= 2) {
  @b: false;
}
.element {
  value: .mixin(1);  // false
  value: .mixin(1)['@b'];  // false
  value: .mixin(1)[1];  // false
}

So, we could have some (very lightweight) type coercion. However, my gut tells me this may cause some unwanted side-effects that may not be immediately apparent. But....Less is a very concise language, so like many parts of the language, the convenience value may be greater than the side effects.

What do you think?

calvinjuarez commented 7 years ago

Options, re: issues w/ the proposed syntax.

It's not conducive to exporting more than one value.

.mixin():@a {
  @a: foo;
  @b: bar;
}

.element {
  value: .mixin();  // foo
  value: .mixin()['@b'];  // bar
}

// OR

.mixin():@out {
  @a: foo;
  @b: bar;
  @out: @a, @b;
}

.element {
  value: .mixin()[0];  // foo
  value: .mixin()[1];  // bar
}

The behavior of multiple matching mixins is not necessarily intuitive.

.mixin():@a {
  @a: foo;
}
.mixin():@b {
  @b: bar;
}

.element {
  value: .mixin()['@a'];  // foo
  value: .mixin()['@b'];  // bar
}

// OR

.mixin():@a {
  @a: foo;
}
.mixin():@b { // error, a return variable has already been specified for `.mixin()` (`@a`).
  @b: bar;
}

My 2¢: I like declaring what's to be returned. It's self-documenting and answers probably 90% of my current use cases, personally. Unlocking with the old style makes sense for multiple values, and returning a list makes sense to me.

I love this feature, and will employ it heavily in whatever form it takes.

matthew-dean commented 7 years ago

@rjgotten - Or, we force mixins with named exports to be singletons. Maybe that's a better "best of both worlds".

@calvinjuarez - As I was typing that, you were providing an example, so I would agree with that last one, but the error should probably be something like: Error: Multiple matching mixins cannot use named exports. Probably another rule should be that mixins with named exports cannot have when guards.

We still have an available solution for retrieving values by key: https://github.com/less/less-meta/issues/12#issuecomment-230340920

In fact, there's no reason that a named export mixin can't also have a variable extracted.

.math-stuff(@a, @b): @y {
  @x: @a * @b;
  @y: @x * @x;
}
element {
  value: .math-stuff(2, 3);
  first-value: .math-stuff(2 ,3)['@x'];
}

So, basically, these features could be developed separately.

rjgotten commented 7 years ago

In fact, there's no reason that a named export mixin can't also have a variable extracted.

I'd vote +1 for that.

Extracting known names as property keys offers great flexibility, iterative development and velocity within your own code; while
a named default export offers a reliable public API for framework authors that don't want to burden their users with requisite knowledge of variables that are available internally in a mixin.

Both serve similar goals, but take different viewpoints that are equally valuable. (And both equally well remove the need for the nasty behavior of variables flowing back up the closure tree, as they do now.)

seven-phases-max commented 7 years ago

I'm OK with .mixin(...)[ident] and I only object automatic deducing of a single ruleset property/variable into a return value. I.e. this.:

.mixin() {
    @b: false;
}
.element {
   value: .mixin();

should throw a error. Otherwise the guesswork is going to be a source of continuous unintended errors. If you want return value - go explicit return: false (or @return: false).

(And finally we came to https://github.com/less/less.js/issues/538 the other way around :P And even no @function garbage turns out to be necessary).

Btw., technically (if we do declare"mixins returning rulesets"):

.mixin() {
    @b: false;
}
.element {
    @value: .mixin();
}

shall be equal to:

@mixin: {
    @b: false;
}
.element {
    @value: @mixin;
}

(i.e. this way the mixin return value is basically nothing but another form of DR).

matthew-dean commented 7 years ago

I only object automatic deducing of a single ruleset property/variable into a return value

Yes, I was already leaning away from that when I said, "However, my gut tells me this may cause some unwanted side-effects that may not be immediately apparent." Explicit is better, and I think that just demonstrated the need for both approaches. So to be clear, I agree with you.

@rjgotten - Your two bullet points are a perfect summary. Basically, I wanted to make sure that the proposal for accessing properties / variables wouldn't be in conflict with a return variable.

Now all we need is a developer to make it happen! This would probably be for a 4.0 release, if we're aligning mixin / DRs to have private vars. Probably this would be good to start a 4.x branch after 3.x is finished and released.

(As far as timing: on 3.x, I just am wanting to refactor the plugin manager to fit more seamlessly into the file manager. Currently, there's a lot of file lookup logic that's decentralized from file operations. Just need to find time to do that.)

matthew-dean commented 7 years ago

As far as writing up a new spec for this, some questions to answer. We've already established that a return variable should only be allowed for a single matching mixin.

Should this also mean that mixins with named exports cannot have when guards?

matthew-dean commented 7 years ago

Btw., technically (if we do declare"mixins returning rulesets")...

Yep, I had that same thought! An evaluated mixin (assigned to a property or variable) without an export could essentially create a detached ruleset. Which could actually be very powerful. So to extend that example:

.mixin() {
  prop-1: foo;
}
.mixin() {
  prop-2: bar;
}
.element {
  @value: .mixin();
  @value();
}

Evaluating to:

.element {
  prop-1: foo;
  prop-2: bar;
}

rjgotten commented 7 years ago

@matthew-dean Should this also mean that mixins with named exports cannot have when guards?

If there are more than a single matching mixin signature, then the value returned from the mixin call should logically just be a list of all the return values for the matching signatures. This matches what the standard ruleset output of today does, after all.

However, thinking more about it: a substantial problem with allowing when guards and multiple signatures is that it is unpredictable what value is going to end up at what index in the list and which values are present and which not.

So maybe we should allow when guards, but also dictate that they be mutually exclusive? I.e. force that you'll never get more than one return value. (And return some type of 'null' value when none of the signatures' guard conditions match.)

seven-phases-max commented 7 years ago

I don't see any problem with multiple mixins matching the same call (guards do not change anything as well). It's just old boring cascade&LDW - i.e. the value returned is always strictly determined (see for example less-plugin-functions).

After all if multiple-matching mixins (and then functions) with multiple-matching guards confuse someone - he just won't use them (like they don't confuse me and I'm just fine using them).

matthew-dean commented 7 years ago

@seven-phases-max So, for you, you would just see it resolved like (?):

.mixin(@b): @a when (@b > 1) {
  @a: 1;
}
.mixin(@b): @c when (@b >= 0) {
  @a: 1;
  @c: 2;
}
.mixin(@b) when (@b < 1) {
  prop-1: foo;
}
.mixin(@b) when (@b < 1) {
  prop-2: bar;
}
.mixin(@b): $prop-1 when (@b < 0) {
  prop-1: foo;
}
.one {
  @val: .mixin(1);   // value is 2
}
.two {
  @val: .mixin(2);  // value is 2
}
.zero {
  @val: .mixin(0);   // value is { prop-1: foo; prop-2: bar; }
}
.neg {
  @val: .mixin(-1);  // value is foo
}

I think, originally, I thought multiple matching mixins would be in conflict, but I was assuming that differing named exports would be irreconcilable. But if we take it as the latter mixin "wins" in the cascade, including the type of export (single value or DR), then it's fairly easy to resolve.

I think you're right that in a normal scenario, a Less author is going to design the complexity that they want, and the above example is probably far beyond the complexity anyone would actually do.

So, yeah, maybe it's not really a problem after all.

matthew-dean commented 7 years ago

I made a mistake in the above example the first time. Corrected.

seven-phases-max commented 7 years ago

So, for you, you would just see it resolved like (?):

I guess I forget to say I do not support this syntax:

.mixin(@b): @a ...

sorry (I do not see how it's more useful than a return property introducing no new syntax concepts). I doubt there's any need in a mixin that can be used simultaneously as returning a ruleset and a single value, at least I do not see this would be worth new syntax concept. So in "my world" the example is just:

.mixin(@b) when (@b > 1) {
  return: 1;
}
.mixin(@b) when (@b >= 0) {
  @a: 1;
  @c: 2;
  return: @c;
}
// etc.

There's still conflict if a mixin is assigned to a variable and can return both DR and a single value specified with return but I guess choosing whatever result we like will be just fine. Just because a code like:

foo {
  x: mixin(2); 
}
bar {
  x: mixin(2)[@a];
}
// hmm, why did he name essentially different functions the same way?
// if one needs a function that returns a particular value extracted
// from a set returned by *another* function he just writes a wrapper.

would be a bad design anyway (that famous "I can do everything" functions).

So with all of this in mind, a final version of a normal code (the one to be considered a typical usage when deciding on solving such syntax conflicts) becomes:

.mixin(@b) when (@b > 1)  {return: 1}
.mixin(@b) when (@b >= 0) {return: .whatever(@b)[@c]}
.whatever(@b) {
    @a: 1;
    @c: 2;
}
// etc.

rjgotten commented 7 years ago

sorry (I do not see how it's more useful than a return property introducing no new syntax concepts)

The major advantage is that it opens up several optimization scenarios in the compiler. It allows a priori detection of whether the mixin has a named export value, simply by looking at the mixin's declaration, without need to parse and evaluate the mixin's body for the presence of a return: property assignment.

The latter could also happen lazily, which means you need to work off of the evaluated mixin before you can choose whether to discard the result or not, because it would or would not be valid in the context of the mixin's call.

(I.e. a mixin with a named export could not be called as a 'normal' mixin producing a ruleset.)

matthew-dean commented 7 years ago

Everything @rjgotten said. Mixin "signatures" can be determined early in parsing, and not in evaluation. And a specially-designated property is arbitrary and not devoid of conflicts. Syntactically, it also just doesn't logically follow, since there's no other instance of "behaviors" attached to property names. However, the concept of assignment is pervasive, so an assignment of the export var (or prop name perhaps) is much more logical. The idea of a "return" statement for mixins has always fallen flat in other threads. It's just not a good fit for the language.

As far as other benefits, IIRC, a lot of the time spent in Less evaluation happens in mixin evaluation, and part of the workout of the JavaScript engine is spent splicing arrays and performing lookups. The ability to designate all vars in and all vars out of a mixin call could greatly optimize mixin evaluation, since, theoretically, the same call with the same parameters could be cached as a single result. Right now, there's a lot of layers of evaluation because of variable exposure in / out of mixin definitions and in / out of mixin calls, and the placement of those within the Less tree. Which, as noted elsewhere, is not only a performance concern, but also has add-on effects that are unexpected and require documentation of those edge cases. So the current behavior is more complex than solving the same problems using a different strategy.

matthew-dean commented 7 years ago

@seven-phases-max Btw, a lot of what I said is not directly in support of the declarative syntax, but more the concept and the benefits, and also the negatives of an arbitrary return.

seven-phases-max commented 7 years ago

@rjgotten

It allows a priori detection of whether the mixin has a named export value, simply by looking at the mixin's declaration,

Not really different from knowing a called mixin has its return value simply by the call statement itself.

@matthew-dean

The ability to designate all vars in and all vars out of a mixin call could greatly optimize mixin evaluation, since, theoretically, the same call with the same parameters could be cached as a single result.

So... regardless of the syntax the main goal is to prohibit stuff like:

.func(@x): @r {
    @r: @p + @x;
}

#foo {
    @p: 1;
    width: .func(2);
}

#bar {
    @p: 3;
    width: .func(2);
}

// ^ can't cache anything

right?

Then I guess I'd be thinking of going back to some @function ... syntax stuff. Otherwise new consistency/aesthetics/confusion problems pile up:

If this stuff is not going to behave like regular mixins then it should not look like regular mixins (i.e. ?function foo(...) declaration beats .foo(...): @result back again - right at the first symbol parsed).
Since it's function name that usually describes its return value (like if you return some "foo-width" its the function usually named foo-width), the syntax forces 99% of the functions to look like:
```
.foo-width(@v): @result {@result: @v * 2}
.bo-height(@v): @result {@result: @v * 3}
// nothing critical but weeeeird....
```
If I write a function that returns a set (i.e. always to be used like .foo(...)[something]), I will still have to define it like .foo(...): @unused {..., will I?

My initial impression was that the goal of this is to make mixins and functions as much interchangeable as possible. But if the primary goal is performance then this all apparently shall run away from mixins as far as possible (incl. the definition syntax).

So... if we start from scratch, how did this .foo(...): @result syntax come out at all? Why the opening dot? Why the return variable shall be explicitly named in the declaration?

Btw. consider how the new syntax can handle possible improvements. For example what about:

#foo {
    width: .bar(1).baz(2)[qux];
}

How the definition of the .bar could possibly look like? (It seems like tending to end with .bar(...): @unused {... yet again).

matthew-dean commented 7 years ago

If this stuff is not going to behave like regular mixins then it should not look like regular mixins (i.e. ?function foo(...) declaration beats .foo(...): @result back again).

This thread actually contains a messy pile of different referenced proposals, and I think that's leading to some confusion.

Proposal 1: Aligning DRs and Mixins (this one)

Basically: either exposing variables from DRs, or making variables private in mixins. There seemed to be some consensus leaning towards considering the latter i.e. making mixin variables private. But then that leads to the inevitable: "Well then how do you export vars from mixins to be available within scope?" From there, there are a few different proposals.

Proposal 2: Namespace referencing - #12

Proposal 2 does not depend on proposal 1, it's just one possible solution for it. That is, being able to reference a variable or property's value from an evaluated ruleset. e.g.

value: .mixin['@var'];

The reason why this works is because "namespaces" are basically parameter-less mixins. So if proposal 2 moves forward, then adding parameterized mixins is not that different. Like mixins, a "namespace" can have multiple matches and would need to be evaluated before a "return value" could be extracted.

Proposal 3: Named variable exports

@rjgotten proposed that here: https://github.com/less/less.js/issues/538#issuecomment-249646504

Proposal 3 is independent of Proposals 1 & 2. It would be another way to solve the issues raised in proposal 1, by explicitly exporting values to be consumed in the parent scope.

Proposal 4: Complete variable isolation in/out of mixin calls

This is basically an extension of Proposal 1, and I shouldn't have brought it up, because that's confusing. Meaning essentially: a proposal that variable scope extends only to mixin definition, and not mixin calls. Proposal 1 is that variables in mixins are not exposed to parents in calls. Proposal 4 is that parent variables are not exposed to mixins during mixin calls, since they can be passed explicitly into the mixin call. However, parent variables would still be exposed to mixin definitions. It would just simplify some of the evaluation behavior.

Note, of course: Proposal 4 is not dependent on Proposals 1, 2, or 3.

So, I think your confusion is based on some different ideas that are floating here, and some of that is my fault.

In other words:

No one is proposing that these specific mixin "signatures" should behave differently from other mixins, because you are correct, that would make no sense.

What's being proposed is that all mixins behave differently, and working through some different ideas about how to do that. Not all of these ideas are breaking changes (such as Proposals 1 & 4), and the basic ideas being discussed are:

Making mixin variables private, which is separate from
Allowing mixins to return single values, which is an alternative to (but not in conflict with):
Allowing single values to be "extracted" from a mixin call

Now, an argument could be made that #2 conflicts with the idea that all mixins behave the same, and maybe #3 doesn't, but I'm curious what your thoughts are now that I've hopefully clarified these into distinct proposals.

matthew-dean commented 7 years ago

And just to add to that clarification: nearly all of these ideas stem from the idea that, currently, variable scope in Less is a little (IMHO quite) messy, especially in regards to variable scope in mixin calls and definitions, and in regards to behaving differently between different node types, and in different contexts.

However, someone may disagree with this entirely, and they feel like variable scope in Less is just fine and nothing should be done about it. And that's a valid opinion to share too.

matthew-dean commented 7 years ago

For the purpose of this thread, we should first just address aligning DR and mixin behavior.

Which of the following should happen:

Mixin vars should be private on mixin calls, like DRs.
DR variables should be public on DR calls, like mixins.
Nothing.

My vote is for #1.

If vars are made private, then they should be exposable by one of the following methods:

Named exports
Namespace referencing
Both

My vote would be #2.

@seven-phases-max @rjgotten Want to pick this back up again?

seven-phases-max commented 7 years ago

If we choose to not expose variables from mixins/DRs, the same should happen to exposed mixins as well. I.e. variables and mixins are entities of the same level and should not behave differently (the same goes for functions when they'll appear).

rjgotten commented 7 years ago

@seven-phases-max Unlocking mixins has one very useful feature you'd be hard-pressed to get in any other way though; partial function application, i.e. , passing in a number of shared arguments into the initial mixin that unlocks the other mixins, which in turn all use (part of) those shared arguments in addition to their own.

seven-phases-max commented 7 years ago

@rjgotten

Then I'm afraid the whole story becomes inconsistent - we hide variables because we don't like them (it does not work like this in JS) but we keep mixins just because we like them there (thought this behavior has nothing to do with canonical scope). Now the questions is: what is the fundamental difference between these language structures (what exactly makes you to believe they should behave differently)? Why one identifier should get visible this way and another should not? (I'm not even recall of properties that will be a headache in this context as soon as we'll be able to "read" them (and this is finally something we can't ban - so our "return variables" come back, just under another name :P)).

matthew-dean commented 7 years ago

If we choose to not expose variables from mixins/DRs, the same should happen to exposed mixins as well. I.e. variables and mixins are entities of the same level and should not behave differently (the same goes for functions when they'll appear).

I'm not sure I agree that this should be an automatic assumption, like, "Well we have to do this, otherwise we can't do anything." We can be pragmatic about it and do what makes sense. But I get where you're coming from.

In my mind, name-spacing should be the correct way to access inner mixins. "Unlocking" leads to a lot of evaluation complexity where rule-arrays get spliced often to include all rules of a mixin beyond the property/value pairs that may be all someone needs.

In other words, Less.js already has these concepts:

Given .namespace { .mixin() { } }, I can call .namespace.mixin();.
I can also call .namespace(); That is: simple ID and class selectors are basically non-parameterized mixins.

So we can already "call" mixins within other mixins without "unlocking". The only thing missing is calling mixins inside parameterized mixins. .namespace(@foo: bar).mixin().

If we move towards namespacing, we don't have to just merge everything and increase the size of lookups for child rulesets within those rules. It could be as simple as:

.namespace(@foo: bar)[prop]
.namespace(@foo: bar)[@var]
.namespace(@foo: bar).mixin()

We could easily cache .namespace(@foo: bar) if we did proposal #4 above, but even without it, those calls would probably be minimal.

So the only two feature expansions needed to remove both variable and mixin leaking (called the much nicer "unlocking" in docs) are to expand namespacing to allow parameters, and expand namespacing to access vars.

rjgotten commented 7 years ago

So we can already "call" mixins within other mixins without "unlocking". The only thing missing is calling mixins inside parameterized mixins. .namespace(@foo: bar).mixin().

Beat me to the punch. :-) This is what I was going to suggest as well.

And if we can store a mixin's result and can continue pulling props and vars from it; why not pull mixins as well?

@factory : .namespace(@foo);
// (...)
@factory.mixin(@bar);
@factory.other-mixin(@baz);

There's only one ambiguity here; and that's if the .namespace(@foo) namespace-mixin would specify a default return value. In that case the result of the evaluation would be that return value and not the entire collection of everything (incl. mixins) in the evaluated namespace.

But still; that's up to Less's users to take notice of and write their styles properly to avoid that issue. I.e. simply don't have mixins have a default return value, when you want to use them as a namespace. It makes no bloody sense to have namespaces have a single return value anyway.

matthew-dean commented 7 years ago

And if we can store a mixin's result and can continue pulling props and vars from it; why not pull mixins as well?

Yep, that is an elegant extension of this idea, and a good way to "cache" a ruleset and pull nodes from it WITHOUT needing to expose anything to a larger scope. It would also be a good way to simplify mixin calls, like:

#mylibrary {
  .core {
    .colors {
      .fancify(@color) {
        // make this color fancy
      } 
      .dullify(@color) {
        // make this color boring
      }
    }
  }
}
.foo {
  @colors: #mylibrary.core.colors;
  color: @colors.fancify(red);
  background-color: @colors.dullify(brown);
}

You can both organize variables and mixins and greatly simplify that organization at the same time in actual usage.

There's only one ambiguity here...But still; that's up to Less's users to take notice of and write their styles properly to avoid that issue.

Agreed.

rjgotten commented 7 years ago

So, in short: If the above feature with parametrized namespace mixins becomes available as an alternative, I think it'd be fine to also stop mixins from bubbling back up out of mixins (i.e. 'unlocking') and achieve parity with properties and variables not bubbling back up.

seven-phases-max commented 7 years ago

Don't forget about just:

@pointer: .somemixin;
@pointer(1, 2, 3); // = .somemixin(1, 2, 3), same for parametric DRs etc.

So far the syntax and semantics look OK for me (after all I was dreaming of all that yet at https://github.com/less/less.js/issues/965#issuecomment-25328937 and https://github.com/less/less.js/issues/1812).

matthew-dean commented 7 years ago

As to:

@pointer: .somemixin;
@pointer(1, 2, 3); // = .somemixin(1, 2, 3), same for parametric DRs etc.

Unfortunately, that wouldn't be possible for this syntax, since @pointer: .somemixin; would assign the variable to the called mixin's result, and not the mixin definition. It would be equivalent to @pointer: .somemixin();. Unless we performed some kind of magic that I'm not thinking of.

The reason why @colors: #mylibrary.core.colors; works in the above example is because it does just that, it returns the ruleset, and is not a reference to the selector per se, as there may be more than one matching mixin.

Basically, @colors: #mylibrary.core.colors; would internally be a call to that mixin, which then gets evaluated, and then returns a detached ruleset to assign to the variable. So, in a sense, all the "referencing" is actually lookups on a detached ruleset, not on individual mixins/namespaces. This has to happen because of the behavior of multiple matching mixins. We first need to evaluate the first part of the call, and then we can access members (rules/nodes) from there.

Note this is what happens in @rjgotten's example:

@factory : .namespace(@foo);
// (...)
@factory.mixin(@bar);
@factory.other-mixin(@baz);

The mixin is evaluated and a DR is returned to @factory. The DR contains mixin definitions, which then get called / evaluated. If the returned DR contained multiple mixin definitions called .mixin and .other-mixin, (because, say, there are multiple matching .namespace blocks) then those would be called using regular mixin-matching behavior.

Basically, the above is roughly equivalent to:

.namespace(@foo) {
  .mixin(@bar) {
  }
  .other-mixin(@baz) {
  }
}
@factory: {
  .namespace(@foo);
};
// (new referencing syntax)
@factory.mixin(@bar);
@factory.other-mixin(@baz);

rjgotten commented 7 years ago

@matthew-dean wrote: [ ... insert kick-ass in-depth explanation ... ]

^ Yup, what he said. ^_^

You could avoid that, but you'd need to change the language rules such that parameter-less mixins no longer automagically evaluate if they are 'called' without braces. That's a whole other thing to debate over.

seven-phases-max commented 7 years ago

It would be equivalent to @pointer: .somemixin();

Well. First of all, this

foo {
    @var: .identifier; // I'm valid and I'm NOT mixin call!
    @{var} {bar: baz}
}

has always been a valid Less code (even if it's a quirk). So it's just something you need to either ban first. Or just another good reason to ban the equivalency of .foo and .foo() elsewhere instead (this sugar has done its trick in the past - these days, after all that extend, namespaces and the rest of fancy things came in, the optional parens of the call only hurt and harm). So no, it's not an excuse to not consider this feature (you already proposed much more dramatic changes for the rest of stuff).

I.e. consider another example:

...:extend(.something);

Obviously extend does not support variables as its arg, but ideally it will, and then, according to you, the code changes its current semantics because .something is a mixin call... :)

matthew-dean commented 7 years ago

@seven-phases-max

I'm not sure if I understand what you're trying to say. It's not a matter of not considering a feature; it's that this code is incompatible with this feature:

@pointer: .somemixin;
@pointer(1, 2, 3);

But, as you said, the above would be a breaking change anyway, so:

Or just another good reason to ban the equivalency of .foo and .foo() elsewhere instead

I'm not against this. That certainly would make things a little more consistent and code self-documenting.

Are you trying to suggest that we do this?

@pointer: .somemixin;    // returns a pointer to .somemixin
@pointer(1, 2, 3);

@result: .othermixin();   // returns a ruleset from a .othermixin call
@pointer2: .othermixin;  // returns a pointer to .othermixin

The only problem I foresee (other than breaking changes) is that it gets kinda confusing (potentially) with the DR call. In other words:

.mixin() {
  foo: bar;
}
.block {
  @pointer: .mixin();   // calls mixin here, returns DR
  @pointer();
}
.block-2 {
  @pointer: .mixin;  // returns something like a mixin lookup pointer
  @pointer();   // calls mixin here
}

So, I don't know if this would cause confusion or not, but @pointer() is not the same call. Maybe that's perfectly fine? I took a look at my code with @colors: #mylibrary.core.colors;, and whether it returns a mixin pointer or a DR, the results (in this case) would be exactly the same.

If we make that distinction and allow mixin / namespace pointers, then I think we'd want to do away with:

.block {
  .mixin;   // this shorthand, which always looked kinda weird anyway
}
.block {
  .mixin();   // better, aligns with DR calls
}

Which I think is what you're saying. Did I understand you correctly @seven-phases-max? And @rjgotten, what do you think?

seven-phases-max commented 7 years ago

Another remark is about "internal" semantic of things like:

@var: anything or @anything;

This does not really change the behaviour of anything proposed above, but it's important to stress to eliminate some confusions.

When we write something like:

@some: whatever; // really anything including DRs
@var: @some;

The @var value is not whatever, it's @some. (The actual value is deduced only where @var is used). Same way when we write something like:

@var: .anything;

The @var value is nothing but the identifier (i.e. simply speaking just a string), it's never anything behind .anything (it's OK for no .anything {...} to not even exists at all!) So when we write something like:

@colors: #mylibrary.core.colors;
color: @colors.fancify(red);

It does not (and should not) assign either definition or value or reference or whatever of the said ruleset to @colors, everything is (and must be) evaluated in the second statement. And that btw. means that a code like:

#lib {
    .ruleset {
        .foo() {/* return something */}
        bar: baz;
    }
}

div {
    @ref: #lib.ruleset;
    color: @ref.foo(); // ok = #lib.ruleset.foo()
    @ref();            // ok as well = #lib.ruleset()
}

would be fine too even w/o banning "optional ()" (not meaning such poor mixed-up code is ever suggested to be used of course)

_{And just in case, before you start the usual "but caching ... bla-bla.bla.." - it's not the value of the @ref (yet again its value is just a raw string w/o any meaning yet) to be cached, but the result of #lib.ruleset lookup (which is a bit different thing)}

matthew-dean commented 7 years ago

It does not (and should not) assign either definition or value or reference or whatever of the said ruleset to @colors, everything is (and must be) evaluated in the second statement.

Exceeept, what about:

.mixin(@val) {
  prop: @val;
}
.block {
  @var1: .mixin(value-1);
  @var2: .mixin(value-2);
  prop-2: @var1[prop];
}

Surely, in this instance, the mixin is evaluated at @var1 and @var2? OR are you suggesting it's evaluated like prop-2: .mixin(value-1)[prop]?

What happens with this case:

.mixin(@argument1, @argument2) {
  is: black;
}
.mixin(@argument1) {
  is: white;
}
.block {
  @pointer: .mixin(one);
  @pointer(one, two);
}

Would we throw an error? Or would the arguments just get replaced? That's my concern with delaying evaluation of the call until later. Or one concern. That is, that errors may happen in unexpected places. But that's not a deal-breaker by any means. I just wonder how you would suggest handling these cases.

seven-phases-max commented 7 years ago

Exceeept, what about:

You could answer it youself. The keyword is "Lazy-Evaluation" - so unless we're designing another scss/stylus clone here (what for?) - it is evaluated in the end. E.g. consider this code:

@var: .mixin(33);
.block {
    prop-2: @var[prop];    
   .mixin(@x) {prop: @x + 44}
}

I can't see any reason for that code to not be valid. So the answer is no, we can't cache .mixin(33) globally there (but only per scope where it's used). This is just how variables (and not only variables) work in Less.

less / less-meta