Merge pull request #1823 from seven-phases-max/mixin-guard-default-2

Improved multiple `default()` guards conflict detection.
less · Jan 22, 2014 · b637360 · b637360
2 parents 6e14080 + 5146c1b
commit b637360
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 34 deletions.
diff --git a/lib/less/tree/mixin.js b/lib/less/tree/mixin.js
@@ -19,8 +19,9 @@ tree.mixin.Call.prototype = {
         }
     },
     eval: function (env) {
-        var mixins, mixin, args, rules = [], match = false, i, m, f, isRecursive, isOneFound, rule;
-        var candidates = [], candidate, conditionResult = [], defaultFunc = tree.defaultFunc, defaultUsed = false; 
+        var mixins, mixin, args, rules = [], match = false, i, m, f, isRecursive, isOneFound, rule,
+            candidates = [], candidate, conditionResult = [], defaultFunc = tree.defaultFunc,
+            defaultResult, defNone = 0, defTrue = 1, defFalse = 2, count; 
 
         args = this.arguments && this.arguments.map(function (a) {
             return { name: a.name, value: a.value.eval(env) };
@@ -49,7 +50,7 @@ tree.mixin.Call.prototype = {
                     }
 
                     if (mixin.matchArgs(args, env)) {  
-                        candidate = {mixin: mixin};
+                        candidate = {mixin: mixin, group: defNone};
 
                         if (mixin.matchCondition) { 
                             for (f = 0; f < 2; f++) {
@@ -58,20 +59,8 @@ tree.mixin.Call.prototype = {
                             }
                             if (conditionResult[0] || conditionResult[1]) {
                                 if (conditionResult[0] != conditionResult[1]) {
-                                    if (defaultUsed) {
-                                        // todo: ideally, it would make sense to also print the candidate 
-                                        // mixin definitions that cause the conflict (current one and the
-                                        // mixin that set defaultUsed flag). But is there any easy method 
-                                        // to get their filename/line/index info here?
-                                        throw { type: 'Runtime',
-                                            message: 'Ambiguous use of `default()` found when matching for `'
-                                                + this.format(args) + '`',
-                                            index: this.index, filename: this.currentFileInfo.filename };
-                                    }
-
-                                    defaultUsed                   = true;
-                                    candidate.matchIfDefault      = true;
-                                    candidate.matchIfDefaultValue = conditionResult[1];
+                                    candidate.group = conditionResult[1]
+                                        ? defTrue : defFalse;
                                 }
 
                                 candidates.push(candidate);
@@ -86,25 +75,35 @@ tree.mixin.Call.prototype = {
                 }
 
                 defaultFunc.reset();
+
+                count = [0, 0, 0];
+                for (m = 0; m < candidates.length; m++) {
+                    count[candidates[m].group]++;
+                }
+
+                if (count[defNone] > 0) {
+                    defaultResult = defFalse;
+                } else {
+                    defaultResult = defTrue;
+                    if ((count[defTrue] + count[defFalse]) > 1) {
+                        throw { type: 'Runtime',
+                            message: 'Ambiguous use of `default()` found when matching for `'
+                                + this.format(args) + '`',
+                            index: this.index, filename: this.currentFileInfo.filename };
+                    }
+                }
 
                 for (m = 0; m < candidates.length; m++) {
-                    candidate = candidates[m];
-                    if (!candidate.matchIfDefault || (candidate.matchIfDefaultValue == (candidates.length == 1))) {
+                    candidate = candidates[m].group;
+                    if ((candidate === defNone) || (candidate === defaultResult)) {
                         try {
-                            mixin = candidate.mixin;
+                            mixin = candidates[m].mixin;
                             if (!(mixin instanceof tree.mixin.Definition)) {
                                 mixin = new tree.mixin.Definition("", [], mixin.rules, null, false);
                                 mixin.originalRuleset = mixins[m].originalRuleset || mixins[m];
                             }
-                            //if (this.important) {
-                            //    isImportant = env.isImportant;
-                            //    env.isImportant = true;
-                            //}
                             Array.prototype.push.apply(
                                   rules, mixin.eval(env, args, this.important).rules);
-                            //if (this.important) {
-                            //    env.isImportant = isImportant;
-                            //}
                         } catch (e) {
                             throw { message: e.message, index: this.index, filename: this.currentFileInfo.filename, stack: e.stack };
                         }

diff --git a/test/css/mixins-guards-default-func.css b/test/css/mixins-guards-default-func.css
@@ -112,6 +112,15 @@ guard-default-multi-4 {
   always: 2;
   case: 2;
 }
+guard-default-not-ambiguos-2 {
+  case: 1;
+  not-default: 2;
+}
+guard-default-not-ambiguos-3 {
+  case: 1;
+  not-default-1: 2;
+  not-default-2: 2;
+}
 guard-default-scopes-3 {
   3: when default;
 }

diff --git a/test/less/errors/mixins-guards-default-func-1.less b/test/less/errors/mixins-guards-default-func-1.less
@@ -1,6 +1,6 @@
 
 guard-default-func-conflict {
-    .m(@x, @y)                 {}
+    .m(@x, 1)                  {}
     .m(@x, 2) when (default()) {}
     .m(@x, 2) when (default()) {}
 

diff --git a/test/less/errors/mixins-guards-default-func-3.less b/test/less/errors/mixins-guards-default-func-3.less
@@ -0,0 +1,9 @@
+
+guard-default-func-conflict {
+    .m(1)                      {}
+    .m(@x) when not(default()) {}
+    .m(@x) when not(default()) {}
+
+    .m(1);
+    .m(2);
+}
diff --git a/test/less/errors/mixins-guards-default-func-3.txt b/test/less/errors/mixins-guards-default-func-3.txt
@@ -0,0 +1,4 @@
+RuntimeError: Ambiguous use of `default()` found when matching for `.m(2)` in {path}mixins-guards-default-func-3.less on line 8, column 5:
+7     .m(1);
+8     .m(2);
+9 }
diff --git a/test/less/mixins-guards-default-func.less b/test/less/mixins-guards-default-func.less
@@ -117,18 +117,18 @@ guard-default-expr-never {
 // not conflicting multiple default() uses:
 
 guard-default-multi-1 {
-    .m(0)                      {case:       0}
-    .m(@x) when (default())    {default-1: @x}
-    .m(2)  when (default())    {default-2: @x}
+    .m(0)                   {case:       0}
+    .m(@x) when (default()) {default-1: @x}
+    .m(2)  when (default()) {default-2: @x}
 
     &-0 {.m(0)}
     &-1 {.m(1)}
 }
 
 guard-default-multi-2 {
-    .m(1, @x)   when (default())  {default-1: @x}
-    .m(2, @x)   when (default())  {default-2: @x}
-    .m(@x, yes) when (default())  {default-3: @x}
+    .m(1, @x)   when (default()) {default-1: @x}
+    .m(2, @x)   when (default()) {default-2: @x}
+    .m(@x, yes) when (default()) {default-3: @x}
 
     &-1 {.m(1, no)}
     &-2 {.m(2, no)}
@@ -158,6 +158,22 @@ guard-default-multi-4 {
     .m(2);
 }
 
+guard-default-not-ambiguos-2 {
+    .m(@x)                     {case:         1}
+    .m(@x) when (default())    {default:     @x}
+    .m(@x) when not(default()) {not-default: @x}
+
+    .m(2);
+}
+
+guard-default-not-ambiguos-3 {
+    .m(@x)                     {case:           1}
+    .m(@x) when not(default()) {not-default-1: @x}
+    .m(@x) when not(default()) {not-default-2: @x}
+
+    .m(2);
+}
+
 // default & scope
 
 guard-default-scopes {