Count a for-range loop variable's start expression as a read

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/controlflow/DataflowAnomalyAnalysis.java

@@ -71,11 +71,26 @@ public boolean uninitialized(NameDef v) {
     }
 
     public VarStates addRead(LocalVarDef v, Element r) {
+        return withRead(v, r, null);
+    }
+
+    /**
+     * Records a read that cannot observe writes made inside {@code excluded}.
+     *
+     * <p>A for-range start expression is evaluated once before the loop is entered, but the CFG
+     * back edge revisits the loop statement, so a plain read would also mark writes made in the
+     * loop body - suppressing a genuine never-read warning for them.
+     */
+    public VarStates addReadOutside(LocalVarDef v, Element r, Element excluded) {
+        return withRead(v, r, excluded);
+    }
+
+    private VarStates withRead(LocalVarDef v, Element r, @Nullable Element excluded) {
         VState s = getVarState(v);
         if (s == null) {
             s = VState.initialDefined;
         }
-        s = s.addRead(r);
+        s = s.addRead(r, excluded);
         Builder<LocalVarDef, VState> builder = ImmutableMap.builder();
         for (Entry<LocalVarDef, VState> e : states.entrySet()) {
             if (e.getKey() != v) {
@@ -207,14 +222,30 @@ public VState addWrite(WStatement s) {
     }
 
     public VState addRead(Element r) {
+        return addRead(r, null);
+    }
+
+    public VState addRead(Element r, @Nullable Element excluded) {
         ImmutableSetMultimap.Builder<WStatement, Element> builder = ImmutableSetMultimap.builder();
         builder.putAll(writesAndReads);
         for (WStatement s : this.activeWrites) {
-            builder.put(s, r);
+            if (excluded == null || !isInside(s, excluded)) {
+                builder.put(s, r);
+            }
         }
         return new VState(mightBeUninitialized, mightBeDestroyed, builder.build(), activeWrites, allWrites);
     }
 
+    /** Whether {@code node} lies within the subtree rooted at {@code ancestor}. */
+    private static boolean isInside(Element node, Element ancestor) {
+        for (Element e = node; e != null; e = e.getParent()) {
+            if (e == ancestor) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     public VState merge(VState other) {
         return new VState(mightBeUninitialized || other.mightBeUninitialized,
                 mightBeDestroyed || other.mightBeDestroyed,
@@ -290,11 +321,18 @@ VarStates calculate(WStatement s, VarStates incoming) {
 
 
         if (s instanceof CompoundStatement) {
+            // A loop that declares its own variable evaluates its whole header exactly once before
+            // the loop is entered - StmtTranslation assigns the start expression and hoists "to",
+            // "step" and "in" into temporaries ahead of the ImLoop. A while condition, by contrast,
+            // is re-evaluated per iteration, so only this family gets the once-only treatment.
+            boolean headerEvaluatedOnce = s instanceof LoopStatementWithVarDef;
             // for a compound statement check only the expressions in the statement
             for (int i = 0; i < s.size(); i++) {
                 if (s.get(i) instanceof Expr) {
                     Expr expr = (Expr) s.get(i);
-                    incoming = handleExprInCompound(incoming, expr);
+                    incoming = headerEvaluatedOnce
+                        ? handleLoopHeaderExpr(incoming, expr, s)
+                        : handleExprInCompound(incoming, expr);
                 }
             }
             if (s instanceof SwitchStmt) {
@@ -305,6 +343,16 @@ VarStates calculate(WStatement s, VarStates incoming) {
                         incoming = handleExprInCompound(incoming, switchCaseExpr);
                     }
                 }
+            } else if (s instanceof LoopStatementWithVarDef) {
+                // Same reason: for "for i = a downto 0" the start expression belongs to the loop
+                // variable's LocalVarDef, not to the loop statement, so the loop above never sees
+                // it and a local read only appearing there looked like a dead assignment.
+                // StmtForFrom binds its loop variable from the "in" expression and has no initial
+                // expression of its own, which the instanceof guard covers.
+                LocalVarDef loopVar = ((LoopStatementWithVarDef) s).getLoopVar();
+                if (loopVar.getInitialExpr() instanceof Expr) {
+                    incoming = handleLoopHeaderExpr(incoming, (Expr) loopVar.getInitialExpr(), s);
+                }
             }
         } else {
             checkIfVarsInitialized(s, incoming);
@@ -378,6 +426,25 @@ private boolean checkNoAccessToThis(Element s) {
         return false;
     }
 
+    /**
+     * Handles a loop header expression - the start value, "to", "step" or "in" - each of which
+     * runs exactly once before the loop is entered.
+     *
+     * <p>The CFG has a back edge to the loop statement, so the fixpoint evaluates this again on
+     * later iterations. Reads are therefore recorded only against writes outside the loop: a write
+     * in the loop body happens after the header has already been evaluated and cannot be observed
+     * by it, so counting it as read would hide a genuine dead assignment.
+     */
+    private VarStates handleLoopHeaderExpr(VarStates incoming, Expr headerExpr, WStatement loop) {
+        checkIfVarsInitialized(headerExpr, incoming);
+        for (NameDef v : headerExpr.attrReadVariables()) {
+            if (isLocalVarDef(v)) {
+                incoming = incoming.addReadOutside((LocalVarDef) v, headerExpr, loop);
+            }
+        }
+        return incoming;
+    }
+
     private VarStates handleExprInCompound(VarStates incoming, Expr expr) {
         checkIfVarsInitialized(expr, incoming);
         for (NameDef v : expr.attrReadVariables()) {

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java

@@ -966,6 +966,110 @@ public void forRangeStartReadsParameter() {
         );
     }
 
+    /** A local read by the loop variable's start expression is read, whichever way the loop counts. */
+    @Test
+    public void forRangeStartReadsLocal() {
+        for (String direction : new String[]{"downto 0", "to 20"}) {
+            CompilationResult result = test()
+                .setStopOnFirstError(false)
+                .executeProg(false)
+                .lines(
+                    "package test",
+                    "native testSuccess()",
+                    "init",
+                    "    let a = 10",
+                    "    for i = a " + direction,
+                    "        skip",
+                    "endpackage"
+                );
+
+            Assert.assertTrue(
+                result.getGui().getWarningList().stream()
+                    .noneMatch(w -> w.getMessage().contains("assignment to local variable a is never read")),
+                "Unexpected never-read warning for 'a' with 'for i = a " + direction + "': "
+                    + result.getGui().getWarningList()
+            );
+        }
+    }
+
+    /**
+     * The start expression runs once before the loop, so a write in the body is not read by it.
+     * The CFG back edge revisits the loop statement, which would otherwise mark the body write as
+     * read and hide this warning.
+     */
+    @Test
+    public void forRangeStartDoesNotCountBodyReassignmentAsRead() {
+        CompilationResult result = test()
+            .setStopOnFirstError(false)
+            .executeProg(false)
+            .lines(
+                "package test",
+                "native testSuccess()",
+                "init",
+                "    var a = 10",
+                "    for i = a to 20",
+                "        a = 5",
+                "endpackage"
+            );
+
+        Assert.assertTrue(
+            result.getGui().getWarningList().stream()
+                .anyMatch(w -> w.getMessage().contains("assignment to local variable a is never read")),
+            "Expected the dead body assignment to 'a' to still warn, got: " + result.getGui().getWarningList()
+        );
+    }
+
+    /**
+     * "to" is hoisted into a temporary before the loop just like the start value, so a body write
+     * to a local it reads is equally dead. Same shape as the start expression, different slot.
+     */
+    @Test
+    public void forRangeToBoundDoesNotCountBodyReassignmentAsRead() {
+        CompilationResult result = test()
+            .setStopOnFirstError(false)
+            .executeProg(false)
+            .lines(
+                "package test",
+                "native testSuccess()",
+                "init",
+                "    var n = 20",
+                "    for i = 0 to n",
+                "        n = 5",
+                "endpackage"
+            );
+
+        Assert.assertTrue(
+            result.getGui().getWarningList().stream()
+                .anyMatch(w -> w.getMessage().contains("assignment to local variable n is never read")),
+            "Expected the dead body assignment to 'n' to still warn, got: " + result.getGui().getWarningList()
+        );
+    }
+
+    /** A body write that is genuinely read later must not warn. */
+    @Test
+    public void forRangeBodyReassignmentReadAfterLoopDoesNotWarn() {
+        CompilationResult result = test()
+            .setStopOnFirstError(false)
+            .executeProg(false)
+            .lines(
+                "package test",
+                "native testSuccess()",
+                "@extern native I2S(int x) returns string",
+                "init",
+                "    var a = 10",
+                "    for i = a to 20",
+                "        a = 5",
+                "    I2S(a)",
+                "endpackage"
+            );
+
+        Assert.assertTrue(
+            result.getGui().getWarningList().stream()
+                .noneMatch(w -> w.getMessage().contains("assignment to local variable a is never read")),
+            "Unexpected never-read warning when 'a' is read after the loop: " + result.getGui().getWarningList()
+        );
+    }
+
     @Test
     public void forRangeLoopVarMutationWarns() {
         testAssertWarningsLines(false, "unexpected iteration side effects",