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]

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 (45be08ac2c44cfb212c28b532d02c4f1d62a8ad3) and after (1e17b659bedbd49673701a3aca23086616ad9edc) - 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!)