Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify Mixin and DR behaviour (and related Functions stuff) #16

Open
seven-phases-max opened this issue Apr 16, 2016 · 79 comments
Open

Unify Mixin and DR behaviour (and related Functions stuff) #16

seven-phases-max opened this issue Apr 16, 2016 · 79 comments

Comments

@seven-phases-max
Copy link
Member

seven-phases-max commented Apr 16, 2016

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
Copy link
Member

👍

@matthew-dean
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

"Anonymous Mixin"

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

@matthew-dean
Copy link
Member

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
Copy link
Member

matthew-dean commented Jul 14, 2016

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
Copy link
Member

matthew-dean commented Sep 27, 2016

Per @rjgotten's comment here: less/less.js#538 (comment)

...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
Copy link
Member

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
Copy link

rjgotten commented Nov 8, 2016

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
Copy link
Member

matthew-dean commented Jan 2, 2017

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. #12 (comment))

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
Copy link

rjgotten commented Jan 2, 2017

@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
Copy link
Member

matthew-dean commented Jan 2, 2017

@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
Copy link
Member

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
Copy link

rjgotten commented Jan 3, 2017

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
Copy link
Member

@rjgotten

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

  1. 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.

  1. 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
Copy link
Member

matthew-dean commented Jan 3, 2017

@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
Copy link
Member

Options, re: issues w/ the proposed syntax.

  1. 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
}
  1. 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
Copy link
Member

@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: #12 (comment)

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
Copy link

rjgotten commented Jan 23, 2017

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
Copy link
Member Author

seven-phases-max commented Jan 24, 2017

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 less/less.js#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
Copy link
Member

matthew-dean commented Jan 24, 2017

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
Copy link
Member

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
Copy link
Member

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
Copy link

rjgotten commented Jan 24, 2017

@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
Copy link
Member Author

seven-phases-max commented Jan 24, 2017

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).

@seven-phases-max seven-phases-max changed the title DR expansion behaviour to match the Mixin one. Matching Mixin and DR behaviour (and related Functions stuff) Apr 20, 2017
@seven-phases-max
Copy link
Member Author

seven-phases-max commented Apr 22, 2017

I guess I forgot to give a practical answer with examples to this:

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.

If I understand it right you mean something like this:

.my-theme-ns(@base-color, ...) {
    @primary:   @base-color;
    @secondary: white - @base-color;
    .some-mixin() {color: @base-color}
    .another-mixin() {background: white - @base-color}
}

#usage {
    .my-theme-ns(red);
    .some-mixin();
    border: 1px solid @secondary;
}

Right? Well, I guess the way I wrote it already should ring some bells. Yep, zero difference between vars and mixins here. So you can't really justify different visibility by that pattern.
And then I can only repeat: all these (variables, mixins, DRs) are the same language entities of the same level, they may vary in details (just like in any other language, e.g. in JS it's var x vs function x(), in Less it's @x vs .x() + cascade vs. override) but their scope visibility should be identical no matter what. (So I'm afraid until Less has all features to ban "callee -> caller exposing" for both we should allow both).


Btw., speaking of above example, it's curious to recall that for mixins we already have "parametric mixin to 'static ns' with subscription" syntactic sugar:

// .my-theme-ns def. from prev. example

#usage {
    .my {.my-theme(red, other stuff)}; // create new object and assign it to .my variable

    .my.some-mixin();
    .my.another-mixin();
}

And yes, it will be sad to loose (since then I will most likely have to use some ugly @ns.mixin() instead of the beautiful .ns.mixin() :) but well, art requires sacrifice :(

@matthew-dean
Copy link
Member

matthew-dean commented May 5, 2017

Right? Well, I guess the way I wrote it already should ring some bells. Yep, zero difference between vars and mixins here. So you can't really justify different visibility by that pattern.

I agree completely, especially once we allow variables to "alias" mixins. At that point they need to have identical scoping rules, so both variables and mixins should be isolated from the parent scope, but you can easily just change:

//Less 1.x - 3.x
#usage {
    .my-theme-ns(red);
    .some-mixin();
    border: 1px solid @secondary;
}

to:

//Less 4.x
#usage {
    .my-theme-ns(red).some-mixin();
    border: 1px solid .my-theme-ns(red)[@secondary];
}

Or for better abstraction (not repeating yourself):

//Less 4.x
#usage {
    @theme: .my-theme-ns(red);
    @theme.some-mixin();
    border: 1px solid @theme[@secondary];
}

Which, it so happens, is WAY better self-documentation, as you know exactly what variables / mixins are coming from where or a result of which mixin call.

So, yes, I agree with @seven-phases-max on this one.

@matthew-dean
Copy link
Member

matthew-dean commented May 5, 2017

What I like about these changes is that there's such an easier-to-understand flow of inputs and outputs. Mixin calls get direct inputs from variables that are required within that mixin call*, and outputs are directly flowed to where they need to be evaluated, with no leaked side-effects.

*Note: I'm speaking only of mixin calls, since mixin DEFINITIONS would still inherit vars just like any other selector

@rjgotten
Copy link

rjgotten commented Jun 6, 2017

Or for better abstraction (not repeating yourself):

Holy crap. That is awesome. I never considered aliasing the to-be-lazily-resolved mixin namespace into a variable like that for further extraction and execution of namespaced logic. But with Less's everything-is-lazy pull semantics that would actually work brilliantly.

And daa---mn is that ever pur'dy compared to the warts I have to produce nowadays.

@matthew-dean
Copy link
Member

@rjgotten Yeah this is in a pretty good place now. I think there is enough consensus in this thread to make it a 4.0 recommended feature?

@seven-phases-max
Copy link
Member Author

seven-phases-max commented Jun 7, 2017

I think there is enough consensus

Yes.

// Less 4.x
#usage {
    .my-theme-ns(red).some-mixin();
    border: 1px solid .my-theme-ns(red)[@secondary];
}

Though I'm not sure how easy it would be to modify the parser to support an arbitrary count of subsequent subscript/call operators... i.e.: foo(...)(...).bar(...)[...](...)[...][...].baz[...](...) // <- valid code. So it's could be spread over Less 3.x-5.x.

@matthew-dean
Copy link
Member

matthew-dean commented Oct 9, 2017

Can someone write up a summary of the recommended feature that we reached consensus on and post a proper feature request to less/less.js?

@matthew-dean
Copy link
Member

Can someone write up a summary of the recommended feature that we reached consensus on and post a proper feature request to less/less.js?

@less/core Hold off on writing up a proposal until #12 is finalized.

@seven-phases-max seven-phases-max changed the title Matching Mixin and DR behaviour (and related Functions stuff) Unify Mixin and DR behaviour (and related Functions stuff) May 31, 2018
@matthew-dean
Copy link
Member

So have a look at this comment on this PR: less/less.js#3242 (comment)

After writing up this example, I was wondering if maybe @seven-phases-max's original proposal wasn't the correct one. That is, aligning behavior by exposing vars from VariableCalls (like MixinCalls) vs the opposite (making MixinCall vars private).

Further to that, I'm wondering if we can't "fix" variables dumped into callee scope by changing precedence, such that the inherited declared vars take precedence over mixincall merged ones.

For example, currently this is the case:

Input (current):

.set-color() {
  @color: blue;
}

.one {
  .set-color();
  val: @color;
}

.two {
  @color: red;
  .set-color();
  val: @color;
}

.three {
  @color: red;
  & {
    .set-color();
    val: @color;
  }
}

Output (current):

.one {
  val: blue;
}
.two {
  val: red;
}
.three {
  val: blue;
}

So far in this thread, we've been leaning this way if we change mixins to have private vars:

.one {
  val: // ERROR
}
.two {
  val: red;
}
.three {
  val: red;
}

I propose that we make the result this:
Output (proposed):

.one {
  val: blue;
}
.two {
  val: red;
}
.three {
  val: red;
}

In other words, the variables declared in selectors take precedence. So, mixin call vars are still merged into callee rules, but marked as merged, and during variable lookup, the cascade gives un-merged vars a higher weight.

I feel like this is less of a breaking change, for both mixin calls, and then if we align to variable calls. Consider the following:

Input (current):

@set-color: {
  @color: blue;
};

.one {
  @set-color();
  val: @color;
}

.two {
  @color: red;
  @set-color();
  val: @color;
}

.three {
  @color: red;
  & {
    @set-color();
    val: @color;
  }
}

Output (current):

.one {
  val: // NameError: variable @color is undefined
}
.two {
  val: red;
}
.three {
  val: red;
}

With my proposed change, we would still keep the output the same, except no error in this case:
Output (proposed):

.one {
  val: blue;
}
.two {
  val: red;
}
.three {
  val: red;
}

I feel like we'd get waaaay fewer complaints about this than causing MixinCalls to error where it currently does not. Instead we cause VariableCalls to NOT error, but also NOT override inherited vars, which would effectively preserve behavior and not be a breaking change for it.

We also would no longer have to "switch" behavior of MixinCalls based on whether or not it finds a variable in the immediate callee scope. Instead, it just faithfully merges all its vars, but it's during evaluation where we find out if it's needed or not. It just has lower precedence.

What do you think? I feel like this is the "safer" breaking change and still aligns the two types.

@rjgotten
Copy link

rjgotten commented Jul 1, 2018

It's safer from a code-migration point of view, but one of the driving points of making mixin variables private is to mirror behavior of nearly every programming language out there; none of which leak local variables outward without some explicit return mechanism. It's basic principle of least astonishment, in that sense.

Using leaking variables as a return mechanism also means mixin consumers need to know about the internals of a mixin, i.e. they need to know what variable names to access.

I also feel like there is quite a bit of complexity and ambiguity in the Less parser due to this mechanism. It requires some rather weird kludges with detecting not-yet-existing variables and postpone bits of logic that need them to run; trying to execute all possible mixins within the current ruleset scope to get them to appear; then resuming with the remainder of the logic in the ruleset scope. Add the possibility of this pattern occuring in nested sequences; or how it interplays with order-dependency of some other parts of the language; ... ouch.

If you make mixin variables private and use an explicit return mechanic, it becomes much easier to statically analyze the code into a dependency call-tree and evaluate everything in order in a robust, predictable way.

@matthew-dean
Copy link
Member

If you make mixin variables private and use an explicit return mechanic, it becomes much easier to statically analyze the code into a dependency call-tree and evaluate everything in order in a robust, predictable way.

Okay, fair enough. I don't disagree with the principle at all. I'm just being pragmatic. I'm also not sure if we mentioned it, but I think this is possibly even more problematic:

#ns {
  .mixin();
}
.foo {
  #ns();
  .mixin();  // this is weird
}

You can already do #ns.mixin();, so I'm not even sure why this exists, and it seems like it would lead to not the least astonishment, as you put it.

Okay, I hear you, and I'm fine to keep moving forward to privatizing mixin members in 4.0 or whatever the next breaking change release would be.

@rjgotten
Copy link

rjgotten commented Jul 2, 2018

Don't get me wrong though.

I think the suggestion to mark variables as 'exported' and to alter their precedence, is a good transitionary solution. It will help clean up some mess now, and can lead up to a breaking change in a later major version.

As you wrote: it's pragmatic.

@matthew-dean
Copy link
Member

I think the suggestion to mark variables as 'exported' and to alter their precedence, is a good transitionary solution.

Good to know. It certainly would help simplify the docs in the short term. To be clear, though, this (setting lower precedence to merged vars) would also be a breaking change. But minor.

@matthew-dean
Copy link
Member

Just some clean-up to do on this feature.

1. Deprecating mixin calls without ()

(I think we'll never should allow any DR/mixin/ruleset calls w/o () anymore). - @seven-phases-max

This has been my conclusion too since writing up tests and implementing namespacing. .mixin; should not be deprecated as a mixin call. Fine to mark that in the docs as deprecated?

2. Exports

Rather than re-visiting named exports, what if we just did something simple like returning the last rule's value if the lookup value is empty? As in:

.add(@p, @x) {
    @r: @p + @x;
}
.foo {
  width: .add(10px, 10px)[];  // returns last rule @r's value
}

It's a very simple change in terms of parsing and lookup. Or @seven-phases-max @rjgotten et. all do you think some special character should indicate the last value? Like .add(10px, 10px)[/]?

With something like that, we could serve not having to write the arbitrary return lookup variable name when using lookups for function-like mixins.

3. Other?

Anything else outstanding from this thread not implemented or resolved?

@matthew-dean
Copy link
Member

Relating to #2, I think .add(10px, 10px)[] would be fine. Instead of "last value of name x wins" it's "last value of unnamed value (all) wins".

@rjgotten
Copy link

rjgotten commented Jul 3, 2018

[] works for me.
It avoids the problem where mixin consumers need to be aware of the internals, i.e. the variable name holding the return value.

@matthew-dean
Copy link
Member

matthew-dean commented Jul 3, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants