Skip to content

Typesafety: assert function to fail at compile time #2747

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

Closed
slorber opened this issue Dec 3, 2015 · 22 comments
Closed

Typesafety: assert function to fail at compile time #2747

slorber opened this issue Dec 3, 2015 · 22 comments

Comments

@slorber
Copy link

slorber commented Dec 3, 2015

Hi,

What I'd like to do is something like:

.background-clip(@value) when (@value = border-box),
                              (@value = padding-box),
                              (@value = content-box),
                              (@value = inherit)
{
    -webkit-background-clip: @value;
       -moz-background-clip: @value;
            background-clip: @value;
}

When the background-clip is called with a wrong value, instead of ignoring that value, it could be nice to make the compilation fail.

Is it currently possible? or could it be in the future?

It would be really great to have a syntax to tell that the guard is a "compilation guard" and not simply "ignore-guard"

One simple solution i can think of is using a function that may fail. Something like:

.background-clip(@value) when (oneOf(@value,"border-box","padding-box","content-box","inherit")
{
            background-clip: @value;
}

The idea is that the function acts like an "assertion" that may make the compilation fail.

As far as I see there are already such functions that can make the compilation fail.

It would be nice to have more compilation safety with a oneOf function.

There are probably other useful options, like ensuring a list contains only know values.

For example when dealing with interaction modes, which can generally be mouse and/or touch. We have to handle special cases because the :hover is not possible on devices with only touch support.

.myMixin(@interactionMode) when (listOf(@interactionMode,"touch","mouse")
{

   div.someButton {
        & when (contains(@interactionMode,"touch")) {
            // ...
        }
        & when (contains(@interactionMode,"mouse"))  {
           &:hover {
                       // ...
           }
        }
        & when (contains(@interactionMode,"mouse","touch")) {
            // ...
        }
   }
}

The contains() function would be useful there :)

Maybe we could also pass as mixin arguments complex json objects and assert in functions that they have a certain "shape"

I don't know which functions would be useful exactly but I think it would be a nice progress for less to have more "typesafety".

@slorber
Copy link
Author

slorber commented Dec 3, 2015

Notice I tried to use this hack to declare js functions in less without success (inside a guard)

#637

@slorber
Copy link
Author

slorber commented Dec 3, 2015

Another idea could be this kind of syntax:

.mixin (@a) when (lightness(@a) >= 50%) {
  background-color: black;
}
.mixin (@a) when (lightness(@a) < 50%) {
  background-color: white;
}
.mixin (@a) {
  fail("This mixin can't be called with this value",@arguments);
}

@slorber
Copy link
Author

slorber commented Dec 3, 2015

Ok so I think @seven-phases-max has an answer for everything :)

As far as I understand I can provide custom functions by using a less plugin.

Here is a List plugin from which I can inspire myself to build some kind of "less-assertions-plugin"

https://github.com/seven-phases-max/less-plugin-lists

@SomMeri
Copy link
Member

SomMeri commented Dec 3, 2015

This might be related to #1459 . It is not exactly the same thing, but has similar goal.

@seven-phases-max
Copy link
Member

I think it's easier to introduce some "assert" function and use it like:

@unused: assert(@condition, "doh!");

instead of inventing some special syntax.

Taking the first use-case into account, less-plugin-lists misses some index-of function or so, to be used together with such assert (currently functions there follow "Don't raise a error if the function fails" convention). But sure one easily can make some function to fit either needs.
(Still I would not mind a built-in assert).

@slorber slorber changed the title Typesafety: Mixin guards to help fail at compile time (enumeration, union types, ...) Typesafety: assert function to fail at compile time Dec 3, 2015
@slorber
Copy link
Author

slorber commented Dec 4, 2015

Yes a built-in assert would permit to reuse existing functions that do not throw!

Could we do something like this?

.mixin (@list) when (assert( lengt(@list) < 3 )) {
  ...
}

@slorber
Copy link
Author

slorber commented Dec 4, 2015

@seven-phases-max I've done some tests and it seems that less can support quite complex expressions in a when clause, so definitively some simple assertion functions would be a huge win!

However I'm still interested in a case to make less compilation simply fail anyway.

See for example how ScottS here had the idea to call a mixin that does not really exist to make less fail (option 3): http://stackoverflow.com/questions/20959298/enums-documentation-with-less-mixins/21052115

Example where this can be needed is when there is a simple case for which you can to assert.
For example:

.complexMixin(@list,@list2) when ( sin(length(@list)) < cos(length(@list2)) ) {
  // not fail
}
.complexMixin(@list,@list2) when ( sin(length(@list)) > cos(length(@list2)) ) {
  // not fail
}
.complexMixin(@list,@list2) {
  fail("the cos and sin can't be equal!");
}

I mean I want to be able to express switch / pattern matching expressions where the default unhandled case should never happen (pattern often used to fail fast):

switch (value) {
  case "x" ...
  case "y": ...
  default: throw new Error("fail!");
}

@slorber
Copy link
Author

slorber commented Dec 4, 2015

So I tried to make a plugin but not really successfully :'( not sure to understand how to make it work and even how to make unit tests. It seems the test does not find my new function:

https://github.com/slorber/less-plugin-assert/

@seven-phases-max
Copy link
Member

Well, your mistake there is that you assume that Less true is equal to JS true - it's not actually.
All Less values (colors, numbers, strings of various types, and logical values in the end) are represented with dedicated JS objects (Color, Dimension, etc. you'll find in less.tree). Thus in your assert impl. you would need to compare the condition value to less.tree.True constant rather than using !condition).


For your particular example, however, it's easier to write just a error (or debug-break or fail or whatever you name it function) to throw a error unconditionally since you're going to use it with when statement anyway. (assert function is supposed to evaluate the condition on its own but since logical statements are only possible inside when statements and cannot be passed to functions, things like assert(length(@list) < 1, "doh!") are not quite possible currently).
In other words, in your plugin just write a function like:

error: function(message) {
    throw {
        type: "Whatever",
        message: message.value // note the message arg is of type less.tree.Quoted (not JavaScript String)
    };
}

and then use it like:

.complexMixin(@list, @list2) when (sin(length(@list)) < cos(length(@list2))) {
    // ok
}
.complexMixin(@list, @list2) when (sin(length(@list)) > cos(length(@list2))) {
    // ok
}
.complexMixin(@list, @list2) when (default()) {
    unused: error("the cos and sin can't be equal!");
}

@slorber
Copy link
Author

slorber commented Dec 4, 2015

ok thanks for the explanation :)

that seems not very better than the "mixin does not exist" than ScottS proposed on stackoverflow unfurtunatly

@seven-phases-max
Copy link
Member

that seems not very better than the "mixin does not exist"`

That was your code not mine :)

For your initial example me would start with something like:

.background-clip(@value) when 
    (at(l(border-box, padding-box, content-box, inherit), @value) = ~'') {
        // ...
}

then it's not a problem to derive a function to remove redudant l(), = ~'' and/or generate a error in place.
I.e. it's just a matter of deciding what the function of interest should actually do, i.e. is it is-value-in-list returning true or false, or ensure-value-in-list generating a error if not, index-of so one could compare it with > 1 etc. and so on.

@seven-phases-max
Copy link
Member

Btw., thinking of the initial example yet more I recall one more method (w/o bothering with custom functions) to generate a bit more readable error message with a bit less verbose code (if compared to the method suggested in the SO Q above): [1].

@rjgotten
Copy link
Contributor

one more method

Using the pattern matching mechanic on an inner mixin; clever!

@rjgotten
Copy link
Contributor

rjgotten commented Mar 7, 2016

@seven-phases-max
I've been thinking about mixin assertions some more. Maybe the language should offer a parametrized version of the default() guard, that can be used to tell the compiler to throw when it is activated, rather than evaluate the mixin body.

E.g.

.complexMixin(@list, @list2) when (sin(length(@list)) < cos(length(@list2))) {
  // ok
}
.complexMixin(@list, @list2) when (sin(length(@list)) > cos(length(@list2))) {
  // ok
}
.complexMixin(@list, @list2) when (default("the cos and sin can't be equal!")) {
  // will never be hit, because the guard will throw with the specified error.
}

If you attach this logic to the default() guard, you have the advantage of having access to the internals for the mixin name, the parameter values with which the mixin was called and the callsite.

You can generate really nice error messages with line numbers set to the correct location that way.

@matthew-dean
Copy link
Member

@rjgotten Smart. I like it. It would allow library authors to enforce type safety and numerical ranges for mixins.

That said... The error where it's being generated isn't really the intended error location. That is, if you want mixin A to fail based on some criteria, you aren't defining it at mixin A. You're defining it at mixin B, which only happens to be encountered because the lack of match at mixin A. But mixin A and B have no real relationship other than they match a mixin call. (So they're both related to call X, but not directly to each other.)

So it seems to me what a developer REALLY wants is to have a failure at the location of a mixin, but that is illogical based on the concept of mixins. A non-match of a single mixin is not a "failure" at that point in the evaluation.

So it seems to me, echoing some of @seven-phases-max's statements, only functions serve this purpose. A function has a single entity (per scope) so they can establish a pass/fail on parameters. Mixins can't. They can only establish a pass, and not an exception.

Some plugin features in development (awaiting review) should make this a lot easier (via JS), and it's not out of the question that we could establish a native Less-based function syntax that allows these assertions down the road.

@rjgotten
Copy link
Contributor

rjgotten commented Mar 7, 2016

@matthew-dean
That said... The error where it's being generated isn't really the intended error location. That is, if you want mixin A to fail based on some criteria, you aren't defining it at mixin A. You're defining it at mixin B, which only happens to be encountered because the lack of match at mixin A.

Ideally, you'll want argument errors to pop at the call-site, i.e. , the location where the mixin was being called:

// This is where we accept (one possible set of) valid parameters
.foo(@num) when (isnumber(@num)) and (@num >= 0) {}

// This is where we set-up the error to throw, when no matched
// signature of the `.foo` mixin managed to pass the guards.
.foo(@num) when (default("Not a positive number")) {}

// But this mixin **call** is what will throw the actual error, complete
// with line number, filename, etc.
.foo(-10);

Is this also what you're trying to refer to?

This means you attach the throw logic to MixinCall nodes, which are exactly where the default() handling logic currently also resides...

There's a second reason you'd want the error to be thrown from the call node: orthogonality.
The call node is also the location from which the error is thrown when no matching mixin definition could be found, period.

@matthew-dean
Copy link
Member

Ideally, you'll want argument errors to pop at the call-site

Or both? In a call stack you'd have both. I wonder if it might be possible to generate just a light stack with the normal line / column of the error (from the call), and what mixin is returning that error?

But yes, there is logic to what you say, and you're right that that's where errors are typically generated, so walking stuff back, maybe your proposal works after all.

Btw, I assume you meant to write:

.foo() when (default("Not a positive number")) {}

?

@rjgotten
Copy link
Contributor

rjgotten commented Mar 8, 2016

@matthew-dean

No. I meant for the @num argument to be there.
Less already has a different built-in error that is thrown when you call a mixin with an incompatible number of arguments. ;-)

@matthew-dean
Copy link
Member

@rjgotten Oh right right that makes sense. Duh.

@seven-phases-max Your thoughts?

@seven-phases-max
Copy link
Member

default("Not a positive number")

I like the semantics, nice spot!. Though due to default implementation it may be quite tricky to code, counting the edge cases like not default("Boo!") // ?!.

@stale
Copy link

stale bot commented Nov 14, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Mar 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 16, 2018
@stale stale bot closed this as completed Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants
@matthew-dean @rjgotten @SomMeri @slorber @seven-phases-max and others