Guard expressions regression in 2.6.0

[nborko]

.test when ((8+4) < 13) { 1: 2; }

will error with:

SyntaxError: expected ')' got '<'

[seven-phases-max]

It's most likely because of #2763 (it seems that this kind of expressions is not covered by tests unfortunately).

[seven-phases-max]

A minimal examples to reproduce:

x when ((3) < 4) {
    y: z;    
}
x when ((3) + 4) {
    y: z;    
}
etc.

[SomMeri]

I will have a look at this, next week at latest. Adding some new tests might be a good idea too.

[seven-phases-max]

I'm trying to fix this too (not sure if I'll be capable to do this earlier though - from what I can see so far, we'll definitely need to consider to switch asap from when (true) and (true) or (true) thing to more conventional (true and true or true), because the parsing of irregular and directly opposite parens patterns for logical-and-or-not and logical-anything-else expressions incredibly clutters the code (not counting not-so-friendly-and-easy to explain/understand guards usage in a long run)).

[SomMeri]

You mean not require parenthesis around true and false? It would make sense to me.

[seven-phases-max]

You mean not require parenthesis around true and false?

Yes, but not only. It's actually more about requiring outer parens after when and for anything else parens to be optional. But in either case this would mean a breaking change.
I.e.:

• with "required outer parens", currently valid when (true) and (true) or (true) becomes illegal
• with "optional outer parens", too ambiguous stuff like when true, true > false comes in (sincetrue > false is also a selector).

[SomMeri]

I recall that when is allowed only in rulesets with one selector, so that concrete example of optional outer parens should not be ambigous. It was discussed in some issue a while ago.

[seven-phases-max]

I recall that when is allowed only in rulesets with one selector,

And it's the parser that has to test for , ( pattern and throw a error if it's just , .... So we either have to throw away , as an or operator, or require parens after , (and thus after when in general) at least to distinguish between condition, selector and condition, condition.
A breaking change in any case.

[SomMeri]

@seven-phases-max Cant we just assume that what follows , is another condition, try to parse out that condition and fail only if it is not there? We do not have to use the , ( optimization.

[SomMeri]

Interestingly, these works:

• .test when (8+4 < 13) {}
• .test when (8+(5-1) < 13) {}
• .test when (8 < (13+1))

It seems like it specifically does not allow parenthesis around left side.

[seven-phases-max]

Cant we just assume that what follows , is another condition, try to parse out that condition and fail only if it is not there?

We can. But then someone writes something like:

html when true, 
body {
   font-size: 16px
}

and the compiler interprets it as

html when true, false {
   font-size: 16px;
}

thus silently throwing body away (as it's parsed as valid (keyword) condition evaluating to false).
We still can require () after the comma but it will get us back to:

when true or false {}
when true, (false) {}
// why they are different?

or

when (true or false) {}
when (true), (false) {}
// why they are different?

[seven-phases-max]

Interestingly, these works: .test when (8+4 < 13) {}

Yes, I suppose this is because when we have the first inner parens in when ((x) < 3) the parser enters condition() -> conditionAnd() and stops here thus still returning properly parsed (x) value ( stored as (x = true) condition), but next comes only < 3 which isn't already parseable (since neither of comparision operands can be a condition(), thus the parser can't even try the other way around). <- well, roughly...

And the reverted expression works simply because the parser instantly goes down to this.addition() and "incomplete and/or values" don't interfere there.

[matthew-dean]

thus silently throwing body away

Well sure, but they've written something explicitly unsupported, and thus has unexpected behavior. I don't see the need to force parentheses here. A comma after when is an or. We're explicit about that. But more importantly, the second part here:

thus silently throwing body away (as it's parsed as valid (keyword) condition evaluating to false).

when body should throw an error. None of the docs show that you can put the expression itself without parentheses. The only thing that can be outside of parentheses (after when) are not,and, and or (or comma). So, all of these statements would be illegal:

html when true { }
html when body { }
html when true, 
body { }

If Less is letting those through, that's a bug.

[seven-phases-max]

Well sure, but they've written something explicitly unsupported,

(at #2798 (comment) the discussion starts to be about "the planned behaviour", i.e. #2807).

[matthew-dean]

@seven-phases-max I saw you talking about required parentheses, but in your example you were talking about and and or being illegal outside of parentheses, which it doesn't have to be. (Or did I mis-interpret?) Only comparison and equality operators (and the not keyword) have to be within parentheses, not conditionals.

[matthew-dean]

That is:

when true or false {}   // should fail
when true, (false) {}    // should fail
when (true or false) {}   // should pass
when (true), (false) {}   // should pass

[seven-phases-max]

being illegal outside of parentheses, which it doesn't have to be.

It depends on what we're about. If it's about the current behaviour then there's no , issue, nothing to discuss.
If we're about #2807, then there're two variants as in #2798 (comment). The first variant bans the comma at all, and the second variant is about to ban it too (otherwise the mentioned ambiguity jumps in).

[matthew-dean]

The first variant bans the comma at all, and the second variant is about to ban it too (otherwise the mentioned ambiguity jumps in).

I don't see how. o_O Any comma after a when and before { is part of the when statement.

[seven-phases-max]

Any comma after a when and before { is part of the when statement.

It's not even true for the current behaviour. And especially it doesn't have to be for the planned one.

[matthew-dean]

@seven-phases-max

It's not even true for the current behaviour.

Then that's a bug. when statements can have commas. They shouldn't be treated differently than mixin guards.

[SomMeri]

@matthew-dean @seven-phases-max I think that comma after when inside ruleset is a bug, but different one then this one. It might even be harder to fix.

[seven-phases-max]

@matthew-dean @SomMeri
I don't see any bug in the example of #2798 (comment).
It's actually just a more user-friendly error message for the ambiguous syntax - if you remove that special test for , (, the error becomes just unrecongnised input, expected condition (is that what would you expect?).

[matthew-dean]

@seven-phases-max By a bug, I mean that it's weird (unexpected) that a mixin can have a comma-guard and a selector can't. That said, per: #2807 (comment), let's get rid of the comma soon.

[SomMeri]

I considered it a bug for the same reason as @matthew-dean - because it is different then in mixin. I agree with deprecating comma for 3.0.

[seven-phases-max]

it's weird (unexpected) that a mixin can have a comma-guard and a selector can't.

I guess you both are missing that x ruleset there is actually valid and compiles fine, it's only the invalid y that throws the error. So still no real bug there (the example is only used to illustrate the ambiguity if we would allow the "all-parens-optional" syntax and don't ban comma).

[matthew-dean]

I guess you both are missing that x ruleset there is actually valid and compiles fine, it's only the invalid y that throws the error. So still no real bug there

The bug would be that y should not be invalid if y is a valid when condition.

If there's consensus, though, we could mark that bug as "wontfix" until the comma is deprecated / removed. The docs don't yet mention the "or" keyword though.

[SomMeri]

@matthew-dean I created new issue for that in docs #396.

Btw, it is great that less.js is getting more active lately 👍 .