[BUG] Division by 0 in the false branch of numerical conditionals

[Yincognito]

I vaguely recall talking about this or something similar a long time ago (and certainly avoid such cases myself), but recently it has been re-discovered by someone else...

Test Skin (click to toggle between A=2 and A=0):

Code: Select all

[Variables]
A=2

[Rainmeter]
Update=1000
AccurateText=1
DynamicWindowSize=1
BackgroundMode=2
SolidColor=47,47,47,255

---Measures---

[B]
Measure=Calc
Formula=((#A#=0)?(0):(1/#A#))
DynamicVariables=1

---Meters---

[Result]
Meter=String
FontColor=255,255,255,255
FontFace=Consolas
FontSize=16
Padding=5,5,5,5
AntiAlias=1
Percentual=1
MeasureName=B
Text=B = %1%
LeftMouseUpAction=[!SetVariable A (2-#A#)][!UpdateMeasure *][!UpdateMeter *][!Redraw]
DynamicVariables=1

Result:
The meter never gets the 0% from the measure when A=0 per the conditional, because of the division by 0 error in the false branch, so it continues to display 50%.

Expected:
The conditional should properly avoid the division by 0 when A=0, by not evaluating the invalid 1/A altogether and returning 0%. According to this, the ternary operator seems to perform a short circuit operation in C++ and skip the non-true case in the conditional...

Notes:
- the result is converted to a percentage via the Percentual option, so 0 will be 0%, 0.5 will be 50%, and 1 will be 100%
- the division by 0 error can be avoided by making something like (1/((#A#=0)?(999):(#A#))) in the Formula, but in practice this is longer and more complex and the result won't exactly be 0

[SilverAzide]

I was under the impression that Rainmeter's math parser doesn't short circuit, i.e., the whole expression gets evaluated.

[Yincognito]

I was under the impression that Rainmeter's math parser doesn't short circuit, i.e., the whole expression gets evaluated.

That is true. Rainmeter, however, is developed in C++, so I reckon that in a way, what is being parsed is somehow being "translated" into C++ instructions, hence my take on this. For example, I don't imagine Rainmeter reinvented the wheel and has all those math functions being dealt with all by itself without using the built-in C++ equivalent functions (if any). Unfortunately my C++ skills are not perfect so I can't figure it all out just by examining Rainmeter's MathParser.cpp as I'd wished.

That being said, even if Rainmeter handles the conditional parts by itself, I suppose it wouldn't be a problem to just ignore the false branch in the conditional, since it technically has no relevance to the actions and operations done internally by Rainmeter in order to get a numerical result from the conditional. I mean, even without short circuiting (if by any chance I was wrong above and there are other details that makes it unusable), isn't manual error handling one of the main purposes of an if...else...elseif statement or conditional equivalent? You know, to avoid executing a potentially problematic branch? Didn't test it, but I bet IfConditions don't throw this kind of errors for perfectly valid tests and true branches. I'd be happy even with an order based solution to this bug, where the 2nd branch of the conditional is ignored if the 1st one is true, by the way, but it has to be dealt with, IMHO, since it has ramifications in every Rainmeter numerical department.

[SilverAzide]

That is true. Rainmeter, however, is developed in C++, so I reckon that in a way, what is being parsed is somehow being "translated" into C++ instructions, hence my take on this.

I don't think this is a valid comparison. C/C++ is a compiled language, ultimately in this case compiling to machine code. Rainmeter is an interpreted language, which is a completely different thing. The C compiler takes those ternary expressions (or whatever) and builds an execution tree (like a series of "if" statements) at compile-time, so it knows what paths it can take (or not) at execution time and can short-circuit them. With an interpreted language, you can't know this until you've parsed and resolved everything, especially since in Rainmeter "variables" are nothing like a real variable in the C/C++ sense.

[Yincognito]

I don't think this is a valid comparison. C/C++ is a compiled language, ultimately in this case compiling to machine code. Rainmeter is an interpreted language, which is a completely different thing. The C compiler takes those ternary expressions (or whatever) and builds an execution tree (like a series of "if" statements) at compile-time, so it knows what paths it can take (or not) at execution time and can short-circuit them. With an interpreted language, you can't know this until you've parsed and resolved everything, especially since in Rainmeter "variables" are nothing like a real variable in the C/C++ sense.

You make a good point and you're probably right about this. This doesn't impede a resolution to this though, if anything it helps it, since, as you said, Rainmeter has to parse and resolve everything before it knows those paths. Certainly since the conditional test (i.e. the "if") would be parsed and resolved first would help in avoiding the error in the false branch (i.e. the "else" part), don't you think? I mean, it's just a boolean result and the branches wouldn't even need parsing to discount one of them since they're literally separated by the ":" symbol. Surely even C++ has some kind of Javascript "try {} catch {}" equivalent system to make sure evaluating a division by 0 doesn't lead to an execution issue. It seems illogical to me to invalidate a whole conditional just because the branch you specifically, as a skin designer (or even a plain developer), want to avoid, is mathematically (not syntactically) invalid. That's the whole point of the conditional in this case (and probably, many others).

EDIT: To further clarify this, I have no problem with the error itself - it's the choice of the Rainmeter developers if they want to spam such errors in the log every time such a situation is encountered. My problem is with canceling the entire execution of the conditional and its perfectly valid branch that will (and should) actually be executed. This is why I didn't question whether this is a bug or not: it has a key negative effect on the functionality of the skin using such constructs, let alone increasing the length of the conditional for every such possible occurrence if the workaround above is applied.

EDIT2: By the way, I rephrased a bit the title of this thread to be more descriptive.

[balala]

- the division by 0 error can be avoided by making something like (1/((#A#=0)?(999):(#A#))) in the Formula, but in practice this is longer and more complex and the result won't exactly be 0

Or by Formula=((#A#=0)?(0):(1/(#A#+0.00000001))) (or an even smaller value added to #A# in the denominator).

[SilverAzide]

Fair enough... but I still would not classify this as a bug. It's really more of a feature request (as in, an enhancement of the parser to support short-circuit evaluation).

[Yincognito]

Or by Formula=((#A#=0)?(0):(1/(#A#+0.00000001))) (or an even smaller value added to #A# in the denominator).

Sure thing. I skipped mentioning this earlier, but now that you reminded me of it, I think it's better than my own and will use it - thanks. For the record, adding 5 zeros after the decimal point for this "epsilon"-like value is enough since Rainmeter's numerical values only support 5 decimal points precision, what's beyond that is discarded (you can make a Calc measure and set it to 0.000001 and then 0.00001 and you'll see what I mean).

Fair enough... but I still would not classify this as a bug. It's really more of a feature request (as in, an enhancement of the parser to support short-circuit evaluation).

I understand why you might think that, and it's partly my fault. Let's put this another way and forget about the short circuit, I don't really care about that, it was only used as an example for correct behaviors. What I'm looking for is to make the conditional and skin functional in this case, cause they're not (at least not without applying a workaround):
- the otherwise valid #A#=0 branch of the conditional is never executed, like at all, so the entire conditional itself is non functional here and exits with an error
- as a result, the skin will never make the result 0%, despite being instructed to do so for these edge cases, thus non functional results in practice
I'm only looking to make them both functional again, that's all. With or without errors, with or without the short circuit, doesn't really matter.

---

Anyway, for now, I'll use the workaround recommended by balala, but I still think this should be fixed. Just my two cents, the rest is up to the devs to decide.

See how KFC's fried chicken inspire others to be hungry, but for solutions? And yes, you made me really hungry for fried chicken with that remark...

[balala]

I skipped mentioning this earlier, but now that you reminded me of it, I think it's better than my own and will use it - thanks.

You1re welcome!

For the record, adding 5 zeros after the decimal point for this "epsilon"-like value is enough since Rainmeter's numerical values only support 5 decimal points precision, what's beyond that is discarded (you can make a Calc measure and set it to 0.000001 and then 0.00001 and you'll see what I mean).

Right, doesn't really matter. When I posted my reply, I wrote the value just like that, didn't think about what exactly to write. Just took care not to have a too large value.

[Brian]

I don't think this is a valid comparison. C/C++ is a compiled language, ultimately in this case compiling to machine code. Rainmeter is an interpreted language, which is a completely different thing. The C compiler takes those ternary expressions (or whatever) and builds an execution tree (like a series of "if" statements) at compile-time, so it knows what paths it can take (or not) at execution time and can short-circuit them. With an interpreted language, you can't know this until you've parsed and resolved everything, especially since in Rainmeter "variables" are nothing like a real variable in the C/C++ sense.

This is correct. If we could just pass things directly to C++, the ternary operator would work the same way. Rainmeters MathParser works by evaluating the left and right tokens of an operation...in order of precedence. It just so happens that the ternary operator considers the left and right tokens to be "parameters" of the operation, so it evaluates both of the tokens before the operation.

I might be able to fix this, however, the math parser was written by a 3rd party many years ago, and it is quite fragile (not to mention complex).

-Brian

Bonus points! I just found a new operator, although I am not sure of its usefulness. It's the $ operator. If the left token is less than or equal to 0, it returns 0. If the right token is 0, it prints an error (Division by 0). Otherwise it returns ceil(left / right). I have no idea why this is there, or what to even call it.
https://github.com/rainmeter/rainmeter/blob/master/Common/MathParser.cpp#L564-L577