Bug 111675 - IFS() and SWITCH() should short-cut evaluation like IF() and CHOOSE()
Summary: IFS() and SWITCH() should short-cut evaluation like IF() and CHOOSE()
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
5.2 all versions
Hardware: All All
: medium normal
Assignee: Ahmed Hamed
URL:
Whiteboard: target:7.4.0 target:7.3.3
Keywords:
: 113595 137135 (view as bug list)
Depends on:
Blocks: Calc-Function
  Show dependency treegraph
 
Reported: 2017-08-10 23:57 UTC by Michael
Modified: 2024-03-18 14:54 UTC (History)
10 users (show)

See Also:
Crash report or crash signature:


Attachments
SampleSTYLE.ods, sample ODS explaining the IFS bug (9.14 KB, application/vnd.oasis.opendocument.spreadsheet)
2017-08-10 23:58 UTC, Michael
Details
Variant in position of CURRENT() in the IFS (10.99 KB, application/vnd.oasis.opendocument.spreadsheet)
2017-08-11 17:39 UTC, Regina Henschel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael 2017-08-10 23:57:01 UTC
Description:
Using Function IFS and CURRENT within Test expressions leads to not comprehensible results.

Target/background: A cell (referencing whatever, e.g. COUNTIF etc.) should have a STYLE() applied, depending on another cell's value, e.g. a treshold in a real life example

Works with nested if, does NOT WORK with IFS. Example uploaded.

Steps to Reproduce:
All in uploaded "SampleSTYLE.ods":

Both ref value B2 and Treshold value B3 are the same, e.g. 4


Actual Results:  
(B5) =B2+STYLE(IFS(CURRENT()<B3;"Grün";CURRENT()=B3;"Gelb";CURRENT()>B3;"Rot")) 
leads to 4 with "Rot" = RED, wrong as CURRENT()=B3 is TRUE!

(B6) =B2+STYLE(IF(CURRENT()<B3;"Grün";IF(CURRENT()=B3;"Gelb";IF(CURRENT()>B3;"Rot"))))
leads to 4 with "Gelb" = YELLOW, correct.

Expected Results:
IFS should be able to work in combination with CURRENT


Reproducible: Always

User Profile Reset: No

Additional Info:


User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Comment 1 Michael 2017-08-10 23:58:33 UTC
Created attachment 135441 [details]
SampleSTYLE.ods, sample ODS explaining the IFS bug
Comment 2 Michael 2017-08-10 23:59:26 UTC Comment hidden (no-value)
Comment 3 Michael 2017-08-11 00:02:39 UTC
Asked this question before opening this bug: https://ask.libreoffice.org/en/question/120323/calc-bug-inbecause-of-function-style-german-vorlage/
Comment 4 m_a_riosv 2017-08-11 07:46:32 UTC Comment hidden (noise)
Comment 5 Michael 2017-08-11 16:07:17 UTC
I've read the help and I'm not sure if I understand your remark. 

In my example ODS both B5 and B6 are getting the same value B2 + STYLE(...), so in fact B2. Only difference is, that the same conditions 

CURRENT()<B3,
CURRENT()=B3 and
CURRENT()>B3

are resolved differently if they're resolved for one IFS() call or nested IF() calls.

not sure if I've got your comment "What should be for you the value of CURRENT() in any place." - Isn't "any place" both IFS and nested IF?
Comment 6 Regina Henschel 2017-08-11 17:39:35 UTC
Created attachment 135474 [details]
Variant in position of CURRENT() in the IFS

I expect, that the result of CURRENT() is only changed, after the IFS was completely evaluated. It seems, that CURRENT() gets an internal values of the condition, that was not met.
Comment 7 TBeholder 2017-11-02 08:39:21 UTC Comment hidden (me-too)
Comment 8 Eike Rathke 2018-03-09 21:31:29 UTC Comment hidden (off-topic)
Comment 9 Michael 2018-03-10 01:54:18 UTC
(In reply to Eike Rathke from comment #8)
> IFS() evaluates conditions until it finds a matching one,
> sprinkling CURRENT() all over makes it depend on internal behaviour what
> arguments will be evaluated first, which currently may be all before IFS()
> is called and in future may be only the ones until one is matched.
> 

As I opened this bug, I would be thankful if this bug is not going to be closed because someone else's examples or additions might be misleading or independent of my bug. 

If you look on my example attached (https://bugs.documentfoundation.org/attachment.cgi?id=135441), the IFS formula in B5 is: 

=B2+STYLE(IFS(CURRENT()<B3;"Grün";CURRENT()=B3;"Gelb";CURRENT()>B3;"Rot"))

And my question is: For B3 constantly being e.g. "4", how could a value of "4" for B2 lead to the condition "CURRENT()>B3" being selected by IFS() ??? 

--> As ONLY the condition "CURRENT()=B3" is applicable, IMHO it does not have any impact if IFS() evaluates sub-formulas from left to right, or vice versa.

In the attached example LO shows a value of "4" for B5, so for whatever reason CURRENT might return any other value than "4", and as B3 is also "4", how could ever the condition "CURRENT()>B3" evaluated as true ??
Comment 10 Eike Rathke 2018-03-12 11:15:25 UTC
I'm repeating myself:
"IFS() evaluates conditions until it finds a matching one, sprinkling CURRENT() all over makes it depend on internal behaviour what arguments will be evaluated first, which currently may be all before IFS() is called and in future may be only the ones until one is matched."
and
"Using CURRENT() multiple times at the same function level within one expression means relying on internal implementation details. One should not. It may change without notice."

> =B2+STYLE(IFS(CURRENT()<B3;"Grün";CURRENT()=B3;"Gelb";CURRENT()>B3;"Rot"))
> 
> And my question is: For B3 constantly being e.g. "4", how could a value of
> "4" for B2 lead to the condition "CURRENT()>B3" being selected by IFS() ??? 
Because here the state of CURRENT is not what you think it is. For the first CURRENT()<B3 it is B2 that was previously pushed to the stack, for CURRENT()=B3 it is "Grün" that was just pushed on the stack, for CURRENT()>B3 it is "Gelb" that was just pushed on the stack, and a string is greater than a numeric value.

Anyway, I'll keep this and close bug 113595 instead, they have the same reason.

IFS() and SWITCH() currently evaluate all parameters' arguments in order encountered from left to right for IFS() and right to left for SWITCH() and return the result matching for the condition(s). They do not short-cut evaluation like IF() and CHOOSE() do. Hence for a function that has side effects like STYLE() all styles are applied and the last one evaluated wins.
Comment 11 Eike Rathke 2018-03-12 11:15:46 UTC
*** Bug 113595 has been marked as a duplicate of this bug. ***
Comment 12 QA Administrators 2019-03-13 03:46:19 UTC Comment hidden (obsolete)
Comment 13 So 2019-12-23 02:17:31 UTC
Still in 6.4.0 RC1. Both
=IFS(0,1/0,1,TRUE())
=SWITCH(1,0,1/0,1,TRUE())
return #DIV/0!
Comment 14 b. 2019-12-28 01:29:20 UTC
@So: u're right, bug, evaluating things that shouldn't ... 
while that's wrong OP question not solveable ...
Comment 15 Eike Rathke 2020-09-30 10:13:50 UTC
*** Bug 137135 has been marked as a duplicate of this bug. ***
Comment 16 Vincent 2020-10-02 22:13:36 UTC
Hi,

First try to solve a bug. I'm trying to find where those functions are defined in the source code. Somebody already look for it ?
Thanks
Comment 17 Vincent 2020-10-02 22:38:49 UTC Comment hidden (off-topic)
Comment 18 Andreas Heinisch 2021-04-15 19:27:47 UTC
The problem here ist that the calculations within the IFS function (for instance the DIV) are evaluated before the actual IFS function itself. Hence, any further calculation leads to a DIV to ZERO error, because if there is an error, no further calculations are made.

So this example leads to this error:
=IFS(0,1/0,1,TRUE())

This example works:
=IFS(0,1/0,1,"test")

So I think the only possibility to get this to work, is to change the generated compilation steps for this function.
Comment 19 Mike Kaganski 2022-03-10 09:02:43 UTC
(In reply to Andreas Heinisch from comment #18)
> The problem here ist that the calculations within the IFS function (for
> instance the DIV) are evaluated before the actual IFS function itself.
> Hence, any further calculation leads to a DIV to ZERO error, because if
> there is an error, no further calculations are made.

I do not see this to be *directly* related to the original problem:

(In reply to Michael from comment #9)
> If you look on my example attached
> (https://bugs.documentfoundation.org/attachment.cgi?id=135441), the IFS
> formula in B5 is: 
> 
> =B2+STYLE(IFS(CURRENT()<B3;"Grün";CURRENT()=B3;"Gelb";CURRENT()>B3;"Rot"))

Additionally, I do not see how could "IFS() and SWITCH() should short-cut evaluation like IF() and CHOOSE()" title (change by Eike from 2018-03-12) be directly related here for the initially stated problem ("UI: Functions IFS does not work together with function CURRENT").

While it might be reasonable to add short-circuiting to the IFS and SWITCH, I do not see how that could avoid evaluating the "unneeded" branches in the presence of CURRENT (or, more specifically, how could it avoid changing CURRENT value "unexpectedly"), which necessarily evaluates what was put to stack last before this invocation of CURRENT, and thus must take either the next or the previous *Value N* argument value - whichever direction the evaluation uses, but definitely not what user would naively expect (that it would use something from the distant past in the formula), independent of the short-circuiting that CHOOSE might do.
Comment 20 Mike Kaganski 2022-03-10 09:06:00 UTC
By the way, instead of trying to make CURRENT work with other functions in a way one or another user might expect in a specific situation, IMO a better would be to implement LET function (bug 137543), which would allow to define a variable to a specific value, and reuse it as many times in different places in that formula as required.
Comment 21 Eike Rathke 2022-03-11 13:46:04 UTC
The problem with CURRENT() within IFS() or SWITCH() is that *all* arguments are evaluated before calling the function, it doesn't behave as someone might expect (i.e. executed only if the corresponding jump condition evaluates to TRUE as this is not implemented as jump code branches). Apart from that many assumptions what CURRENT() would do are simply wrong..

If implementation was done with short-cut evaluation jump code branches then an expression like
=23+IFS(FALSE();CURRENT()*2;FALSE();CURRENT()*3;TRUE();CURRENT()*4)
would yield 115 same as
=23+IF(FALSE();CURRENT()*2;IF(FALSE();CURRENT()*3;CURRENT()*4))
does, but currently it yields 27 because for the last condition the CURRENT() has the value 1 of the preceding parameter's TRUE() argument.

Doing this with jump code branches would eliminate unnecessary calculations, with the side effect that using STYLE() in conditional branches would actually apply the desired style and not the last encountered during evaluation of arguments. Apart from that using STYLE() is bad style and conditional formatting should be used instead..
Comment 22 Eike Rathke 2022-03-11 13:54:38 UTC
Best forget about CURRENT() and STYLE() in all this context, they're just symptoms. Better suited examples are of comment 13

=IFS(0;1/0;1;TRUE())
=SWITCH(1;0;1/0;1;TRUE())

return #DIV/0!
Comment 23 Commit Notification 2022-03-12 10:34:45 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/84720c09ef30e79c56936026c3992240b4ae010b

Related: tdf#111675 Replace Pop() with PopError() where relevant

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Commit Notification 2022-03-12 11:19:06 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/efb713febeaaf3e4b17886d405d46845d4a0e676

Related: tdf#111675 Clear global error for arguments if possible

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 25 Eike Rathke 2022-03-12 11:29:23 UTC
This fixes only the error inheritance of subsequent evaluated arguments as mentioned in comment 22.
Pending review for 7-3
https://gerrit.libreoffice.org/c/core/+/131362
https://gerrit.libreoffice.org/c/core/+/131363
Comment 26 Commit Notification 2022-03-14 13:20:55 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/69bf8f5e6dc43adde087403531641f2bd064a22c

Related: tdf#111675 Replace Pop() with PopError() where relevant

It will be available in 7.3.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 27 Commit Notification 2022-03-14 13:22:07 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/91ca6a8eded939373e78ecf6091b3598936f2c08

Related: tdf#111675 Clear global error for arguments if possible

It will be available in 7.3.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 28 Commit Notification 2022-03-14 14:26:22 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/9d5ec2c9d0bf8765fc75c17a0e6fdcb2583335d0

tdf#111675: sc_logical_functions: Add unittest

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 29 QA Administrators 2024-03-14 03:16:09 UTC Comment hidden (obsolete)