diff --git a/Compiler/Core/Tests/LoopLoweringTests.cpp b/Compiler/Core/Tests/LoopLoweringTests.cpp index cd84a2cd..79d57432 100644 --- a/Compiler/Core/Tests/LoopLoweringTests.cpp +++ b/Compiler/Core/Tests/LoopLoweringTests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -451,3 +452,55 @@ TEST_CASE("do-while continue targets the trailing condition block", for (const auto &block : function.blocks) CHECK(ReachesReturn(function, block.id)); } + +TEST_CASE("Core verifier rejects continue in a for update region", + "[core][verifier][loop]") +{ + const auto codes = [](std::vector body, + std::vector update) { + std::vector result; + for (const auto &issue : Core::Verify(LoopModule(Core::Statement::For( + Compare(Core::Primitive::LessThan, kIndex, 3), + std::move(body), + std::move(update))))) + result.push_back(issue.code); + return result; + }; + const auto has + = [](const std::vector &found, std::string_view code) { + return std::ranges::find(found, code) != found.end(); + }; + + // The update region is the loop's continuation point: a `continue` + // there would re-enter the update and never test the condition again. + CHECK(has(codes({}, { Core::Statement::Continue() }), "VXC1066")); + CHECK(has( + codes({}, + { Core::Statement::If(Compare(Core::Primitive::Equal, kIndex, 1), + { Core::Statement::Continue() }, + {}) }), + "VXC1066")); + + // `continue` in the body, `break` in the update, and `continue` in a + // loop nested inside the update all keep a defined target. + CHECK( + codes({ Core::Statement::Continue() }, { Increment(kIndex, U"index") }) + .empty()); + CHECK(codes({ Accumulate() }, + { Increment(kIndex, U"index"), Core::Statement::Break() }) + .empty()); + CHECK(codes({ Accumulate() }, + { Increment(kIndex, U"index"), + Core::Statement::While( + Compare(Core::Primitive::LessThan, kTotal, 0), + { Core::Statement::Continue() }) }) + .empty()); + + // Outside any loop the existing diagnostic applies, not the new one. + auto module = LoopModule(Core::Statement::Continue()); + std::vector outside; + for (const auto &issue : Core::Verify(module)) + outside.push_back(issue.code); + CHECK(has(outside, "VXC1065")); + CHECK_FALSE(has(outside, "VXC1066")); +} diff --git a/Compiler/Core/Verifier.cpp b/Compiler/Core/Verifier.cpp index a5bdd808..dc5572b4 100644 --- a/Compiler/Core/Verifier.cpp +++ b/Compiler/Core/Verifier.cpp @@ -178,23 +178,37 @@ namespace Visual::XSharp::Core } return false; } + /// Where a `break` or `continue` would transfer to. A `for` + /// update region is inside its loop for `break`, which leaves the + /// loop, but it is the loop's continuation point itself: a + /// `continue` there has no later point of the same iteration to + /// reach and would re-enter the update without testing the + /// condition. A loop nested in an update opens a body scope again. + enum class TransferScope : std::uint8_t + { + OutsideLoop, + InLoopBody, + InForUpdate + }; + void VerifyStatements(const llvm::ArrayRef statements, Environment &environment, const Type &expectedReturnType, - const std::size_t loopDepth = 0U) + const TransferScope scope + = TransferScope::OutsideLoop) { for (const auto &statement : statements) VerifyStatement(statement, environment, expectedReturnType, - loopDepth); + scope); } void VerifyStatement(const Statement &statement, Environment &environment, const Type &expectedReturnType, - const std::size_t loopDepth) + const TransferScope scope) { switch (statement.kind) { @@ -269,11 +283,11 @@ namespace Visual::XSharp::Core VerifyStatements(statement.trueBranch, trueEnvironment, expectedReturnType, - loopDepth); + scope); VerifyStatements(statement.falseBranch, falseEnvironment, expectedReturnType, - loopDepth); + scope); return; } case Statement::Kind::Evaluate: @@ -291,25 +305,29 @@ namespace Visual::XSharp::Core VerifyStatements(statement.loopBody, loopEnvironment, expectedReturnType, - loopDepth + 1U); + TransferScope::InLoopBody); if (statement.kind == Statement::Kind::For) { auto updateEnvironment = environment; VerifyStatements(statement.loopUpdate, updateEnvironment, expectedReturnType, - loopDepth + 1U); + TransferScope::InForUpdate); } return; } case Statement::Kind::Break: - if (loopDepth == 0U) + if (scope == TransferScope::OutsideLoop) Add("VXC1064", "Core break appears outside a loop"); return; case Statement::Kind::Continue: - if (loopDepth == 0U) + if (scope == TransferScope::OutsideLoop) Add("VXC1065", "Core continue appears outside a loop"); + if (scope == TransferScope::InForUpdate) + Add("VXC1066", + "Core continue appears in a for update " + "region"); return; } } @@ -673,7 +691,7 @@ namespace Visual::XSharp::Core VerifyStatements(*expression.closureBody, closureEnvironment, expression.closureReturnType, - 0U); + TransferScope::OutsideLoop); if (expression.closureReturnType != Type::unit() && !AlwaysReturns(*expression.closureBody)) Add("VXC1042", diff --git a/Compiler/Haskell/Core/src/Visual/XSharp/Core/Verifier.hs b/Compiler/Haskell/Core/src/Visual/XSharp/Core/Verifier.hs index 19d1d9d9..906abd34 100644 --- a/Compiler/Haskell/Core/src/Visual/XSharp/Core/Verifier.hs +++ b/Compiler/Haskell/Core/src/Visual/XSharp/Core/Verifier.hs @@ -64,7 +64,7 @@ verifyFunction functionEnvironment function = ++ unresolvedType "VXC1003" "Core function has an unresolved return type" (coreFunctionReturnType function) ++ duplicates "VXC1004" "duplicate Core parameter symbol" parameterSymbols ++ concatMap (uncurry verifyParameter) (coreFunctionParameters function) - ++ fst (verifyStatements initialEnvironment (coreFunctionReturnType function) 0 (coreFunctionBody function)) + ++ fst (verifyStatements initialEnvironment (coreFunctionReturnType function) OutsideLoop (coreFunctionBody function)) ++ missingReturn where parameterSymbols = map (resolvedSymbol . fst) (coreFunctionParameters function) @@ -82,15 +82,24 @@ verifyParameter name valueType = invalidSymbol "VXC1006" "Core parameter symbol must be positive" name ++ unresolvedType "VXC1007" "Core parameter has an unresolved type" valueType -verifyStatements :: Environment -> Type -> Int -> [CoreStatement] -> ([Diagnostic], Environment) +{- | Where a @break@ or @continue@ would transfer to. A @for@ update region is +inside its loop for @break@, which leaves the loop, but it is the loop's +continuation point itself: a @continue@ there has no later point of the same +iteration to reach and would re-enter the update without testing the +condition. A loop nested in an update opens an ordinary body scope again. +-} +data TransferScope = OutsideLoop | InLoopBody | InForUpdate + deriving (Eq) + +verifyStatements :: Environment -> Type -> TransferScope -> [CoreStatement] -> ([Diagnostic], Environment) verifyStatements environment _ _ [] = ([], environment) -verifyStatements environment returnType loopDepth (statement : remaining) = - let (currentProblems, nextEnvironment) = verifyStatement environment returnType loopDepth statement - (remainingProblems, finalEnvironment) = verifyStatements nextEnvironment returnType loopDepth remaining +verifyStatements environment returnType scope (statement : remaining) = + let (currentProblems, nextEnvironment) = verifyStatement environment returnType scope statement + (remainingProblems, finalEnvironment) = verifyStatements nextEnvironment returnType scope remaining in (currentProblems ++ remainingProblems, finalEnvironment) -verifyStatement :: Environment -> Type -> Int -> CoreStatement -> ([Diagnostic], Environment) -verifyStatement environment returnType loopDepth statement = case statement of +verifyStatement :: Environment -> Type -> TransferScope -> CoreStatement -> ([Diagnostic], Environment) +verifyStatement environment returnType scope statement = case statement of CoreBind binding -> let name = coreBindingName binding symbol = resolvedSymbol name @@ -135,29 +144,30 @@ verifyStatement environment returnType loopDepth statement = case statement of ++ [ problem "VXC1017" "Core condition must be bool or numeric" | expressionType condition /= boolType && not (isCoreNumericType (expressionType condition)) ] - (trueProblems, _) = verifyStatements environment returnType loopDepth trueBranch - (falseProblems, _) = verifyStatements environment returnType loopDepth falseBranch + (trueProblems, _) = verifyStatements environment returnType scope trueBranch + (falseProblems, _) = verifyStatements environment returnType scope falseBranch in (conditionProblems ++ trueProblems ++ falseProblems, environment) CoreEvaluate value -> (verifyExpression environment value, environment) CoreWhile condition body -> let conditionProblems = verifyLoopCondition environment "while" condition - (bodyProblems, _) = verifyStatements environment returnType (loopDepth + 1) body + (bodyProblems, _) = verifyStatements environment returnType InLoopBody body in (conditionProblems ++ bodyProblems, environment) CoreDoWhile body condition -> - let (bodyProblems, _) = verifyStatements environment returnType (loopDepth + 1) body + let (bodyProblems, _) = verifyStatements environment returnType InLoopBody body conditionProblems = verifyLoopCondition environment "do/while" condition in (bodyProblems ++ conditionProblems, environment) CoreFor condition body update -> let conditionProblems = verifyLoopCondition environment "for" condition - (bodyProblems, _) = verifyStatements environment returnType (loopDepth + 1) body - (updateProblems, _) = verifyStatements environment returnType (loopDepth + 1) update + (bodyProblems, _) = verifyStatements environment returnType InLoopBody body + (updateProblems, _) = verifyStatements environment returnType InForUpdate update in (conditionProblems ++ bodyProblems ++ updateProblems, environment) CoreBreak -> - ( [problem "VXC1045" "Core break is not nested in a loop" | loopDepth == 0] + ( [problem "VXC1045" "Core break is not nested in a loop" | scope == OutsideLoop] , environment ) CoreContinue -> - ( [problem "VXC1046" "Core continue is not nested in a loop" | loopDepth == 0] + ( [problem "VXC1046" "Core continue is not nested in a loop" | scope == OutsideLoop] + ++ [problem "VXC1066" "Core continue appears in a for update region" | scope == InForUpdate] , environment ) @@ -229,7 +239,7 @@ verifyClosure environment captures parameters returnType body valueType = ++ concatMap verifyCapture captures ++ concatMap (uncurry verifyParameter) parameters ++ callableTypeProblems - ++ fst (verifyStatements closureEnvironment returnType 0 body) + ++ fst (verifyStatements closureEnvironment returnType OutsideLoop body) ++ [ problem "VXC1032" "non-void Core closure may complete without returning" | returnType /= unitType && not (statementsAlwaysReturn body) ] diff --git a/Compiler/Haskell/Driver/test/CoreVerifierTests.hs b/Compiler/Haskell/Driver/test/CoreVerifierTests.hs index d6f9f1a7..49bd4a63 100644 --- a/Compiler/Haskell/Driver/test/CoreVerifierTests.hs +++ b/Compiler/Haskell/Driver/test/CoreVerifierTests.hs @@ -75,6 +75,23 @@ coreVerifierTests = , rejectedWith "VXC1028" (primitiveModule CoreEqual [boolValue, boolValue] intType) ) , ("Core verifier accepts all-returning branches", accepted allReturningBranch) + , ("Core verifier rejects break outside a loop", rejectedWith "VXC1045" (loopModule [CoreBreak])) + , ("Core verifier rejects continue outside a loop", rejectedWith "VXC1046" (loopModule [CoreContinue])) + , ("Core verifier accepts continue in a for body", accepted (forModule [CoreContinue] [])) + , ("Core verifier accepts break in a for update region", accepted (forModule [] [CoreBreak])) + , ("Core verifier rejects continue in a for update region", rejectedWith "VXC1066" (forModule [] [CoreContinue])) + , + ( "Core verifier rejects continue nested in a branch of a for update region" + , rejectedWith "VXC1066" (forModule [] [CoreIf boolValue [CoreContinue] []]) + ) + , + ( "Core verifier accepts continue in a loop nested in a for update region" + , accepted (forModule [] [CoreWhile boolValue [CoreContinue]]) + ) + , + ( "Core verifier does not report an outside-loop continue as an update continue" + , not (rejectedWith "VXC1066" (loopModule [CoreContinue])) + ) , ("Core verifier accepts a well-typed direct call", accepted validDirectCall) , ("Core verifier accepts a well-typed closure", accepted validClosure) ] @@ -383,3 +400,9 @@ validDirectCall = validClosure :: CoreModule validClosure = coreModule [unitFunction mainName [CoreEvaluate validClosureValue]] + +loopModule :: [CoreStatement] -> CoreModule +loopModule body = coreModule [unitFunction mainName body] + +forModule :: [CoreStatement] -> [CoreStatement] -> CoreModule +forModule body update = loopModule [CoreFor boolValue body update] diff --git a/Documents/CORE-IR.md b/Documents/CORE-IR.md index fb224c87..07b05722 100644 --- a/Documents/CORE-IR.md +++ b/Documents/CORE-IR.md @@ -234,6 +234,17 @@ tracks loop nesting independently for each function and closure body and rejects either statement outside a loop. Nested loops push a new target pair, so an inner transfer cannot accidentally jump to an outer loop. +The update list of a classic `for` is that loop's continuation point, so the +two transfers differ there. `CoreBreak` in an update list is valid and exits +the loop after the statements before it. `CoreContinue` placed directly in an +update list, including inside a `CoreIf` there, is rejected with `VXC1066`: +it has no later point of the same iteration to reach, and lowering it would +jump back to the start of the update without testing the condition. A loop +nested inside an update list has its own body and continuation point, so +`CoreContinue` is valid again inside it. Source code cannot produce this form +because a `for` update is a list of expressions; the rule protects Core built +or transformed by other means. + Optimizer passes preserve the explicit loop form unless their rewrite proves the replacement semantics, including effects and transfer edges. In particular, a constant condition does not permit deleting an effectful body or