Skip to content
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

if function shouldn't evaluate its not returned arg #3371

Closed
Horcrux7 opened this issue Mar 8, 2019 · 13 comments
Closed

if function shouldn't evaluate its not returned arg #3371

Horcrux7 opened this issue Mar 8, 2019 · 13 comments

Comments

@Horcrux7
Copy link

Horcrux7 commented Mar 8, 2019

The sample:

@some: foo;

div {
    margin: if((2 > 1), 0, 3px);
    color:  if((iscolor(@some)), darken(@some, 10%), black);
}

produce with version 3.9.0 the follow error:

RuntimeError: error evaluating function `darken`: color.toHSL is not a function in sample.less on line 5, column 34:
4     margin: if((2 > 1), 0, 3px);
5     color:  if((iscolor(@some)), darken(@some, 10%), black);
6 }

it look like that the THEN part is evaluate if the ELSE part should be used.

@seven-phases-max
Copy link
Member

Did it work before? (Now when you mentioned this I have some doubts... Even if it was me putting that example there. The problem is most likely not if choosing the wrong value, but the arguments themselves evaluated before entering the func).

@Horcrux7
Copy link
Author

Horcrux7 commented Mar 8, 2019

I does not know if this has work before. I expect that the person which have write the documentation has it test. In general it is easier to run the code and copy the result as to write the expected result manually.

But if you think this can not work and it is a problem in the documentation then forward this to the documentation team.

I have use this sample only as test code for JLessC which can evaluate it without an error.

@seven-phases-max
Copy link
Member

I have use this sample only as test code for JLessC which can evaluate it without an error.

Just out of curiosity, what is JLessC result for:

x {
    a:  if(true, darken(red, 10%), 123);
    @x: red;
    b:  if(true, darken(@x, 10%), 123);
    @y: darken(@x, 10%);
    c:  if(true, @y, 123);
}

?

@Horcrux7
Copy link
Author

Horcrux7 commented Mar 8, 2019

x {
  a: #cc0000;
  b: #cc0000;
  c: #cc0000;
}

Do you see a problem with it?

@seven-phases-max
Copy link
Member

Do you see a problem with it?

No, that's what I would expect (I was just wondering how/if it also evaluates args before the function itself).


In summary (as far as I understand it) I'm afraid it was just initially not-working example (my bad).
Which in turn means the if implementation should be improved (the way it is now, the false arg is always evaluated - and this is not what we'd expect (ideally)).

@seven-phases-max seven-phases-max changed the title if function sample does not work if function shouldn't evaluate its not returned arg Mar 10, 2019
seven-phases-max added a commit to less/less-docs that referenced this issue Mar 10, 2019
@matthew-dean
Copy link
Member

What's the issue here? The sample should fail because foo is not a color. What am I missing?

@matthew-dean
Copy link
Member

Regarding the arguments being evaluated regardless of being "needed", that's essentially how all Less functions work AFAIK. Less doesn't know what the function will do with the data. if() is (more or less) just a generic function, and arguments are always evaluated before the function is evaluated. Doing otherwise would be counter-intuitive, because the first argument has to be evaluated. There's no special case where any given function evaluates a portion of arguments but not all arguments.

So, to me, this is 100% expected.

@seven-phases-max
Copy link
Member

So, to me, this is 100% expected.

Yes, it is expected by the way functions are implemented. But it spoils the very idea of the function if it's not following the mainstream languages logic of not evaluating the false path.
Counting we've had to hack it to parse stuff not like other functions do, it would not be too greedy to expect it to be hacked to not evaluate the never used argument value.

@seven-phases-max
Copy link
Member

From a consistency point of view this can be reworded as:

If evaluates all of its arguments but ignores any errors triggered at the 'false' path.

(basically the same thing).

@matthew-dean
Copy link
Member

If evaluates all of its arguments but ignores any errors triggered at the 'false' path.

Hmm....yes that's maybe possible, but I'm not sure how this could be done in a generic way.... unless.... functions had an API / flag for discarding errors in eval'd arguments? Maybe functions would receive an error array (mapping to argument position) that they could decide to discard?

Although.... honestly.... isn't it just simpler to not pass un-evaluatable expressions to functions in the first place? Seems like a lot of feature re-work for something that could be solved via refactoring.

@seven-phases-max
Copy link
Member

isn't it just simpler to not pass un-evaluatable expressions to functions in the first place?

Well, yes. But an "un-evaluatable expression" might be the very reason the if function is wanted to be used at all :·)
Sure, often it's not a problem to reorder the whole expression like the original example
to something like:

color: darken(if(iscolor(@some), @some, lighten(gray, 10%)), 10%);

But the "naturality" is lost.


Either I can't see anything wrong in... hmm... to wait and see how soon the issue raises on its own.
(If it will at all, after all this one seems to encouraged only by the wrong docs example, and this is quite curious. Looking at the SO questions it was interesting for me to note how slow the if knowledge spreads if compared to each for example... :·).

@stale
Copy link

stale bot commented Jul 11, 2019

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.

@matthew-dean
Copy link
Member

FYI - I've done some initial work in 4.x to support this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants