Default function and conflicts detection

[SomMeri]

1.) This is considered to be ambiguous, but I do not think it should be:

guard-default-expr-not {
    .m(@x)                      {case:     1}
    .m(@x) when (default())     {default: @x}
    .m(@x) when not(default())  {not-default: @x}

    .m(2);
}

error:

RuntimeError: Ambiguous use of `default()` found when matching for `.m(2)` in test.less on line 6, column 5:
5
6     .m(2);
7 }

Expected result:

guard-default-expr-not {
  case: 1;
  not-default: 2;
}

2-question.) This is considered ambiguous too, was that intentional? It more or less makes sense either way.

guard-default-expr-not {
    .m(@x) when not(default())  {not-default-1: @x}
    .m(@x) when not(default())  {not-default-2: @x}

    .m(2);
}

[SomMeri]

3.) I think that this should compile without errors too:

guard-default-expr-not {
    .m(@x)                      {case:     1}
    .m(@x) when not(default())  {not-default-1: @x}
    .m(@x) when not(default())  {not-default-2: @x}

    .m(2);
}

returned error:

RuntimeError: Ambiguous use of `default()` found when matching for `.m(2)` in test.less on line 6, column 5:
5
6     .m(2);
7 }

expected result:

guard-default-expr-not {
  case: 1;
  not-default-1: 2;
  not-default-2: 2;
}

[seven-phases-max]

I'm agreed with all three.
Yes, currently the multiple defaults conflict detection algorithm is very primitive, strictly speaking there's no algorithm at all. Internally the rule is as dumb as "no more then one default() per mixin call" (L61). When we planned this feature it was supposed to not allow multiple default() calls at all, but I decided to allow certain cases just as a free bonus (well, mostly because those allowed cases are really "code-free"). Here's a brief of how it works currently:

brief {
    .m(2);

    .m(3)  when (default()) {}
        // ^ this one is not even matched so it does not affect other definitions
    .m(@x) when (default()), true {}
        // ^ this one is always true so it is not marked as using default()
    .m(@x) when (default()), not(default()) {} 
        // ^ this one is always true so it is not marked as using default()
    .m(@x) when (default()) and false {}
        // ^ this one is always false so it is not marked as using default()
    .m(@x) when (default()) and not(default()) {}
        // ^ this one is always false so it is not marked as using default()
    // etc.

    .m(@x) when (default()) {}
        // ^ now this one uses default with (yet) "unknown" value (marked as using default())

    .m(@x) when not(default()) {}
        // ^ another default() with "unknown" value? -> ERROR

    .m(@x) {}
}

It's really primitive. (So there I put that "potential conflict" remark :)
So "Ambiguous use of default() found when matching for" error is really just "Sorry I can't understand it, too complicated" :) Or in other words, the current implementation can be thought as more like "I can detect all incorrect cases but cannot detect all correct combinations".

---

Technically I think it is possible to support multiple default calls correctly (as in above examples and similar cases) but this will require a full-featured analysis of all possible default() return value combinations somewhere at this point. Honestly i did not even try to craft such analysis in #1606 because at that time we weren't even sure if this feature is worth to be in at all so my main goal was to make it is as simple as possible (while meeting all base requirements like "independency of definition order" etc.).

[SomMeri]

I have seen less.js code as I am porting it into java and actually found it clever. I really liked this trick. I think that main problem is that this exception is premature.

What I'm trying to do is to categorize found mixins into categories and count how many of them are in each category:

• require default() to be true,
• require default() to be false,
• default() does not matter and patterm match,
• pattern does not match.

Then I analyze numbers, somewhat like this:

• some "default() does not matter" mixins are available -> use only "default() does not matter" and "default() must be false" mixins,
• multiple "default() must be false" mixins are available -> the same as above,
• some "pattern does not match" and exactly one "default must be true" available --> use "default must be true",
• some "pattern does not match" and multiple "default must be true" available --> error,
• etc.

I'm in the middle of trying to figure out those conditions :). So, if you want, I can either post them once I think it works and then reproduce them in javascript.

Edit: so basically I'm doing what you suggests in your comment.

[seven-phases-max]

I think that main problem is that this exception is premature.

Yes of course, for the analysis algorithm to work this L61 error condition needs to be removed.

So, if you want, I can either post them once I think it works and then reproduce them in javascript.

Are you doing that in context of less4j? I'll be glad to port it back to javascript (though I'm not too familiar with Java I think I can read the code).

[seven-phases-max]

Btw., I thought a bit more of (2) and indeed, this case makes sense in either way ("both expanded" and "consdered ambiguous"). It's hard to say which one would be more useful in isolation from possible use-cases... Maybe we could leave it "ambiguous" yet? (Assuming that its "ambiguity" detection moves to the new algorithm of course). But it's not that important yet again so either "just easier to detect" hadling will be OK.

[SomMeri]

I think I got it here: https://github.com/SomMeri/less4j/blob/master/src/main/java/com/github/sommeri/less4j/core/compiler/stages/MixinsSolver.java#L114 Only mixins with right name, right number of parameters and matching patter enter the method (as third mixins argument not visible in github gui)

This is where I store candidates: https://github.com/SomMeri/less4j/blob/master/src/main/java/com/github/sommeri/less4j/core/compiler/stages/MixinsSolver.java#L120

Each mixin is categorized as one of: DEFAULT_OBLIVIOUS, ONLY_IF_DEFAULT, ONLY_IF_NOT_DEFAULT

• DEFAULT_OBLIVIOUS - guard is true no matter what value of default() method
• ONLY_IF_DEFAULT - guard is true only if default() returns true
• ONLY_IF_NOT_DEFAULT - guard is true only if default() returns false

Which categories to use is evaluated on this line using logic implemented in this method.

Logic:

• First, if there were multiple ONLY_IF_DEFAULT mixins - throw error.
• If there is exactly one ONLY_IF_DEFAULT mixin and nothing else, use that one mixin.
• If there is at least one DEFAULT_OBLIVIOUS mixin or multiple ONLY_IF_NOT_DEFAULT mixins, use only DEFAULT_OBLIVIOUS and ONLY_IF_NOT_DEFAULT mixins

At that point, we know that:

• if there is ONLY_IF_DEFAULT mixin, then there is something else with it too.
• there are no normal mixins and there is maximum one ONLY_IF_NOT_DEFAULT mixin

Therefore, we there are either:

• exactly one ONLY_IF_DEFAULT mixin and one ONLY_IF_NOT_DEFAULT mixin,
• or exactly one ONLY_IF_NOT_DEFAULT mixin,
• or nothing.

The rest of logic:

• If there is one ONLY_IF_DEFAULT mixin and one ONLY_IF_NOT_DEFAULT mixin, return that ONLY_IF_NOT_DEFAULT mixin.
• If there is only one ONLY_IF_NOT_DEFAULT mixin, then we can not use it.
• If there is nothing, then we use nothing.

[SomMeri]

I wrote all that yesterday evening, so your last comment is not implemented there. However, it should be easy to change that. If you want, I will change it, but now I run out of time now.

The change would go here.

Unit tests (less):

• copy of less.js original tests,
• additional cases,
• error handling.

Expected css is in the same directory in file with the same name and css suffix:

• copy of less.js original tests,
• additional cases,
• .err suffix - contains expected errors.

[seven-phases-max]

Oh, thanks! I'll try to implement this in js.

[SomMeri]

I updated my implementation to check for multiple not default here

It now stars by:

• First, if there were multiple ONLY_IF_DEFAULT mixins - throw error.
• Second, if there were multiple ONLY_IF_DEFAULT

However, while doing that, I found this (so the condition is not completely done).

If this is supposed to fail - and fails currently in less.js:

guard-default-expr-not {
    .m(1)                      {case:     1}
    .m(@x) when not(default()) {default: @x}
    .m(@x) when not(default()) {default: @x}

    &-2 {.m(2)}
}

shouldnt this fail too?

guard-default-expr-not {
    .m(1)                      {case:     1}
    .m(@x) when not(default()) {default: @x}

    &-2 {.m(2)}
}

and this too:

guard-default-expr-not {
    .m(@x) when not(default()) {default: @x}

    &-2 {.m(2)}
}

Last to two less files compile into nothing.

[seven-phases-max]

By "fail" do you mean throwing a error?
I would expect the last two to be silent (this is just how they're written: "do nothing if I'm alone"), could you please elaborate on why do you think they should fail? The first example is more "ambiguous" because there it's not quite evident if two (or more) not(default())s should "force" each other. (Hypothetically I imagine a use-case for a not(default()) mixin to be like a sort of "attachment" for a normal mixin and in that context it would be undesired for two not(default())s to unlock each other. But all this is very .. yes, hypothetical... So if you have some other ideas I'd be happy to rethink).

[SomMeri]

@seven-phases-max I meant throwing an error. It seemed to be logical, but now I think I just got confused. There were too many possibilities. Alright, it is as it is and I'm not going to touch it further.

[seven-phases-max]

@SomMeri
It took a bit longer than I hoped. I just entered a world of infinite uncertainties, at first about implementation (structured but verbose vs. short but cryptic) and then about the algorithm itself and all those cases... Had to re-think it all over again and again. Now I guess I have a code I'm satisfied with more or less.
Good news: the logic is very simple while still (I hope) covering all cases mentioned earlier.
Bad news: Besides (expected) minor incompatibilities with current Less version implementation, there're also a few differences from the current less4j implementation.

In summary:

Changes from current less.js:

The conflict detection is less restrictive now, it does not throw a error until conflicting conditions are actually about to be expanded. The most notable example:

.m(1)                   {}
.m(@x) when (default()) {}
.m(@x) when (default()) {}

.m(1); // OK (was Err. before)
.m(2); // Err.: ambiguous

Differences from less4j:

(A) Additional examples has the following test case marked as not ambiguous (unfortunately I noticed it only when I've started to update/collect tests):

not-ambiguos-1 {
    .m(@x) when (default())    {default: @x}
    .m(@x) when not(default()) {not-default: @x}

    .m(2);
}

But it is ambiguous. There both conditions recursively depend on each other. Both default() calls can't return true or false without changing the result of the other and therefore reverting back its own return value.

And several examples in error handling file:

(B) L8: same as (A).

(C) L21: No conflict there because the first unconditional mixin makes all default() calls return false.

(D) L55: Produces error. It's a bit tricky, the first mixing can't return true because that would make all three mixins to expand (i.e. making returned 'true' not valid), so it could return false but this makes the whole snippet to be equal to L41 (i.e. considered as ambiguous).

(E) L56: Should produce error (it's the same code as L41).

[lukeapage]

Can we close this now? I think its into the realms of not going to affect anyone?

[seven-phases-max]

Yes, I guess we can close this (and we can always re-open if something new of the subject pops up).

[SomMeri]

@seven-phases-max Thank you for detailed description. "Infinite uncertainties" is perfect description to what happened to my brain while trying to implement this.