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

3 layers of mixin invocations with variables across scopes produces duplicated broken output #2350

Closed
SLaks opened this issue Dec 21, 2014 · 9 comments

Comments

@SLaks
Copy link
Contributor

SLaks commented Dec 21, 2014

Source:

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }
    .Define(First, red);
    .Define(Second, green);
    .Define(Third, blue);
    .Define(Fourth, black);
}

Actual result:

a {
  output: First red #ffcccc;
  output: First green #4dff4d;
  output: First blue #ccccff;
  output: Second blue #ccccff;
  output: First black #666666;
  output: Second black #666666;
  output: Third black #666666;
}

Expected result:

a {
  output: First red #ffcccc;
  output: Second green #4dff4d;
  output: Third blue #ccccff;
  output: Fourth black #666666;
}

Workarounds:
This seems to be caused by mixing variables from different scopes.
Removing the @name from the output: property, or inlining the InvokerCallback() mixin into Invoker() fixes it.
Moving .Invoker() outside the ruleset also fixes it.

Motivation:

In my actual (messier) code, .Invoker() is a generic loop mixin which iterates over a list parameter comes and calls InvokerCallback() separately for each item in the list, and the entire thing is a mixin with a further callback that lets me apply theming to different elements.

This lets me define a number of names for each color in a single call to Define().

This whole thing would become much saner if you add support for lambdas (#2270)

@seven-phases-max
Copy link
Member

This looks like more curvy variant of #1291 (examples there were fixed at some point near v1.7 but since that was unintentional fix it may appear still in more complex cases).
E.g. notice even more curious behaviour with:

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }

    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }

    foo {.Define(First, red);}
    .Define(Second, green);
}

So, yes, currently it's important to avoid using outer mixin parameters within inner mixins (at least when it comes to complex scope hierarchies).

This whole thing would become much saner if you add support for lambdas (#2270)

See also related discussion started at #1648 (comment) (as well as the initial #965). Most likely that the another variant ("passing mixins by reference") would suit parametric callbacks better (so that instead of defining quite heavy on syntax (at least like it looks in #2270) "parametric detached rulesets" we would be allowed use ordinal mixins as explicit callbacks at will).

@SLaks
Copy link
Contributor Author

SLaks commented Dec 21, 2014

Your "more curious behavior" isn't all that much more curious; it's the same thing that happens if you swap the order without foo{}.

@seven-phases-max
Copy link
Member

Change foo to & and you'll see that the fun part is not in "swapping" (well, either way the purpose of that my example is to demonstrate it's the same problem as #1291).

@SLaks
Copy link
Contributor Author

SLaks commented Dec 21, 2014

See https://github.com/SLaks/SLaks.Blog/blob/gh-pages/css/_colors.less for my actual use case; I'm replicating lambdas by passing 5 "parameters" as variables from the callsite for the ruleset callback.

@lukeapage
Copy link
Member

its not so unexpected

Lets do the first call

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }
    .Define(First, red);
    .Define(Second, green);
    .Define(Third, blue);
    .Define(Fourth, black);
}
a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }
    .InvokerCallback(@baseColor, @foreColor) {
        output: First @baseColor @foreColor;
    }
    output: First red...;

    .Define(Second, green);
    .Define(Third, blue);
    .Define(Fourth, black);
}

Now the 2nd call - Note that we have included InvokerCallback in the scope already, so it is called again

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }
    .InvokerCallback(@baseColor, @foreColor) {
        output: First @baseColor @foreColor;
    }
    output: First red...;
    .InvokerCallback(@baseColor, @foreColor) {
        output: Second @baseColor @foreColor;
    }
    output: First green...;
    output: Second green;

    .Define(Second, green);
    .Define(Third, blue);
    .Define(Fourth, black);
}

Now if multiple mixins match within a scope, they are all called e.g.

.b {
   .fall();
}

.fall() {
  color: red;
}

.fall() {
  color: green;
}

becomes

.b {
  color: red;
  color: green;
}

So my expected would be

a {
  output: First red #ffcccc;
  output: First green #4dff4d;
  output: Second green #4dff4d;
  output: First blue #ccccff;
  output: Second blue #ccccff;
  output: Third blue #ccccff;
  output: First black #666666;
  output: Second black #666666;
  output: Third black #666666;
  output: Fourth black #666666;
}

and there is definitely something weird going on because we don't get that, but I wouldn't expect what you got.

@seven-phases-max
Copy link
Member

Curiouse. Actually if we assume the code after the first call to be equal to:

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }
    .InvokerCallback(@baseColor, @foreColor) {
        output: First @baseColor @foreColor;
    }

    .Define(Second, green);
}

(which indeed seems to be logical) then the original result becomes expected because in this case the a scope (parent scope of .Invoker) has higher precedence (see #1316) than the scope of .Define (caller scope of .Invoker). Hence .Define(Second, green); expands only to output: First green #4dff4d; and so on. :)

So this becomes closely related to #1316 stuff (just not for parametric/non-parametric difference but for expecting different non-local scope precedences for different use-cases).

@seven-phases-max
Copy link
Member

So I'm actually closing this as "currently expected behaviour".

As I mentioned above it's just: Inside .Invoker, .InvokerCallback definitions expanded in to a scope by previous .Define calls get higher precedence than .InvokerCallback definition of the next .Define call (because the caller scope has the lowest precedence, thus previously generated a.InvokerCallback {...} > new .Define.InvokerCallback {...}).

For a detailed explanation of how scope precedences work for mixin expansions see #2435 (comment). For an overall discussion around global/parent > caller behaviour and (not-)possible changes see #2212 and #1316.


Workarounds for this particular example:
[1] Isolate each .Define call:

a {
    .Invoker(@color) {
        .InvokerCallback(@color, lighten(@color, 40%));
    }
    .Define(@name, @color) {
        .InvokerCallback(@baseColor, @foreColor) {
            output: @name @baseColor @foreColor;
        }
        .Invoker(@color);
    }

    & {.Define(First,  red)}
    & {.Define(Second, green)}
    & {.Define(Third,  blue)}
    & {.Define(Fourth, black)}
}

[2] Use DRs instead of caller callbacks.

a {
    .Invoker(@color, @apply) {
        @baseColor: @color;
        @foreColor: lighten(@color, 40%);
        @apply();
    }

    .Define(@name, @color) {
        .Invoker(@color, {
            output: @name @baseColor @foreColor;
        });
    }

    .Define(First,  red);
    .Define(Second, green);
    .Define(Third,  blue);
    .Define(Fourth, black);
}

@seven-phases-max
Copy link
Member

Closing as "expected behaviour".

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

No branches or pull requests

3 participants