diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h index 407d9a5afd3f6..7abe57ca066cc 100644 --- a/include/swift/Sema/ConstraintSystem.h +++ b/include/swift/Sema/ConstraintSystem.h @@ -1067,6 +1067,15 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &out, const Score &score); struct Score { unsigned Data[NumScoreKinds] = {}; + /// The maximum score that can be represented. + static Score max() { + Score x; + for (unsigned i = 0; i != NumScoreKinds; ++i) { + x.Data[i] = ~0U; + } + return x; + } + friend Score &operator+=(Score &x, const Score &y) { for (unsigned i = 0; i != NumScoreKinds; ++i) { x.Data[i] += y.Data[i]; diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp index 9f8e339586b2e..be6b895b54e1e 100644 --- a/lib/Sema/CSSolver.cpp +++ b/lib/Sema/CSSolver.cpp @@ -2361,8 +2361,10 @@ void DisjunctionChoiceProducer::partitionDisjunction( return false; }); - // Then unavailable constraints if we're skipping them. - if (!CS.shouldAttemptFixes()) { + // Then unavailable constraints if we're skipping them. Also do this for + // completion since we enable fixes there but still want to try unavailable + // overloads last. + if (!CS.shouldAttemptFixes() || CS.isForCodeCompletion()) { forEachChoice(Choices, [&](unsigned index, Constraint *constraint) -> bool { if (constraint->getKind() != ConstraintKind::BindOverload) return false; diff --git a/lib/Sema/CSStep.cpp b/lib/Sema/CSStep.cpp index a13787aa5227b..8630bbead8dcc 100644 --- a/lib/Sema/CSStep.cpp +++ b/lib/Sema/CSStep.cpp @@ -615,6 +615,18 @@ bool TypeChecker::isDeclRefinementOf(ValueDecl *declA, ValueDecl *declB) { false); } +bool DisjunctionStep::isLastChoiceBetterThan(ScoreKind kind) const { + if (!LastSolvedChoice) + return false; + + auto &lastScore = LastSolvedChoice->second; + for (unsigned i = 0, n = unsigned(kind) + 1; i != n; ++i) { + if (lastScore.Data[i] > 0) + return false; + } + return true; +} + bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const { auto &ctx = CS.getASTContext(); @@ -635,27 +647,21 @@ bool DisjunctionStep::shouldSkip(const DisjunctionChoice &choice) const { if (choice.isDisabled()) return skip("disabled"); - if (!CS.shouldAttemptFixes()) { + if (!CS.shouldAttemptFixes() || CS.isForCodeCompletion()) { // Skip unavailable overloads. - if (choice.isUnavailable()) - return skip("unavailable"); + if (choice.isUnavailable()) { + // For code completion, we only want to do this if we've already seen + // a better choice. + if (!CS.isForCodeCompletion() || isLastChoiceBetterThan(SK_Unavailable)) + return skip("unavailable"); + } // Since the disfavored overloads are always located at the end of // the partition they could be skipped if there was at least one // valid solution for this partition already, because the solution // they produce would always be worse. - if (choice.isDisfavored() && LastSolvedChoice) { - bool canSkipDisfavored = true; - auto &lastScore = LastSolvedChoice->second; - for (unsigned i = 0, n = unsigned(SK_DisfavoredOverload) + 1; i != n; - ++i) { - if (lastScore.Data[i] > 0) { - canSkipDisfavored = false; - break; - } - } - - if (canSkipDisfavored) + if (choice.isDisfavored()) { + if (isLastChoiceBetterThan(SK_DisfavoredOverload)) return skip("disfavored"); } } @@ -799,9 +805,24 @@ bool DisjunctionStep::attempt(const DisjunctionChoice &choice) { bool ConjunctionStep::attempt(const ConjunctionElement &element) { ++CS.solverState->NumConjunctionTerms; - // Outside or previous element score doesn't affect - // subsequent elements. - CS.solverState->BestScore.reset(); + // Outside or previous element score doesn't affect subsequent elements + // subsequent elements. However SK_Fix and SK_Hole *are* propagated outside + // of the conjunction, so we can use this to inform the best score within + // the conjunction. + if (BestScore) { + auto bestScore = Score::max(); + bestScore.Data[SK_Fix] = BestScore->Data[SK_Fix]; + bestScore.Data[SK_Hole] = BestScore->Data[SK_Hole]; + CS.solverState->BestScore = bestScore; + } else { + CS.solverState->BestScore.reset(); + } + + // Make sure that element is solved in isolation + // by dropping all non-fatal scoring information. + CS.clearScore(); + CS.increaseScore(SK_Fix, PreviousScore.Data[SK_Fix]); + CS.increaseScore(SK_Hole, PreviousScore.Data[SK_Hole]); // Apply solution inferred for all the previous elements // because this element could reference declarations @@ -811,14 +832,12 @@ bool ConjunctionStep::attempt(const ConjunctionElement &element) { // Note that solution is removed here. This is done // because we want build a single complete solution // incrementally. - CS.replaySolution(Solutions.pop_back_val(), - /*shouldIncrementScore=*/false); + auto S = Solutions.pop_back_val(); + CS.replaySolution(S, /*shouldIncrementScore=*/false); + CS.increaseScore(SK_Fix, S.getFixedScore().Data[SK_Fix] - PreviousScore.Data[SK_Fix]); + CS.increaseScore(SK_Hole, S.getFixedScore().Data[SK_Hole] - PreviousScore.Data[SK_Hole]); } - // Make sure that element is solved in isolation - // by dropping all scoring information. - CS.clearScore(); - // Reset the scope and trail counters to avoid "too complex" // failures when closure has a lot of elements in the body. CS.NumSolverScopes = 0; @@ -902,8 +921,10 @@ StepResult ConjunctionStep::resume(bool prevFailed) { if (Solutions.size() == 1) { auto score = Solutions.front().getFixedScore(); - if (score.Data[SK_Fix] > 0 && !CS.isForCodeCompletion()) + if (score.Data[SK_Fix] > PreviousScore.Data[SK_Fix] && + !CS.isForCodeCompletion()) { Producer.markExhausted(); + } } } else if (Solutions.size() != 1) { return failConjunction(); @@ -942,6 +963,12 @@ StepResult ConjunctionStep::resume(bool prevFailed) { Snapshot && "Isolated conjunction requires a snapshot of the constraint system"); + // Subtract fatal score kinds. + for (auto &solution : Solutions) { + solution.getFixedScore().Data[SK_Fix] -= PreviousScore.Data[SK_Fix]; + solution.getFixedScore().Data[SK_Hole] -= PreviousScore.Data[SK_Hole]; + } + // In diagnostic mode it's valid for an element to have // multiple solutions. Ambiguity just needs to be merged // into the outer context to be property diagnosed. diff --git a/lib/Sema/CSStep.h b/lib/Sema/CSStep.h index 906fda9895050..dfd319379639e 100644 --- a/lib/Sema/CSStep.h +++ b/lib/Sema/CSStep.h @@ -662,6 +662,11 @@ class DisjunctionStep final : public BindingStep { bool shortCircuitDisjunctionAt(Constraint *currentChoice, Constraint *lastSuccessfulChoice) const; + /// Whether the last choice has a better score than a given score kind, i.e + /// any non-zero components of its score are strictly less impactful than the + /// given kind. + bool isLastChoiceBetterThan(ScoreKind kind) const; + bool shouldSkipGenericOperators() const { if (!BestNonGenericScore) return false; @@ -858,6 +863,8 @@ class ConjunctionStep : public BindingStep { void replaySolution(const Solution &solution); }; + Score PreviousScore; + /// Best solution solver reached so far. std::optional BestScore; @@ -901,7 +908,7 @@ class ConjunctionStep : public BindingStep { SmallVectorImpl &solutions) : BindingStep(cs, {cs, conjunction}, conjunction->isIsolated() ? IsolatedSolutions : solutions), - BestScore(getBestScore()), + PreviousScore(cs.CurrentScore), BestScore(getBestScore()), OuterNumSolverScopes(cs.NumSolverScopes, 0), OuterNumTrailSteps(cs.NumTrailSteps, 0), Conjunction(conjunction), diff --git a/test/IDE/complete_closure_ambiguity.swift b/test/IDE/complete_closure_ambiguity.swift index a1daa7be26079..2961a1cbcef35 100644 --- a/test/IDE/complete_closure_ambiguity.swift +++ b/test/IDE/complete_closure_ambiguity.swift @@ -24,8 +24,7 @@ foo { return x } -// CONSTRAINTS: attempting disjunction choice {{.*}}:12:6 -// CONSTRAINTS: increasing 'disfavored overload' score -// CONSTRAINTS: solution is worse than the best solution +// CONSTRAINTS: attempting disjunction choice {{.*}}:9:6 +// CONSTRAINTS: skipping disfavored disjunction choice {{.*}}:12:6 // CONSTRAINTS-NOT-NOT: increasing 'hole' diff --git a/test/IDE/complete_in_closures.swift b/test/IDE/complete_in_closures.swift index 5da2b98c0c8f6..8bbe3ad1dc893 100644 --- a/test/IDE/complete_in_closures.swift +++ b/test/IDE/complete_in_closures.swift @@ -543,8 +543,8 @@ func testCompleteAfterClosureWithIfExprThatContainErrors() { }#^AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS^# // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS: Begin completions, 2 items - // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Keyword[self]/CurrNominal: .self[#() -> _#]; name=self - // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Pattern/CurrModule/Flair[ArgLabels]: ()[#_#]; name=() + // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Keyword[self]/CurrNominal: .self[#() -> Void#]; name=self + // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS-DAG: Pattern/CurrModule/Flair[ArgLabels]: ()[#Void#]; name=() // AFTER_CLOSURE_WITH_IF_EXPR_THAT_CONTAIN_ERRORS: End completions } diff --git a/test/IDE/complete_unavailable_skip.swift b/test/IDE/complete_unavailable_skip.swift new file mode 100644 index 0000000000000..58350ef45d0cd --- /dev/null +++ b/test/IDE/complete_unavailable_skip.swift @@ -0,0 +1,33 @@ +// RUN: %empty-directory(%t) +// RUN: %batch-code-completion -debug-constraints 2> %t/constraints.log +// RUN: %FileCheck %s -check-prefix CONSTRAINTS < %t/constraints.log + +struct S { + func bar(_ x: Int) {} + func bar(_ x: String) {} +} + +@available(*, unavailable) +func foo(_ fn: (S) -> Void) {} +func foo(_ x: () -> Void) {} + +// Make sure we can try the unavailable overload if necessary. +foo { + $0.#^COMPLETE1^# + // COMPLETE1: Decl[InstanceMethod]/CurrNominal: bar({#(x): Int#})[#Void#]; name=bar(:) + // COMPLETE1: Decl[InstanceMethod]/CurrNominal: bar({#(x): String#})[#Void#]; name=bar(:) +} + +func baz(_ fn: (S) -> Void) {} + +@available(*, unavailable) +func baz(_ x: () -> Void) {} + +// Here we can prune the unavailable overload of baz. +baz { + $0.#^COMPLETE2^# + // COMPLETE2: Decl[InstanceMethod]/CurrNominal: bar({#(x): Int#})[#Void#]; name=bar(:) + // COMPLETE2: Decl[InstanceMethod]/CurrNominal: bar({#(x): String#})[#Void#]; name=bar(:) +} +// CONSTRAINTS: attempting disjunction choice {{.*}}:21:6 +// CONSTRAINTS: skipping unavailable disjunction choice {{.*}}:24:6 diff --git a/validation-test/compiler_crashers_2/2d5e95ea862946ed.swift b/validation-test/compiler_crashers_2_fixed/2d5e95ea862946ed.swift similarity index 80% rename from validation-test/compiler_crashers_2/2d5e95ea862946ed.swift rename to validation-test/compiler_crashers_2_fixed/2d5e95ea862946ed.swift index f5857b88ddd50..cf239d1280ded 100644 --- a/validation-test/compiler_crashers_2/2d5e95ea862946ed.swift +++ b/validation-test/compiler_crashers_2_fixed/2d5e95ea862946ed.swift @@ -1,5 +1,5 @@ // {"kind":"typecheck","signature":"(anonymous namespace)::ExprWalker::walkToExprPost(swift::Expr*)","signatureAssert":"Assertion failed: (Ptr && \"Cannot dereference a null Type!\"), function operator->"} -// RUN: not --crash %target-swift-frontend -typecheck %s +// RUN: not %target-swift-frontend -typecheck %s func a { { \ b() a