Skip to content

Sema: Fix source location bookkeeping for 'reasonable time' diagnostic #85293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sema: Fix source location bookkeeping for 'reasonable time' diagnostic #85293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 25 additions & 65 deletions include/swift/Sema/ConstraintSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,36 +236,23 @@ struct RestrictionOrFix {


class ExpressionTimer {
public:
using AnchorType = llvm::PointerUnion<Expr *, ConstraintLocator *>;

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)
Expand Down Expand Up @@ -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<Expr *, ConstraintLocator *>;

private:
ASTContext &Context;
ExprOrLocator CurrentAnchor;
Copy link
Contributor

@xedin xedin Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this to SolverState instead just like we do with indent?


public:
DeclContext *DC;
Expand Down Expand Up @@ -5394,6 +5386,9 @@ class ConstraintSystem {
/// \returns The selected conjunction.
Constraint *selectConjunction();

void diagnoseTooComplex(SourceLoc fallbackLoc,
SolutionResult &result);
Copy link
Contributor

@xedin xedin Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are willing to work on this I think we should move all this logic into steps instead of the constraint system. In general I think constraint system should have to deal with anything but constraints. The context of "too complex" belongs to the solver itself IMHO, the algorithm that drives the constraint processing.


/// Solve the system of constraints generated from provided expression.
///
/// \param target The target to generate constraints from.
Expand Down Expand Up @@ -5491,6 +5486,8 @@ class ConstraintSystem {
compareSolutions(ConstraintSystem &cs, ArrayRef<Solution> 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.
Expand Down Expand Up @@ -5528,7 +5525,6 @@ class ConstraintSystem {
std::optional<unsigned> findBestSolution(SmallVectorImpl<Solution> &solutions,
bool minimize);

public:
/// Apply a given solution to the target, producing a fully
/// type-checked target or \c None if an error occurred.
///
Expand Down Expand Up @@ -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.
Expand All @@ -5594,53 +5600,7 @@ class ConstraintSystem {
return range.isValid() ? range : std::optional<SourceRange>();
}

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<Solution> solutions) {
if (isAlreadyTooComplex.first)
Expand Down
4 changes: 1 addition & 3 deletions lib/Sema/BuilderTransform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
36 changes: 17 additions & 19 deletions lib/Sema/CSSolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/CSStep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
12 changes: 4 additions & 8 deletions lib/Sema/CSStep.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,8 +867,7 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {

/// The number of milliseconds until outer constraint system
/// is considered "too complex" if timer is enabled.
std::optional<std::pair<ExpressionTimer::AnchorType, unsigned>>
OuterTimeRemaining = std::nullopt;
std::optional<unsigned> OuterTimeRemaining = std::nullopt;

/// Conjunction constraint associated with this step.
Constraint *Conjunction;
Expand Down Expand Up @@ -910,7 +909,7 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {

if (cs.Timer) {
auto remainingTime = cs.Timer->getRemainingProcessTimeInSeconds();
OuterTimeRemaining.emplace(cs.Timer->getAnchor(), remainingTime);
OuterTimeRemaining.emplace(remainingTime);
}
}

Expand All @@ -925,11 +924,8 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
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;
Expand Down
Loading