Repository: sjbarag/brs
Pull request #311 (2019-09-02 17:39:17)

Fixed `for loop` issue that should not process loop when step is on the wrong direction of the start-end parameters #310

lvcabral: Added both fixes for #310 and #315 on the same PR

Patch:
diff --git a/src/interpreter/index.ts b/src/interpreter/index.ts
index f1b0cc8..e883766 100644
--- a/src/interpreter/index.ts
+++ b/src/interpreter/index.ts
@@ -19,6 +19,7 @@ import {
     Callable,
     BrsNumber,
     Comparable,
+    Float,
 } from "../brsTypes";
 
 import { Lexeme } from "../lexer";
@@ -984,9 +985,19 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
         // BrightScript for/to loops evaluate the counter initial value, final value, and increment
         // values *only once*, at the top of the for/to loop.
         this.execute(statement.counterDeclaration);
-        const finalValue = this.evaluate(statement.finalValue);
-        const increment = this.evaluate(statement.increment);
-
+        const startValue = this.evaluate(statement.counterDeclaration.value) as Int32 | Float;
+        const finalValue = this.evaluate(statement.finalValue) as Int32 | Float;
+        let increment = this.evaluate(statement.increment) as Int32 | Float;
+        if (increment instanceof Float) {
+            increment = new Int32(Math.trunc(increment.getValue()));
+        }
+        if (
+            (startValue.getValue() > finalValue.getValue() && increment.getValue() > 0) ||
+            (startValue.getValue() < finalValue.getValue() && increment.getValue() < 0)
+        ) {
+            // Shortcut, do not process anything
+            return BrsInvalid.Instance;
+        }
         const counterName = statement.counterDeclaration.name;
         const step = new Stmt.Assignment(
             { equals: statement.tokens.for },
@@ -1015,34 +1026,52 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
 
         let loopExitReason: Stmt.BlockEnd | undefined;
 
-        while (
-            this.evaluate(new Expr.Variable(counterName))
-                .equalTo(finalValue)
-                .not()
-                .toBoolean()
-        ) {
-            // execute the block
-            try {
-                this.execute(statement.body);
-            } catch (reason) {
-                if (reason instanceof Stmt.ExitForReason) {
-                    loopExitReason = reason;
-                    break;
-                } else {
-                    // re-throw returns, runtime errors, etc.
-                    throw reason;
+        if (increment.getValue() > 0) {
+            while (
+                (this.evaluate(new Expr.Variable(counterName)) as Int32 | Float)
+                    .greaterThan(finalValue)
+                    .not()
+                    .toBoolean()
+            ) {
+                // execute the block
+                try {
+                    this.execute(statement.body);
+                } catch (reason) {
+                    if (reason instanceof Stmt.ExitForReason) {
+                        loopExitReason = reason;
+                        break;
+                    } else {
+                        // re-throw returns, runtime errors, etc.
+                        throw reason;
+                    }
                 }
-            }
 
-            // then increment the counter
-            this.execute(step);
-        }
+                // then increment the counter
+                this.execute(step);
+            }
+        } else {
+            while (
+                (this.evaluate(new Expr.Variable(counterName)) as Int32 | Float)
+                    .lessThan(finalValue)
+                    .not()
+                    .toBoolean()
+            ) {
+                // execute the block
+                try {
+                    this.execute(statement.body);
+                } catch (reason) {
+                    if (reason instanceof Stmt.ExitForReason) {
+                        loopExitReason = reason;
+                        break;
+                    } else {
+                        // re-throw returns, runtime errors, etc.
+                        throw reason;
+                    }
+                }
 
-        // BrightScript for/to loops execute the body one more time when initial === final
-        if (loopExitReason === undefined) {
-            this.execute(statement.body);
-            // they also increments the counter once more
-            this.execute(step);
+                // then increment the counter
+                this.execute(step);
+            }
         }
 
         return BrsInvalid.Instance;


Test patch:
diff --git a/test/e2e/Syntax.test.js b/test/e2e/Syntax.test.js
index 7bf9184..5dd1e03 100644
--- a/test/e2e/Syntax.test.js
+++ b/test/e2e/Syntax.test.js
@@ -125,6 +125,8 @@ describe("end to end syntax", () => {
             "2",
             "1",
             "0", // count down
+            "128", // step non multiple of final
+            "85", // step non multiple of final
             "for loop exit", // exit early
             "0", // e after loop
             "initial = final", // loop where initial equals final
diff --git a/test/e2e/resources/for-loops.brs b/test/e2e/resources/for-loops.brs
index a710df1..ce63333 100644
--- a/test/e2e/resources/for-loops.brs
+++ b/test/e2e/resources/for-loops.brs
@@ -17,6 +17,26 @@ for d = 3 to 0 step -1
     print d
 end for
 
+' make sure positive step works even if not a multiple of the target
+steps = 0
+for b = 0 to 255 step 2
+    steps++
+    if b > 255 then exit for
+next
+'it should result in 128
+print steps
+
+' make sure negative step works even if not a multiple of the target
+steps = 0
+for b = 255 to 3 step -3.9 'steps are always truncated to the integer part
+    steps++
+    if b < 3 then exit for
+next
+'it should result in 128
+print steps
+
+
+
 ' ensure for/to loops can be exited
 for e = 0 to 10
     print "for loop exit"
@@ -29,4 +49,16 @@ print e
 ' ensure body is executed when initial=final
 for a = 0 to 0
     print "initial = final"
-end for
\ No newline at end of file
+end for
+
+'ensure do not loop if final is lower than initial and step > 0
+for a = 0 to -1
+    print "this should not be printed"
+    exit for
+next
+
+'ensure do not loop if final is higher than initial and step < 0
+for a = 1.9 to 10.2 step -1.1
+    print "this should not be printed"
+    exit for
+next
\ No newline at end of file