Property accessors for Less for Issue #2433

[matthew-dean]

Sorry about messing up the repo for a bit. (If no one noticed, yay!) I committed to the wrong one first. Ah, git.

So! About the property accessors: this works, with some caveats.

1. A property's values always evaluate, instead of dumping into an anonymous value if not assigned to a variable (if I'm interpreting that correctly).
2. Because the values evaluate, even though the output CSS is correct, it causes the CSS tests to fail.

So, feedback wanted on this. And since this is my first pull request for a new feature, let me know if there are some steps I missed.

Addresses issue / feature #2433

[matthew-dean]

Also, there are some places where the curly-braced referencing didn't make sense (to me) for properties while it did for variables, such as within selectors. So they behave mostly like vars but there are a few differences.

[seven-phases-max]

Good work!

---

I did a few primarily tests and so far noticed the following uncertainties:

---

[a]. recursion tolerance:

a {
    color: $color;
    color: green;
}

I wonder if this should compile at all. Well, at first glance there's nothing wrong with this code, it works just because the value of the first property is not even considered when this property is evaluated itself. It's just contra-intuitive (and may lead to a lot of confusions eventually, e.g. see [c] below).

---

[b]. The Implementation does not consider property merging, e.g.:

a {
    color+: red;
    color+: foo;

    &b {
        background: $color;
    }
}

results in:

a {
  color: red, foo;
}
a b {
  background: foo;
}

I.e. property evaluation code should handle merging on its own (e.g. by applyingless.visitors.ToCSSVisitor.prototype._mergeRules to the (copy of?) currently evaluated ruleset/scope).

---

[c]. Just an edge case test to illustrate why I think that any recursion should be explicitly forbidden:

a {
    color+: $color;
    color+: foo;
}

b {
    color+: $color;
    color+: foo;
    color:  bar;
}

These snippets currently result in all sort of weirdness because of [b], but I suppose they will become even more weird when [b] is implemented. (I remember we had a few examples like that in some other thread - I can't find it right now though). I.e. a combo of (already recursive-like) merging, fallback properties and "self-referencing" may be too unpredictable to not forbid the latter early (e.g. I mean I;m thinking of any color: $color; to always throw a error no matter what), otherwise later it may be quite painful to explain why some combinations like above are compiled just fine and some very similar things are not.
On the other hand I can imagine contra-argument like:

div {
    color: fade($color, 100%); // IE8 fallback
    color: rgba(1, 2, 3, .4);
}

[matthew-dean]

Ah, oops, I meant to test for property merging and then forgot by the time I was trying to get something together for the end of the day.

The recursion thing I actually did notice, but I wasn't sure either if it mattered. There is actually a recursion error, I think, if a property references itself when no other properties are present.

And, like your very last example, that may actually make a certain kind of sense. That is, properties are slightly different from variables, in that a variable can really only logically have one final value, and any other prior variable assignments are moot. Properties end up with many values. We still pick the last one for referencing because otherwise it's mass hysteria, cats and dogs living together and stuff like that. That is, it keeps the behavior simple. So, if a value is available, it makes sense to be able to reference it. In that way, the property isn't referencing itself the way a variable is referencing itself. It's referencing the last available value assigned to that property name. It's a slight difference, but enough that the behavior could go either way. So I think this might be more a question of what someone might expect. I'm personally inclined to the behavior where an error is not triggered, just because it's less jarring to have success if someone tries it than an error if that's the behavior they want. But I prefer error-correcting parsers rather than error-throwing parsers, so maybe that's just me.

The property merging though I'll have to fix.

[matthew-dean]

To address your other examples.

a {
    color+: $color;
    color+: foo;
}

b {
    color+: $color;
    color+: foo;
    color:  bar;
}

[a] is a clear error, because the final property value can't be evaluated since a part of that value includes the value, which is a difference from referencing the last value.

[b] ... well, it still seems fairly evaluatable. The final value of color is "bar". Properties 1 and 2 are merged with that as the substituted value, therefore the output would be:

b {
    color: bar, foo;
    color: bar;
}

There are a lot of edge cases you could get like that where the value is not entirely useful, but still seems fairly predictable.

[seven-phases-max]

The property merging though I'll have to fix.

It should be relatively easy (for example see https://github.com/seven-phases-max/less-plugin-functions/blob/v0.0.2/lib/main.js#L107... I.e. I suppose you just need to put such call on current frame.rules somewhere around find function or so).

[matthew-dean]

Awesome. Thanks for the tips. This is my first time diving deep into the codebase. (I figured I was overdue for putting some time in at actual coding.)

What about the failing CSS tests? As far as I can tell, evaluating all values so that those values are the right type when passed to a function works, and produces valid CSS, but changes the structure because of the CSS generation being different. Is transforming those values to anonymous ones done for performance? Or is it just so that people can have pass-through structure if they want? (Or both.) Does the rule essentially circumvent the parser if the value does not reference a variable (nor is assigned to a variable)?

I tried for a bit to evaluate values and determine types at call time. (In other words, leave the value anonymous until it's referenced), but I'm not sure how to do that without reparsing the value. Or if that's even possible at all.

[seven-phases-max]

Is transforming those values to anonymous ones done for performance?

I suppose so (can't imagine any other reason). So yes I'm afraid if we about to add this feature we have to abandon such optimization and always parse properties too (and then just fix failing tests accordingly).

I tried for a bit to evaluate values and determine types at call time.

I did it in the functions plugin and it works (but for sure my code does not handle all possible edge cases and I doubt something like that would make sense in the core... Though if it could help to not kill performance... Hmm. Need tests to see how bad the performance drop actually is).

[matthew-dean]

I think I did it!

[lukeapage]

I think I did it!

There are a few jshint errors outstanding on the tests - if you click on the failed checks you can see the build. If it was a build failure you should have permission to restart.

[matthew-dean]

@lukeapage Okay, thanks, I will take a look at the jshint errors soon. I didn't catch that.

[matthew-dean]

@lukeapage Committed update with all tests passing

[seven-phases-max]

Adding v3 label per less/less-meta#10. Though in fact I don't mind merging this before v3 (after all it's not really a breaking change, maybe with just marking it as "yet experimental" feature). The only thing I'm concerned is performance. Simple grunt benchmark would be fine (it's pretty coarse test since it shows performance of "fully-warmed-up" V8 performance contrary to "first-run" one we usually have with lessc, but still it should give an overall picture... Just to make sure it did not become twice slower :)

[matthew-dean]

@seven-phases-max Your performance concerns are not invalid. Shelving this until I have a chance to really dig into benchmarking.

[matthew-dean]

Closing since this was merged at some other place.