toLeopard: Store # of times to repeat statically when it's a block

[towerofnix]

Resolves #119. See the issue for some related (philosophical) discussion.

For control_repeat blocks whose number of times to repeat is a reporter, this stores that expression in a times variable to be evaluated only once, alongside i = 0; like in Scratch, that value remains static over the course of the for loop's evaluation. This improves compatibility with a relatively minimal impact on (output) code legibility.

// Before

for (let i = 0; i < 10; i++) {
  this.x += 10;
  yield;
}
for (let i = 0; i < this.y + 10; i++) {
  this.y += 10;
  yield;
}
this.vars.index = 1;
for (let i = 0; i < this.vars.list.length; i++) {
  yield* this.doSomething(this.itemOf(this.vars.list, this.vars.index - 1));
  this.vars.index++;
  yield;
}

// After

for (let i = 0; i < 10; i++) {  /* Non-block inputs are treated same as before */
  this.x += 10;
  yield;
}
for (let i = 0, times = this.y + 10; i < times; i++) {
  this.y += 10;
  yield;
}
this.vars.index = 1;
for (let i = 0, times = this.vars.list.length; i < times; i++) {
  yield* this.doSomething(this.itemOf(this.vars.list, this.vars.index - 1));
  this.vars.index++;
  yield;
}

Tested manually. I'd have liked to update the snapshot test, but couldn't figure out how to open and edit it in Scratch without inadvertently causing a number of unrelated changes to the sb3.

Requesting a review from either or both - this is a change to Leopard project generation that will affect quite a number of projects, though it's always an improvement for compatibility with Scratch. IMO it lays bare how both the repeat block and for loops work - but declaring two variables in the beginning of a for loop is admittedly not very common.

At the moment, it's impossible to move the times declaration out of the for loop because we don't have trck which JavaScript variables in-scope - you'd have to ensure let times1, let times2, etc get declared, not just reusing the one name. That said, this is only an issue for loops on the same level, not loops nested in each other:

/* i & times don't exist */
for (        /* loop A                    */
  let i = 0, times = 5 * 4;
  i < times; /* i & times are from loop A */
  i++        /* i is from loop A          */
) {
  /* i & times are from loop A */
  for (        /* loop B                    */
    let i = 0, times = 100 + 25;
    i < times; /* i & times are from loop B */
    i++        /* i is from loop B          */
  ) {
    /* i & times are from loop B */
    ...
  }
  /* i & times are from loop A */
}
/* i & times don't exist */

If we wanted to go with a separate times declaration from the loop, I think that would be doable - but I don't feel the code would be much nicer.

// After, hypothetically

for (let i = 0; i < 10; i++) {
  this.x += 10;
  yield;
}
let times1 = this.y + 10;
for (let i = 0; i < times1; i++) {
  this.y += 10;
  yield;
}
this.vars.index = 1;
let times2 = this.vars.list.length;
for (let i = 0; i < times2; i++) {
  yield* this.doSomething(this.itemOf(this.vars.list, this.vars.index - 1));
  this.vars.index++;
  yield;
}

So my preference is to leave it as this PR has it (and not just because that's less work for me! LOL). But interested to hear what others think.

[PullJosh]

Nice! I kinda forgot that you can declare whatever you want within the for loop. Just to make sure: we should never run into a case where the code within the loop is expecting to access a times variable from outside the loop, right? All user-defined Scratch variables are under this.vars so I think we're in the clear?

[towerofnix]

Yep, absolutely. There are no expressions which aren't prefixed by some constant identifier (usually this, but also Sprite, Color, Math, Date) — and no arbitrary property accesses that aren't prefixed on this.vars, this.watchers, this.stage.vars etc.

It might be interesting to explore "transforming" variables that are only accessed within the scope of a single script/custom block (or whose value can be proven to be exactly known within that script and never accessed outside that script) into actual local JavaScript variables, but that's definitely something to look at later!

[PullJosh]

Looks great! 😄

[PullJosh]

no arbitrary property accesses that aren't prefixed on this.vars, this.watchers, this.stage.vars etc.

I just realized that custom blocks are converted to methods with inputs as parameters, which have no prefix. Could we end up with a situation like this?

*myCustomBlock(times) { // Suppose times = 5 and this.vars.count = 10
  for (let i = 0, times = this.vars.count; i < times; i++) {
    this.say(times); // Should say "5" but instead says "10"
  }
}

[towerofnix]

I just realized that custom blocks are converted to methods with inputs as parameters, which have no prefix. Could we end up with a situation like this?

That is terrifying :)

Excellent catch. I'm going to investigate this but I think you're right.

On an interim solution, I'm probably just going to iterate over times, times2, times3, etc, until a name is found which doesn't clash with procedure input names. Fortunately blockToJS is (in general) scoped in blockToJSWithContext, which gives access to the script, so this is doable at all.

I don't think it would make sense to roll a more complex system of getting available local variable names into this PR, but I'd be happy to do a follow-up (as a PR or issue) exploring that. It would be a useful capability to have for performing more complex block-to-JS operations in general!

[PullJosh]

That is terrifying :)

I felt that and agree. 😅 But I think your proposed solution makes sense.

[towerofnix]

Just did a force push to rebase after merging #124. Compare before (45be08a) and after (1e17b65) - this one benefitted a lot from that cleanup, and that'll be pretty typical of similar pull requests! ^^

[towerofnix]

I just made a test project to confirm each of the things that should be working: https://scratch.mit.edu/projects/941395074/

(The last two still aren't, of course!)

[towerofnix]

Hoo-ray! We fixed the problems with overshadowing a custom block input named "times" - as well as sequentially higher names - and did the same thing for "i", since that's also a name we use, and might accidentally overshadow.

Plus, we added a totally new snapshot test, to actually make sure this is working. Highly recommend looking at the 4 latest commits ("add test ... don't overshadow i, either") in order, to understand our process:

1. Add the snapshot test that captures the current behavior (note that we only take a snapshot of the sprite we're interested in, for convenience)
2. Manually update the snapshot test with the expected behavior - at this point the snapshot test fails
3. Do one part of the fix - gets closer to the snapshot test but stil lfails
4. Do the next part of the fix - matches the snapshot test, succeeds

We went back and edited commits 1 and 2 as we realized they needed additions or changes.

We also tested manually by using the test project we made in December: https://scratch.mit.edu/projects/941395074/

Note that in one case we expect this code:

*avoidAbscuring(times, i) {
  for (let i2 = 0; i2 < 1; i2++) {
    for (
      let i2 = 0, times2 = this.x + 6 * this.toNumber(i);
      i2 < times2;
      i2++
    ) {
      this.x += this.toNumber(times);
      yield;
    }
    yield;
  }
}

This is OK JS, but it's yucky (we define i2 twice, ew). We think there's a kind of dumb approach to make this nicer, will give that a shot....

[towerofnix]

Yes, that worked! It was pretty much as simple as associating the unique-local-var-name function with the script, so it could be reused in blockToJS calls. The resulting code is hopefully quite a bit neater too, and we can use uniqueLocalVarName in other block definitions if ever need be. Have a look at the commit updating the snapshot test if you're curious!

We're pretty sure this is basically the perfect solution to #122 (comment) LOL. If it keeps working as it appears to~

[towerofnix]

One more addendum: Currently, uniqueLocalVarName is not suitable for arbitrary use, i.e. to convert user-defined names. To support that, it must never return a name in JS_RESERVED_WORDS, and it should never overwrite some arbitrary common global names - including lowercase-prefix letters. That's a bit related to #138. It's out of scope for this PR since we only call it with hard-coded "preferred" names ("i", "times") and those won't cause problems.

[PullJosh]

This looks great. Thank you for adding an additional (very comprehensive) test.

[PullJosh]

In the future, I would consider using i, j, k, etc. as opposed to i, i2, i3, etc. but this PR is perfect for now.