toLeopard: Translation for repeat blocks is incorrect

[ZXMushroom63]

LeopardJS coverts Scratch's 'repeat n times' block in to a for loop, like this:

for (i=0; i < n; i++) {
   //code
}

However, this translation is invalid when it comes to n being non-static, as while Scratch 3 only calculates it once and keeps the value, leopard will calculate it at each iteration of the loop.

for (i=0; i < (someValue - someOtherValue); i++) {
   //code
}

I encountered this issue when I was converting a project with looped through the length of a list minus a variable, and changed the variable inside the loop.
An easy fix would be to have a variable before the for loop:

var tmp = (someValue - someOtherValue);
for (i=0; i < tmp; i++) {
   //code
}

[towerofnix]

Yeah, good catch. There's a bit of a philosophical trouble over this, and it has to do with how much we translate (and thus detect) Scratch programming constructs into analogous JavaScript constructs, to the ends of trying to teach more of JavaScript the way you'd see it in the real world.

For example Scratch doesn't have the basic for construct in JavaScript or other languages, which is in its general form quite complicated: "starting from an initial state, looping until a certain condition is met, performing a certain iterating action, and taking these statements for each iteration..." Instead it's just got "repeat N times" and "repeat until condition". The latter is almost directly analogous to while (it's only flipped), but the ƒormer doesn't have a direct analogy in most languages, JavaScript included.

A "simple" fix would be to try to determine if the value in the repeat block's input is static. This could be as simple as checking if it's a number or as complex as checking if it's a reporter expression which only depends on variables, lists, or other values that certainly won't be mutated over the course of the repeat block's evaluation. (Not impossible to guess at, but definitely not trivial.)

// scratch: repeat (10) ...
for (let i = 0; i < 10; i++) { ... }

// scratch: repeat ((4) + (6)) ...

// Could translate like this:
for (let i = 0; i < 4 + 6; i++) { ... }

// Or like this:
let times = 4 + 6;
for (let i = 0; i < times; i++) { ... }

// scratch:
// set index to 1
// repeat length of my list:
//   say item index of my list
//   change index by 1

// Could translate like this:
for (this.index = 1; this.index < this.myList.length; this.index++) {
  this.say(this.myList[this.index - 1]);  // or whatever
}

// Or like this:
this.index = 1;
const times = this.myList.length;
for (let i = 0; i < times; i++) {
  this.say(this.myList[this.index - 1]);  // or whatever
  this.index++;
}

// scratch:
// repeat length of my list:
//   add pick random 1 to 100 to my list

const times = this.myList.length;
for (let i = 0; i < times; i++) {
  this.myList.push(random(1, 100));  // or whatever
}

And then there's the boring way that we, unfortunately, have to fall back to sometimes, mostly when Scratch behavior is particularly unique and not able to be represented concisely. You can do something similar for the "repeat" loop:

this.repeat(10, () => {
  ...
});

this.index = 1;
this.repeat(this.myList.length, () => {
  this.say(this.myList[this.index - 1]);  // or whatever
  this.index++;
});

this.repeat(this.myList.length, () => {
  this.myList.push(random(1, 100));  // or whatever
});

Despite having a certain kind of elegance, this is patently awful for actually teaching JavaScript. The trouble is that this here below really isn't much better.

this.index = 1;
const times = this.myList.length;
for (let i = 0; i < times; i++) {
  this.say(this.myList[this.index - 1]);  // or whatever
  this.index++;
}

Static code analysis is a longer term potential direction of sb-edit (and Leopard, really) that we haven't taken a lot of time to dig into because it's a major project and everyone developing sb-edit has been busy with other projects. And without a good amount of infrastructure or utilities for performing that analysis, it's just impossible to greedily translate code to analogous JS without risking breaking programs.

I've moved this issue over to sb-edit since that's what is in charge of actually converting Scratch projects. You're right in pointing out a bug: existing projects with reporters in the "if" block don't work. For now I think we're best off having a special-case for if the input is a number (not a block), which will behave like it currently does, and in all other cases we store the number of times in a temporary variable, as you suggest.

On a practical note, for now, the times variable has to be declared in the for loop (as part of the initial state). This isn't a common code practice per se, but it's necessary to avoid scoping problems - we currently hard-code the name i in for (let i = ...) and that's only acceptable because i is scoped into the for loop, not the function scope or containing block scope - either of which there would be issues if there were two repeat blocks in one script.

this.index = 1;
for (let i = 0, times = this.myList.length; i < times; i++) {
  this.say(this.myList[this.index - 1]);
  this.index++;
}

[ZXMushroom63]

I think I understand Leopard's goal a bit better now :)
It's not just about converting a project in to JavaScript, it's also about making it look as close as possible to natural JS.

I've also found another bug by accident, which is the modulo operator

[ZXMushroom63]

I think just checking if the repeat's input is a static number would be enough, as it is: a simple change, fixes an obscure bug, and looks neat and understandable to someone trying to learn JS.