From e4184039acebe86b26e8f23dc3ed4e33d8b79883 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Mon, 3 Nov 2025 11:00:27 -0500 Subject: [PATCH] Sema: Fix source location bookkeeping for 'reasonable time' diagnostic We already had bookkeeping to track which statement in a multi-statement closure we were looking at, but this was only used for the 'reasonable time' diagnostic in the case that we hit the expression timer, which was almost never hit, and is now off by default. The scope, memory, and trial limits couldn't use this information, so they would always diagnose the entire target being type checked. Move it up from ExpressionTimer to ConstraintSystem, so that we get the right source location there too. Also, factor out some code duplication in BuilderTransform to ensure we get the same benefit for result builders applied to function bodies too. --- include/swift/Sema/ConstraintSystem.h | 90 +++++------------ lib/Sema/BuilderTransform.cpp | 4 +- lib/Sema/CSSolver.cpp | 36 ++++--- lib/Sema/CSStep.cpp | 6 +- lib/Sema/CSStep.h | 12 +-- lib/Sema/ConstraintSystem.cpp | 96 +++++++++++++++---- .../too_complex_source_location.swift | 19 ++++ .../SwiftUI/too_complex_source_location.swift | 43 +++++++++ .../type_checker_perf/slow/rdar19612086.swift | 4 +- 9 files changed, 191 insertions(+), 119 deletions(-) create mode 100644 test/Constraints/too_complex_source_location.swift create mode 100644 validation-test/Sema/SwiftUI/too_complex_source_location.swift diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h index df9701e508aa0..f8bd5a140ee89 100644 --- a/include/swift/Sema/ConstraintSystem.h +++ b/include/swift/Sema/ConstraintSystem.h @@ -236,36 +236,23 @@ struct RestrictionOrFix { class ExpressionTimer { -public: - using AnchorType = llvm::PointerUnion; - -private: - AnchorType Anchor; - ASTContext &Context; + ConstraintSystem &CS; llvm::TimeRecord StartTime; /// The number of seconds from creation until /// this timer is considered expired. unsigned ThresholdInSecs; - bool PrintDebugTiming; bool PrintWarning; public: const static unsigned NoLimit = (unsigned) -1; - ExpressionTimer(AnchorType Anchor, ConstraintSystem &CS, - unsigned thresholdInSecs); + ExpressionTimer(ConstraintSystem &CS, unsigned thresholdInSecs); ~ExpressionTimer(); - AnchorType getAnchor() const { return Anchor; } - - SourceRange getAffectedRange() const; - - unsigned getWarnLimit() const { - return Context.TypeCheckerOpts.WarnLongExpressionTypeChecking; - } + unsigned getWarnLimit() const; llvm::TimeRecord startedAt() const { return StartTime; } /// Return the elapsed process time (including fractional seconds) @@ -2159,7 +2146,12 @@ struct ClosureIsolatedByPreconcurrency { /// solution of which assigns concrete types to each of the type variables. /// Constraint systems are typically generated given an (untyped) expression. class ConstraintSystem { +public: + using ExprOrLocator = llvm::PointerUnion; + +private: ASTContext &Context; + ExprOrLocator CurrentAnchor; public: DeclContext *DC; @@ -5394,6 +5386,9 @@ class ConstraintSystem { /// \returns The selected conjunction. Constraint *selectConjunction(); + void diagnoseTooComplex(SourceLoc fallbackLoc, + SolutionResult &result); + /// Solve the system of constraints generated from provided expression. /// /// \param target The target to generate constraints from. @@ -5491,6 +5486,8 @@ class ConstraintSystem { compareSolutions(ConstraintSystem &cs, ArrayRef solutions, const SolutionDiff &diff, unsigned idx1, unsigned idx2); + void startExpressionTimer(); + public: /// Increase the score of the given kind for the current (partial) solution /// along the current solver path. @@ -5528,7 +5525,6 @@ class ConstraintSystem { std::optional findBestSolution(SmallVectorImpl &solutions, bool minimize); -public: /// Apply a given solution to the target, producing a fully /// type-checked target or \c None if an error occurred. /// @@ -5581,7 +5577,17 @@ class ConstraintSystem { /// resolved before any others. void optimizeConstraints(Expr *e); - void startExpressionTimer(ExpressionTimer::AnchorType anchor); + /// Set the current sub-expression (of a multi-statement closure, etc) for + /// the purposes of diagnosing "reasonable time" errors. + void startExpression(ExprOrLocator anchor); + + /// Returns the above anchor. + ExprOrLocator getCurrentAnchor() const { + return CurrentAnchor; + } + + /// The source range of the above. + SourceRange getAffectedRange() const; /// Determine if we've already explored too many paths in an /// attempt to solve this expression. @@ -5594,53 +5600,7 @@ class ConstraintSystem { return range.isValid() ? range : std::optional(); } - bool isTooComplex(size_t solutionMemory) { - if (isAlreadyTooComplex.first) - return true; - - auto CancellationFlag = getASTContext().CancellationFlag; - if (CancellationFlag && CancellationFlag->load(std::memory_order_relaxed)) - return true; - - auto markTooComplex = [&](SourceRange range, StringRef reason) { - if (isDebugMode()) { - if (solverState) - llvm::errs().indent(solverState->getCurrentIndent()); - llvm::errs() << "(too complex: " << reason << ")\n"; - } - isAlreadyTooComplex = {true, range}; - return true; - }; - - auto used = getASTContext().getSolverMemory() + solutionMemory; - MaxMemory = std::max(used, MaxMemory); - auto threshold = getASTContext().TypeCheckerOpts.SolverMemoryThreshold; - if (MaxMemory > threshold) { - // No particular location for OoM problems. - return markTooComplex(SourceRange(), "exceeded memory limit"); - } - - if (Timer && Timer->isExpired()) { - // Disable warnings about expressions that go over the warning - // threshold since we're arbitrarily ending evaluation and - // emitting an error. - Timer->disableWarning(); - - return markTooComplex(Timer->getAffectedRange(), "exceeded time limit"); - } - - auto &opts = getASTContext().TypeCheckerOpts; - - // Bail out once we've looked at a really large number of choices. - if (opts.SolverScopeThreshold && NumSolverScopes > opts.SolverScopeThreshold) - return markTooComplex(SourceRange(), "exceeded scope limit"); - - // Bail out once we've taken a really large number of steps. - if (opts.SolverTrailThreshold && NumTrailSteps > opts.SolverTrailThreshold) - return markTooComplex(SourceRange(), "exceeded trail limit"); - - return false; - } + bool isTooComplex(size_t solutionMemory); bool isTooComplex(ArrayRef solutions) { if (isAlreadyTooComplex.first) diff --git a/lib/Sema/BuilderTransform.cpp b/lib/Sema/BuilderTransform.cpp index 326fb242476f9..a1e11d4dd4a98 100644 --- a/lib/Sema/BuilderTransform.cpp +++ b/lib/Sema/BuilderTransform.cpp @@ -1053,9 +1053,7 @@ TypeChecker::applyResultBuilderBodyTransform(FuncDecl *func, Type builderType) { case SolutionResult::Kind::TooComplex: reportSolutionsToSolutionCallback(salvagedResult); - func->diagnose(diag::expression_too_complex) - .highlight(func->getBodySourceRange()); - salvagedResult.markAsDiagnosed(); + cs.diagnoseTooComplex(func->getLoc(), salvagedResult); return nullptr; } diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp index a17c7222a0bf9..95f61f4018cbd 100644 --- a/lib/Sema/CSSolver.cpp +++ b/lib/Sema/CSSolver.cpp @@ -829,9 +829,7 @@ bool ConstraintSystem::Candidate::solve( // Allocate new constraint system for sub-expression. ConstraintSystem cs(DC, std::nullopt); - - // Set up expression type checker timer for the candidate. - cs.startExpressionTimer(E); + cs.startExpression(E); // Generate constraints for the new system. if (auto generatedExpr = cs.generateConstraints(E, DC)) { @@ -1455,18 +1453,7 @@ ConstraintSystem::solve(SyntacticElementTarget &target, return std::nullopt; case SolutionResult::TooComplex: { - auto affectedRange = solution.getTooComplexAt(); - - // If affected range is unknown, let's use whole - // target. - if (!affectedRange) - affectedRange = target.getSourceRange(); - - getASTContext() - .Diags.diagnose(affectedRange->Start, diag::expression_too_complex) - .highlight(*affectedRange); - - solution.markAsDiagnosed(); + diagnoseTooComplex(target.getLoc(), solution); return std::nullopt; } @@ -1501,6 +1488,19 @@ ConstraintSystem::solve(SyntacticElementTarget &target, llvm_unreachable("Loop always returns"); } +void ConstraintSystem::diagnoseTooComplex(SourceLoc fallbackLoc, + SolutionResult &result) { + auto affectedRange = result.getTooComplexAt(); + + SourceLoc loc = (affectedRange ? affectedRange->Start : fallbackLoc); + auto diag = getASTContext().Diags.diagnose(loc, diag::expression_too_complex); + + if (affectedRange) + diag.highlight(*affectedRange); + + result.markAsDiagnosed(); +} + SolutionResult ConstraintSystem::solveImpl(SyntacticElementTarget &target, FreeTypeVariableBinding allowFreeTypeVariables) { @@ -1518,9 +1518,8 @@ ConstraintSystem::solveImpl(SyntacticElementTarget &target, assert(!solverState && "cannot be used directly"); - // Set up the expression type checker timer. if (Expr *expr = target.getAsExpr()) - startExpressionTimer(expr); + startExpression(expr); if (generateConstraints(target, allowFreeTypeVariables)) return SolutionResult::forError(); @@ -1701,8 +1700,7 @@ bool ConstraintSystem::solveForCodeCompletion( // Tell the constraint system what the contextual type is. setContextualInfo(expr, target.getExprContextualTypeInfo()); - // Set up the expression type checker timer. - startExpressionTimer(expr); + startExpression(expr); shrink(expr); } diff --git a/lib/Sema/CSStep.cpp b/lib/Sema/CSStep.cpp index 8b52fad07a6d5..c451357a27cbf 100644 --- a/lib/Sema/CSStep.cpp +++ b/lib/Sema/CSStep.cpp @@ -823,10 +823,10 @@ bool ConjunctionStep::attempt(const ConjunctionElement &element) { // (expression) gets a fresh time slice to get solved. This // is important for closures with large number of statements // in them. - if (CS.Timer) { + if (CS.Timer) CS.Timer.reset(); - CS.startExpressionTimer(element.getLocator()); - } + + CS.startExpression(element.getLocator()); auto success = element.attempt(CS); diff --git a/lib/Sema/CSStep.h b/lib/Sema/CSStep.h index a0b7a40cb03b9..df92850ea2b34 100644 --- a/lib/Sema/CSStep.h +++ b/lib/Sema/CSStep.h @@ -867,8 +867,7 @@ class ConjunctionStep : public BindingStep { /// The number of milliseconds until outer constraint system /// is considered "too complex" if timer is enabled. - std::optional> - OuterTimeRemaining = std::nullopt; + std::optional OuterTimeRemaining = std::nullopt; /// Conjunction constraint associated with this step. Constraint *Conjunction; @@ -910,7 +909,7 @@ class ConjunctionStep : public BindingStep { if (cs.Timer) { auto remainingTime = cs.Timer->getRemainingProcessTimeInSeconds(); - OuterTimeRemaining.emplace(cs.Timer->getAnchor(), remainingTime); + OuterTimeRemaining.emplace(remainingTime); } } @@ -925,11 +924,8 @@ class ConjunctionStep : public BindingStep { if (HadFailure) restoreBestScore(); - if (OuterTimeRemaining) { - auto anchor = OuterTimeRemaining->first; - auto remainingTime = OuterTimeRemaining->second; - CS.Timer.emplace(anchor, CS, remainingTime); - } + if (OuterTimeRemaining) + CS.Timer.emplace(CS, *OuterTimeRemaining); } StepResult resume(bool prevFailed) override; diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp index 7823676ee3e3b..bae73c2262e5e 100644 --- a/lib/Sema/ConstraintSystem.cpp +++ b/lib/Sema/ConstraintSystem.cpp @@ -54,43 +54,101 @@ using namespace inference; #define DEBUG_TYPE "ConstraintSystem" -ExpressionTimer::ExpressionTimer(AnchorType Anchor, ConstraintSystem &CS, - unsigned thresholdInSecs) - : Anchor(Anchor), Context(CS.getASTContext()), - StartTime(llvm::TimeRecord::getCurrentTime()), - ThresholdInSecs(thresholdInSecs), - PrintDebugTiming(CS.getASTContext().TypeCheckerOpts.DebugTimeExpressions), - PrintWarning(true) {} +void ConstraintSystem::startExpression(ConstraintSystem::ExprOrLocator anchor) { + CurrentAnchor = anchor; + + startExpressionTimer(); +} -SourceRange ExpressionTimer::getAffectedRange() const { +SourceRange ConstraintSystem::getAffectedRange() const { ASTNode anchor; - if (auto *locator = Anchor.dyn_cast()) { + if (auto *locator = CurrentAnchor.dyn_cast()) { anchor = simplifyLocatorToAnchor(locator); // If locator couldn't be simplified down to a single AST // element, let's use its root. if (!anchor) anchor = locator->getAnchor(); } else { - anchor = cast(Anchor); + anchor = cast(CurrentAnchor); } return anchor.getSourceRange(); } +bool ConstraintSystem::isTooComplex(size_t solutionMemory) { + if (isAlreadyTooComplex.first) + return true; + + auto CancellationFlag = getASTContext().CancellationFlag; + if (CancellationFlag && CancellationFlag->load(std::memory_order_relaxed)) + return true; + + auto markTooComplex = [&](SourceRange range, StringRef reason) { + if (isDebugMode()) { + if (solverState) + llvm::errs().indent(solverState->getCurrentIndent()); + llvm::errs() << "(too complex: " << reason << ")\n"; + } + isAlreadyTooComplex = {true, range}; + return true; + }; + + auto used = getASTContext().getSolverMemory() + solutionMemory; + MaxMemory = std::max(used, MaxMemory); + auto threshold = getASTContext().TypeCheckerOpts.SolverMemoryThreshold; + if (MaxMemory > threshold) { + // Bail once we've used too much constraint solver arena memory. + return markTooComplex(getAffectedRange(), "exceeded memory limit"); + } + + if (Timer && Timer->isExpired()) { + // Disable warnings about expressions that go over the warning + // threshold since we're arbitrarily ending evaluation and + // emitting an error. + Timer->disableWarning(); + + return markTooComplex(getAffectedRange(), "exceeded time limit"); + } + + auto &opts = getASTContext().TypeCheckerOpts; + + // Bail out once we've looked at a really large number of choices. + if (opts.SolverScopeThreshold && NumSolverScopes > opts.SolverScopeThreshold) + return markTooComplex(getAffectedRange(), "exceeded scope limit"); + + // Bail out once we've taken a really large number of steps. + if (opts.SolverTrailThreshold && NumTrailSteps > opts.SolverTrailThreshold) + return markTooComplex(getAffectedRange(), "exceeded trail limit"); + + return false; +} + +ExpressionTimer::ExpressionTimer(ConstraintSystem &CS, unsigned thresholdInSecs) + : CS(CS), + StartTime(llvm::TimeRecord::getCurrentTime()), + ThresholdInSecs(thresholdInSecs), + PrintWarning(true) {} + +unsigned ExpressionTimer::getWarnLimit() const { + return CS.getASTContext().TypeCheckerOpts.WarnLongExpressionTypeChecking; +} + ExpressionTimer::~ExpressionTimer() { auto elapsed = getElapsedProcessTimeInFractionalSeconds(); unsigned elapsedMS = static_cast(elapsed * 1000); + auto &ctx = CS.getASTContext(); - if (PrintDebugTiming) { + if (ctx.TypeCheckerOpts.DebugTimeExpressions) { // Round up to the nearest 100th of a millisecond. llvm::errs() << llvm::format("%0.2f", std::ceil(elapsed * 100000) / 100) << "ms\t"; - if (auto *E = Anchor.dyn_cast()) { - E->getLoc().print(llvm::errs(), Context.SourceMgr); + auto anchor = CS.getCurrentAnchor(); + if (auto *E = anchor.dyn_cast()) { + E->getLoc().print(llvm::errs(), ctx.SourceMgr); } else { - auto *locator = cast(Anchor); - locator->dump(&Context.SourceMgr, llvm::errs()); + auto *locator = cast(anchor); + locator->dump(&ctx.SourceMgr, llvm::errs()); } llvm::errs() << "\n"; } @@ -103,10 +161,10 @@ ExpressionTimer::~ExpressionTimer() { if (WarnLimit == 0 || elapsedMS < WarnLimit) return; - auto sourceRange = getAffectedRange(); + auto sourceRange = CS.getAffectedRange(); if (sourceRange.Start.isValid()) { - Context.Diags + ctx.Diags .diagnose(sourceRange.Start, diag::debug_long_expression, elapsedMS, WarnLimit) .highlight(sourceRange); @@ -140,7 +198,7 @@ ConstraintSystem::~ConstraintSystem() { } } -void ConstraintSystem::startExpressionTimer(ExpressionTimer::AnchorType anchor) { +void ConstraintSystem::startExpressionTimer() { ASSERT(!Timer); const auto &opts = getASTContext().TypeCheckerOpts; @@ -156,7 +214,7 @@ void ConstraintSystem::startExpressionTimer(ExpressionTimer::AnchorType anchor) timeout = ExpressionTimer::NoLimit; } - Timer.emplace(anchor, *this, timeout); + Timer.emplace(*this, timeout); } void ConstraintSystem::incrementScopeCounter() { diff --git a/test/Constraints/too_complex_source_location.swift b/test/Constraints/too_complex_source_location.swift new file mode 100644 index 0000000000000..24cf1258f8c86 --- /dev/null +++ b/test/Constraints/too_complex_source_location.swift @@ -0,0 +1,19 @@ +// RUN: %target-typecheck-verify-swift -solver-scope-threshold=10 + +// Note: the scope threshold is intentionally set low so that the expression will fail. +// +// The purpose of the test is to ensure the diagnostic points at the second statement in +// the closure, and not the closure itself. +// +// If the expression becomes very fast and we manage to type check it with fewer than +// 10 scopes, please *do not* remove the expected error! Instead, make the expression +// more complex again. + +let s = "" +let n = 0 + +let closure = { + let _ = 0 + let _ = "" + s + "" + s + "" + s + "" + n + "" // expected-error {{reasonable time}} + let _ = 0 +} diff --git a/validation-test/Sema/SwiftUI/too_complex_source_location.swift b/validation-test/Sema/SwiftUI/too_complex_source_location.swift new file mode 100644 index 0000000000000..470b5dbd61b47 --- /dev/null +++ b/validation-test/Sema/SwiftUI/too_complex_source_location.swift @@ -0,0 +1,43 @@ +// RUN: %target-typecheck-verify-swift -target %target-cpu-apple-macosx10.15 -swift-version 5 +// REQUIRES: objc_interop + +// https://forums.swift.org/t/roadmap-for-improving-the-type-checker/82952/9 +// +// The purpose of the test is to ensure the diagnostic points at the right statement in +// the function body, and not the function declaration itself. +// +// Ideally, we would produce a useful diagnostic here. Once we are able to do that, we +// will need to devise a new test which complains with 'reasonable time' to ensure the +// source location remains correct. + +import SwiftUI + +struct ContentView: View { + @State var selection = "" + + @State var a: Int? + @State var b: Int? + @State var c: Int? + + var body: some View { + ScrollView { + VStack { + Picker(selection: $selection) { + ForEach(["a", "b", "c"], id: \.self) { + Text($0) // expected-error {{reasonable time}} + .foregroundStyl(.red) // Typo is here + } + } label: { + } + .pickerStyle(.segmented) + } + .padding(.horizontal) + } + .onChange(of: a) { oldValue, newValue in + } + .onChange(of: b) { oldValue, newValue in + } + .onChange(of: c) { oldValue, newValue in + } + } +} diff --git a/validation-test/Sema/type_checker_perf/slow/rdar19612086.swift b/validation-test/Sema/type_checker_perf/slow/rdar19612086.swift index 1b85cdf490590..0510537e25013 100644 --- a/validation-test/Sema/type_checker_perf/slow/rdar19612086.swift +++ b/validation-test/Sema/type_checker_perf/slow/rdar19612086.swift @@ -11,8 +11,8 @@ struct rdar19612086 { let x = 1.0 var description : String { - return "\(i)" + Stringly(format: "%.2f", x) + // expected-error {{reasonable time}} - "\(i+1)" + Stringly(format: "%.2f", x) + + return "\(i)" + Stringly(format: "%.2f", x) + + "\(i+1)" + Stringly(format: "%.2f", x) + // expected-error {{reasonable time}} "\(i+2)" + Stringly(format: "%.2f", x) + "\(i+3)" + Stringly(format: "%.2f", x) + "\(i+4)" + Stringly(format: "%.2f", x) +