diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/controlflow/DataflowAnomalyAnalysis.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/controlflow/DataflowAnomalyAnalysis.java index 0234df8a1..7d834bff8 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/controlflow/DataflowAnomalyAnalysis.java +++ b/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}. + * + *

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 builder = ImmutableMap.builder(); for (Entry 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 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. + * + *

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()) { diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java index d20248032..f81f572c3 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java +++ b/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",