Make Scratch 3 comparisons with +/-Infinity match Scratch 2

[spectranaut]

Resolves

Resolves #1699

Proposed Changes

This PR makes changes to scratch-vm/src/util/cast.js's Cast.compare:
* Cast.compare relies on the javascript subtraction operator (if v1 - v2 === 0, then we they are equal) to compare numbers. This will not work with Infinty because Infinity - Infinity = NaN, so there is extra logic to handle this case.

Reason for Changes

Scratch 3 does not treat infinity the same as Scratch 2.

Test Coverage

In this PR I added two additional unit test files. You can also see the original Scratch2 test of comparisons here: https://scratch.mit.edu/projects/260178207/

[joker314]

Nice! My changes in #1701 are almost the same, except you treat lowercase infinity as a number (this isn't needed because string comparison works correctly), this is probably a good idea.

My PR was closed because it was alleged that the issue may have been wider than this Infinity minus Infinity issue and so should be kept internal. That perspective (which I think might be mistaken) would probably apply to this PR too

[joker314]

Stylistic suggestions

[joker314]

I just wanted to note that this PR introduces a breaking change.

Some projects might have been detecting [upper|lower]case like this:

That is, relying on the fact that "infinity" and "Infinity" are actually parsed differently, and that while this is hidden during comparisons, subtracting 0 shows this off.

That's not to say take it out! -- it depends on how frequently this is used in the wild, so maybe we should wait for ST opinion on whether this breaking change should be included.

[spectranaut]

@joker314 is right!

In scratch 2:

• if a variable X is set to the string "infinity", then X - 0 = 0.
• if a variable X is set to the string "Infinity" (with a capital 'i'), then X - 0 = Infinity.
• if a variable X is set to the operator block (1) / (0), then X - 0 = Infinity

In scratch 3, the changes I introduced will make all three of these equal Infinity. There are tests for this case in the unit tests. Whoever reviews this request should look at all the unit tests to make sure they are the way Scratch 3 should behave.

The question here is: should scratch 3 mimic this different treatment in of "infinity"? In the minus block, "infinity" is treated like a string. In the equals block, "infinity" is treated like a number.

[kchadha]

Initial review: two minor comments to start addressing,

1. From a quick search through our code base, it looks like there is one place (previously added by me, oops!) where we use Cast.toNumber(value) where we probably mean Number(value). This is in src/engine/comment.js, do you mind changing the calls there in this PR?

2. Not strictly necessary, since you wrote a lot of tests for this PR (thank you!), but a nice to have, we might want to add some execute integration tests like the ones found in test/fixtures/execute/ so that we can ensure that 3.0 behaves the same way as 2.0

[spectranaut]

hi @kchadha, I addressed the Caste.toNumber in the last commit! Thanks for catching that.

I think you are right, there should be an execute integration tests. I'll try to write one next week! Are these tests always run on both scratch 2 and scratch 3?

When you get back to reviewing, please look closely at all the unit tests and make sure this is the way we want scratch 3 to behave. There are a few points of divergence from Scratch 2. When writing the tests, I didn't realize that the string "infinity" is treated differently in a comparison block than a math block (like minus or division). In Scratch 3 -- should we copy the scratch 2 behavior, or make the treatment of the string "infinity" the same in comparison and math blocks?

The unit tests are written assuming that the string "infinity" is treated the same across all blocks.

[fsih]

The question here is: should scratch 3 mimic this different treatment in of "infinity"? In the minus block, "infinity" is treated like a string. In the equals block, "infinity" is treated like a number.

@spectranaut: @kchadha and I were talking about this and feel like the use of "Infinity" in comparison and math blocks is probably an edge case that only very high-level Scratchers use, not to mention it only takes the English word, and we should just keep its behavior the same in scratch 3 as it was in scratch 2 (we want to avoid changing things unless they are a big help to understanding). It looks like the behavior in Scratch 2 is that all the math operation blocks only recognize capitalized Infinity, and the equals block is casting both sides to string and doing a case insensitive comparison. I'll try to dig around more in the flash code to find what it is exactly.

[fsih]

https://github.com/LLK/scratch-flash/blob/646523e6846ad0dd993213a38b46fe2ea511d026/src/primitives/Primitives.as#L48

Here is where we define the function of the blocks. So +, -, *, / see their arguments as numargs, and <, > = see their arguments as args

https://github.com/LLK/scratch-flash/blob/72e4729b8189d11bbe51b6d94144b0a3c392ac9a/src/interpreter/Interpreter.as#L390

Here is where numarg and arg are defined. numarg is just casting to AS3 Number
(Tangentially, we discovered that Number("2e") in js is NaN, while Number("2e") in AS3 is 2, but I feel like that's something we're willing to accept changing)

https://github.com/LLK/scratch-flash/blob/646523e6846ad0dd993213a38b46fe2ea511d026/src/primitives/Primitives.as#L151

Here is where compare is defined for >, <, =. It looks like it's trying to compare as numbers and, if that fails, compares the two values as strings.

[joker314]

Note that <, >, = all try to cast their numbers using Interpreter.asNumber (defined in the same file as arg and numarg are)

This has a check for at least one digit, using a loop. We can probably use a regular expression instead: it's looking for a digit character, so that a string of just whitespace will not be cast to 0, but instead result in NaN,

However, this also (accidentally?) excludes Infinity and -Infinity, since neither contain digits. Hence, the comparison blocks do not consider those numbers.

In Scratch 3.0, we have a utility isWhiteSpace in src/util/cast.js. I git grepped for it, and I think it was created just for this purpose, and isn't used elsewhere. It's also used when setting the costumes and backdrops, because in Scratch 2.0 Interpreter.asNumber was used there too (numbers would be treated as costume numbers).

Should we change the utility to be something which casts to a number (like asNumber), maybe strictToNumber or fromNumericSquareSlot (since, as far as I can tell, this is used when a square slot is designed to have special functionality for a number)? I prefer the former though.

[fsih]

However, this also (accidentally?) excludes Infinity and -Infinity, since neither contain digits. Hence, the comparison blocks do not consider those numbers.

In the same line of reasoning as before, I think we should keep the behavior the same as it was in Scratch 2. So Infinity will equal infinity in the equals block.

[spectranaut]

ok @fsih and @kchadha , thanks for the detailed feedback and I've implemented what you asked!

[joker314]

This looks good. Just a slight compatibility issue, not in the comparison block, but caused by the same root problem:

Take a look at this incompatibility (in src/blocks/scratch3_looks.js):

1. Load the Scratch Cat sprite, which has two costumes
2. Switch to the second costume (e.g. manually)
3. Drag out a "switch to costume" block
4. Place a "join Infin ity" block as the argument to the "switch costume" block
5. Click on the block

In Scratch 2.0, Infinity is not considered a number (since it contains no digits), and is therefore considered a non-existent costume--remaining at costume 2. However, the current Scratch 3.0 implementation parses this as a number, which causes it to go to the first costume (1).

I think maybe creating a method to match 2.0's Interpreter.asNumber makes sense, named something like Cast.strictToNumber. In future, there may be extensions which use square inputs but expect numbers to be treated differently, and such a utility function would be useful to them.

[fsih]

@joker314 let's make a separate bug for instances like that where Infinity is getting interpreted incorrectly as a string/number

[fsih]

Looks great! Just the one lint fix is needed to make Travis pass.

[spectranaut]

Sorry I didn't catch that, fixed @fsih ! :)

[fsih]

LGTM, thank you!