Detached Rulesets and local scope protection

[SomMeri]

Variables returned from detached ruleset behave differently then variables returned from mixins:

• Variables returned from mixins are copied into caller scope only if they are not present in local scope. However, there is no protection for parent scope.
• Variables returned from detached rulesets are copied into caller scope only if the caller scope does not contain them at all. Both local and parent scope are protected.

Example with detached rulesets:

.wrap-mixin(@ruleset) {
  @b: non-local; // parent scope is protected
  .wrap-selector {
      @ruleset(); // variable returned from detached ruleset is ignored
      visible-one: @b; 
    }
}

.wrap-mixin({
  @b: returned from detached ruleset;
}); // call the whole thing

compiles into:

.wrap-selector {
  visible-one: non-local;
}

Example with mixins:

.ruleset-mixin() {
  @b-mixin: returned from mixin;
}

.wrap-mixin-mixin() {
  @b-mixin: non-local; // parent scope - no protection
  .wrap-selector {
      .ruleset-mixin(); // variable returned from mixin wins over the one defined in parent scope
      visible-one: @b-mixin;
    }
}

.wrap-mixin-mixin(); // call the whole thing

compiles into:

.wrap-selector {
  visible-one: returned from mixin;
}

Expected behavior: mixins and detached rulesets should implement the same caller scope protection. Since detached rulesets are new feature and mixins old one, mixins should win and both of them should protect only local scope.

Tested with lessc 1.7.0 on windows node-js.

[lukeapage]

It's a difficult one.. I kept mixins overriding parent scope because it's
used in the wild. It might be nice to not leak variables at all, but it's
used as a way of doing a return.
When I did detached rulesets I didn't think someone would want to override
a variable in the calling ruleset... They could be clearer by adding a new
param.

Is there a use for this behaviour in detached rulesets?

Do you think we should ever stop mixins returning variables?

[seven-phases-max]

Do you think we should ever stop mixins returning variables?

Probably we could if we have something like #538, but with or without it this would be too harsh breaking change I suspect.

Is there a use for this behaviour in detached rulesets?

Having different variable scoping behaviour for ordinal mixins and detached rulesets is not a good idea regardless of use-cases I guess (that way you just never know what to expect, especially since neither behaviour is ever documented at all except the subtle remark).

[lukeapage]

I know its inconsistent, but given its on the outskirts of most peoples knowledge of less, I'd rather not make it consistent and have less chance of a bug in someones less code than make it consistent and therefore be more likely to negatively effect people.

Thats why I ask if there is a usecase for it, because if there isn't we are making things worse for the sake of consistency.

In terms of documentation we should definitely improve it (and the many other places too)

[seven-phases-max]

In that context I guess I agree, i.e. there's nothing wrong in leaving it as it is now until someone gives a good use-case example where this would look like a show-stopper bug/issue. (I can imagine a few examples of course but in the same time I know how to write those the other way around :).

[SomMeri]

Off-topic: Sorry for late answer, I was too deep in code to answer quickly. Detached rulesets are hardest to port feature so far.

On-topic: I would prefer having them consistent. People will use detached rulesets in the same situations as they use mixins and they will try to use them exactly the same way. Subtle hard to remember differences may lead to more difficult bugs then weaker scope protection.

That being said, I agree that stronger scope protection is superior over the one mixins currently have. It is easier to rename variables about to be returned then those already existing. So, I wont fight for this too much.

"It might be nice to not leak variables at all, but it's used as a way of doing a return."

I agree that it can not be removed without replacement and time to refactor old sheets. It is too useful. Actually, return variables do not bother me at all, altrough ability to make them private or rename them upon return (upon call) might be nice.

Mixins seeing callers scope bother me more, mostly because it unnecessary complicates scoping and is easy to miss. I prefer explicite parameters over implicite, but there is no way back now.

"Is there a use for this behaviour in detached rulesets?"
I do not know. Theoretically, detached rulesets might be easier to read replacement of if the mixin has too many parameters. Maybe, I'm not sure about it. I guess we have to wait to see all creative use cases people will come up with.

"Do you think we should ever stop mixins returning variables?"

No, I suspect it is used a lot and useful.

Shall I close this as wont fix?

[SomMeri]

Closed in favour of less/less-meta#16.

[JawsomeJason]

I'd like to offer a valid use case for this: themed/mapped variables:

// MIXINS FOR SETTINGS/GETTING THEMES
#THEMES {
  .set(@name, @vars) {
    #THEMES {
      .get(@@name) {
        @vars();
      }
    }
  }
}


// ESTABLISH THEMES
#THEMES.set(default, {
  @primary: black;
  @contrast: white;
});

#THEMES.set(inverted, {
  @primary: white;
  @contrast: black;
});

#THEMES.set(error, {
  @primary: white;
  @contrast: red;
});

// use themes
body {
  #THEMES.get(default);
  background-color: @primary;
  color: @contrast;
}

.error {
  #THEMES.get(error);

  color: @contrast;
  background-color: lighten(@contrast, 95%);
  border: 1px solid @contrast;
}

IMO, this is far better and cleaner than the redundancy of @primary-default: ..., @primary-inverted: ... variables.

[seven-phases-max]

@TheJase You don't need DRs for this. Mixins are capable of the same w/o any DRs (and with more clean syntax): #2442 (comment) (and here's corresponding "no DR" equivalent of your snippet above: https://gist.github.com/seven-phases-max/1f917de67753385c920816740b60c39b).

As for the ticket itself: it's still considered an issue in less/less-meta#16 (just for language consistency reasons) - so I'll modify the closing post here accordingly.