[BR] var::equals string/double bug

General JUCE discussion
21

I agree that this change is somewhat intrusive. But not as bad as it might seem at first sight. I also understand that for someone who’s used to strong typing (like me) it feels unsettling to implicitly convert types before comparing them. PHPs type juggling for example is the cause for many security related flaws. Tripple equals to the rescue !

But here we’re only talking about the case in which one of the operands is a string and the other one is some sort of number. So:

"1." != "1.0"
"1." == 1
1 == "1."

it’s not the plan to have every type willy-nilly convertible into each other type.

And if you’re going to do type conversion - as var is doing already - it’s probably a good idea to do it in a well defined manner so that the operation is commutative instead of converting the type of the right operand into the type of the left operand.

IMHO the current implementation of var::equals() is wrong and I’m pretty sure that who ever wrote it would agree after being aware of the issue. The documentation of Variant states “The var class is intended to act like the kind of values used in dynamic scripting languages.” - and in this regard it currently doesn’t.

EDIT: as a matter of caution I also don’t think it’s a good idea to change it per default - it should be opt-in via some sort of flag.

22

The reason why I see it differently is, in C++ and in the juce::var we know the type of the data. So we know what method for comparison is adequate. So why throwing that information away?

If I know there is a string inside my var, I want to know if they have the same characters.
And if I know there is a number in my var, I want to know if they represent the same number.
Imagine you could put a fraction into a var, I would indeed want to see { num=1; den=4 } == 0.25 as true (numerical limitations apply ofc).

As far as I know JS you cannot distinguish between 1.0 and “1.0”. Is that correct?

Anyway, I think all is said from my side, the decision is with the juce team.

23

I haven’t discussed this with the rest of the team, but as I see it, making this change to var has the potential to subtly break a swathe of other code, so I’d be reluctant to change the implementation of operator== directly.

Putting the change behind a compile-time flag is probably even more difficult to get right, as we’d now have to test both configurations and keep them both working as expected.

A safer option would be to add a new equalsWithJSSemantics function, allowing new code to opt in to the new equality semantics. However, having two implementations of equals is still quite confusing.

The situation isn’t ideal, but I don’t see any really good alternatives at the moment.

24

isn’t there already an equalsWithSameType?

so, there’s already two different equals functions…

25

“<”, “<=”, “>” and “<=” are already implemented in a commutative manner. I wonder why “==” isn’t also using compare(). So currently:

var a{1.};
var b{"1."};
jassert(!(a < b)); // pass
jassert(!(a > b)); // pass
jassert(a == b); // pass
jassert(b == a); // fail

I somehow doubt that this is intentional.

26

Good point, the JS version would be the third kind of equality check. Adding a function is the least-bad of the solutions I can think of at the moment, so that’s probably the solution we’d go with if we were to do this.

At the same time, there are other higher-priority issues that need our attention at the moment, so I don’t see much changing here in the short term.

The compare algorithm is quite basic (it does string comparison if both sides are strings, otherwise it falls back to comparing both sides as doubles). Implementing == in terms of compare would also have some surprising effects:

var a ("abc");
var b (0.0);

expect (! (a < b)); // pass
expect (! (b < a)); // pass
// These both pass, implying a == b

It’s difficult to say. In any case, given that there’s code that depends on the current behaviour, we can’t change it without a very strong rationale.

2 Likes