From 6a603c7e4228fac20b0f7fcb2ff8fb6d67dc15a3 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 19 May 2023 13:36:24 -0700 Subject: [PATCH 1/5] [CSSolver] Split ambiguity handling for diagnostics and code completion --- lib/Sema/CSSolver.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp index 2f6d0c5f0100d..edb05f334f585 100644 --- a/lib/Sema/CSSolver.cpp +++ b/lib/Sema/CSSolver.cpp @@ -1476,18 +1476,23 @@ ConstraintSystem::solve(SyntacticElementTarget &target, return std::nullopt; } - case SolutionResult::Ambiguous: - // If salvaging produced an ambiguous result, it has already been - // diagnosed. + case SolutionResult::Ambiguous: { // If we have found an ambiguous solution in the first stage, salvaging // won't produce more solutions, so we can inform the solution callback // about the current ambiguous solutions straight away. - if (stage == 1 || Context.SolutionCallback) { + if (Context.SolutionCallback) { reportSolutionsToSolutionCallback(solution); solution.markAsDiagnosed(); return std::nullopt; } + // If salvaging produced an ambiguous result, it has already been + // diagnosed. + if (stage == 1) { + solution.markAsDiagnosed(); + return std::nullopt; + } + if (Options.contains( ConstraintSystemFlags::AllowUnresolvedTypeVariables)) { dumpSolutions(solution); @@ -1499,6 +1504,7 @@ ConstraintSystem::solve(SyntacticElementTarget &target, } LLVM_FALLTHROUGH; + } case SolutionResult::UndiagnosedError: if (stage == 1) { From 151d3b519a279027db0d520b45c0d82af24b53f6 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 19 May 2023 15:41:58 -0700 Subject: [PATCH 2/5] [Diagnostics] Point ambiguity diagnostic to key path component instead of parent expr --- lib/Sema/ConstraintSystem.cpp | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp index 95784054690d7..a4e23e8a5bf91 100644 --- a/lib/Sema/ConstraintSystem.cpp +++ b/lib/Sema/ConstraintSystem.cpp @@ -3347,28 +3347,37 @@ bool ConstraintSystem::diagnoseAmbiguity(ArrayRef solutions) { // FIXME: We would prefer to emit the name as written, but that information // is not sufficiently centralized in the AST. DeclNameRef name(getOverloadChoiceName(overload.choices)); + + // A location to attach ambiguity diagnostic to. + SourceLoc diagLoc; + + // Some of the locations do not simplify all the way to anchor, + // for example - key path components and dynamic member references + // are not represented via ASTNode, auto anchor = simplifyLocatorToAnchor(overload.locator); - if (!anchor) { - // It's not clear that this is actually valid. Just use the overload's - // anchor for release builds, but assert so we can properly diagnose - // this case if it happens to be hit. Note that the overload will - // *always* be anchored, otherwise everything would be broken, ie. this - // assertion would be the least of our worries. - anchor = overload.locator->getAnchor(); - assert(false && "locator could not be simplified to anchor"); + if (anchor) { + diagLoc = getLoc(anchor); + } else if (auto keyPathComponent = overload.locator->getFirstElementAs< + LocatorPathElt::KeyPathComponent>()) { + auto *KPE = castToExpr(overload.locator->getAnchor()); + diagLoc = KPE->getComponents()[keyPathComponent->getIndex()].getLoc(); + } else { + diagLoc = getLoc(overload.locator->getAnchor()); } // Emit the ambiguity diagnostic. auto &DE = getASTContext().Diags; - DE.diagnose(getLoc(anchor), + DE.diagnose(diagLoc, name.isOperator() ? diag::ambiguous_operator_ref : diag::ambiguous_decl_ref, name); - TrailingClosureAmbiguityFailure failure(solutions, anchor, - overload.choices); - if (failure.diagnoseAsNote()) - return true; + if (anchor) { + TrailingClosureAmbiguityFailure failure(solutions, anchor, + overload.choices); + if (failure.diagnoseAsNote()) + return true; + } // Emit candidates. Use a SmallPtrSet to make sure only emit a particular // candidate once. FIXME: Why is one candidate getting into the overload From 0dbd39f35280498a523763297da19de4733ad75e Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 19 May 2023 15:45:23 -0700 Subject: [PATCH 3/5] [CSSolver] Don't re-run solver to diagnose ambiguities If ambiguity has been detected during normal solving, let's diagnose that right away instead of re-running the solver in diagnostic mode because it would produce exactly the same set of solutions (all solutions with fixes added in diagnostic mode would be filtered out because they'd have worse scores). --- lib/Sema/CSSolver.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp index edb05f334f585..efe819578da19 100644 --- a/lib/Sema/CSSolver.cpp +++ b/lib/Sema/CSSolver.cpp @@ -1503,6 +1503,23 @@ ConstraintSystem::solve(SyntacticElementTarget &target, return std::move(result); } + auto solutionsRef = std::move(solution).takeAmbiguousSolutions(); + SmallVector ambiguity( + std::make_move_iterator(solutionsRef.begin()), + std::make_move_iterator(solutionsRef.end())); + + { + SolverState state(*this, FreeTypeVariableBinding::Disallow); + + // Constraint generator is allowed to introduce fixes to the + // contraint system. + if (diagnoseAmbiguityWithFixes(ambiguity) || + diagnoseAmbiguity(ambiguity)) { + solution.markAsDiagnosed(); + return std::nullopt; + } + } + LLVM_FALLTHROUGH; } @@ -1590,7 +1607,7 @@ bool ConstraintSystem::solve(SmallVectorImpl &solutions, // Filter deduced solutions, try to figure out if there is // a single best solution to use, if not explicitly disabled // by constraint system options. - filterSolutions(solutions); + filterSolutions(solutions, /*minimize=*/true); // We fail if there is no solution or the expression was too complex. return solutions.empty() || isTooComplex(solutions); From f81ac999bbec92d449081cb91317cc886458df76 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 23 Jul 2025 00:49:51 -0700 Subject: [PATCH 4/5] [Tests] NFC: Adjust tests improved by the changes to ambiguity diagnostics --- test/expr/postfix/init/unqualified.swift | 14 +++++++------- .../45e1131d17a7561c.swift | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) rename validation-test/{compiler_crashers_2 => compiler_crashers_2_fixed}/45e1131d17a7561c.swift (82%) diff --git a/test/expr/postfix/init/unqualified.swift b/test/expr/postfix/init/unqualified.swift index bb8372378d2c2..dd1ebbbe55c71 100644 --- a/test/expr/postfix/init/unqualified.swift +++ b/test/expr/postfix/init/unqualified.swift @@ -3,15 +3,15 @@ // https://github.com/apple/swift/issues/43464 class Aaron { - init(x: Int) { + init(x: Int) { // expected-note {{found this candidate}} func foo() { // Make sure we recover and assume 'self.init'. // expected-error@+2 {{initializer expression requires explicit access; did you mean to prepend it with 'self.'?}} {{11-11=self.}} - // expected-error@+1 {{type of expression is ambiguous without a type annotation}} + // expected-error@+1 {{ambiguous use of 'init'}} _ = init } } - convenience init() { + convenience init() { // expected-note {{found this candidate}} // Make sure we recover and assume 'self.init'. // expected-error@+2 {{initializer expression requires explicit access; did you mean to prepend it with 'self.'?}} {{5-5=self.}} // expected-error@+1 {{cannot convert value of type 'Bool' to expected argument type 'Int'}} @@ -23,7 +23,7 @@ class Aaron { func foo() { _ = init() } } - required init(y: Int) {} + required init(y: Int) {} // expected-note {{found this candidate}} static func aaronFn() { // Make sure we recover and assume 'self.init'. @@ -40,7 +40,7 @@ class Aaron { } class Theodosia: Aaron { - init() { + init() { // expected-note {{found this candidate}} // Make sure we recover and assume 'super.init'. // expected-error@+2 {{initializer expression requires explicit access; did you mean to prepend it with 'super.'?}} {{5-5=super.}} // expected-error@+1 {{cannot convert value of type 'Bool' to expected argument type 'Int'}} @@ -48,11 +48,11 @@ class Theodosia: Aaron { // Make sure we recover and assume 'self.init'. // expected-error@+2 {{initializer expression requires explicit access; did you mean to prepend it with 'self.'?}} {{22-22=self.}} - // expected-error@+1 {{type of expression is ambiguous without a type annotation}} + // expected-error@+1 {{ambiguous use of 'init'}} func foo() { _ = init } } - required init(y: Int) {} + required init(y: Int) {} // expected-note {{found this candidate}} static func theodosiaFn() { // Make sure we recover and assume 'self.init'. diff --git a/validation-test/compiler_crashers_2/45e1131d17a7561c.swift b/validation-test/compiler_crashers_2_fixed/45e1131d17a7561c.swift similarity index 82% rename from validation-test/compiler_crashers_2/45e1131d17a7561c.swift rename to validation-test/compiler_crashers_2_fixed/45e1131d17a7561c.swift index 0eeda0a5946a5..e20a9c7b98826 100644 --- a/validation-test/compiler_crashers_2/45e1131d17a7561c.swift +++ b/validation-test/compiler_crashers_2_fixed/45e1131d17a7561c.swift @@ -1,5 +1,5 @@ // {"signature":"swift::constraints::simplifyLocator(swift::ASTNode&, llvm::ArrayRef&, swift::SourceRange&)"} -// RUN: not --crash %target-swift-frontend -typecheck %s +// RUN: not %target-swift-frontend -typecheck %s struct a { eq = "" b "Self ecuador1 Self) > Bool { let getProperties = ( \.eq as Self -> _ From 032ed865cf0e56bcc18b307903500cafdc4d8bac Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 30 Jul 2025 21:30:25 -0700 Subject: [PATCH 5/5] [CSBindings] Unwrap optional type binding for a result of leading-dot syntax Using only optional type as a result of leading-dot syntax could result in a solution that implicit unwraps the base. Instead let's record the unwrapped type as well (if it's resolved enough) and allow the solver to decide on a better solution. --- lib/Sema/CSBindings.cpp | 55 +++++++++++++++++++++++------------ lib/Sema/CSStep.h | 2 ++ test/decl/enum/enumtest.swift | 10 ++----- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/lib/Sema/CSBindings.cpp b/lib/Sema/CSBindings.cpp index 54ad8b4a191d6..9bfa1a4dec8c3 100644 --- a/lib/Sema/CSBindings.cpp +++ b/lib/Sema/CSBindings.cpp @@ -794,25 +794,42 @@ bool BindingSet::finalize(bool transitive) { return true; } - if (CS.shouldAttemptFixes() && - locator->isLastElement()) { - // Let's see whether this chain is valid, if it isn't then to avoid - // diagnosing the same issue multiple different ways, let's infer - // result of the chain to be a hole. - auto *resultExpr = - castToExpr(locator->getAnchor()); - auto *baseLocator = CS.getConstraintLocator( - resultExpr->getChainBase(), ConstraintLocator::UnresolvedMember); - - if (CS.hasFixFor( - baseLocator, - FixKind::AllowInvalidStaticMemberRefOnProtocolMetatype)) { - CS.recordPotentialHole(TypeVar); - // Clear all of the previously collected bindings which are inferred - // from inside of a member chain. - Bindings.remove_if([](const PotentialBinding &binding) { - return binding.Kind == AllowedBindingKind::Supertypes; - }); + if (locator->isLastElement()) { + if (CS.shouldAttemptFixes()) { + // Let's see whether this chain is valid, if it isn't then to avoid + // diagnosing the same issue multiple different ways, let's infer + // result of the chain to be a hole. + auto *resultExpr = + castToExpr(locator->getAnchor()); + auto *baseLocator = CS.getConstraintLocator( + resultExpr->getChainBase(), ConstraintLocator::UnresolvedMember); + + if (CS.hasFixFor( + baseLocator, + FixKind::AllowInvalidStaticMemberRefOnProtocolMetatype)) { + CS.recordPotentialHole(TypeVar); + // Clear all of the previously collected bindings which are inferred + // from inside of a member chain. + Bindings.remove_if([](const PotentialBinding &binding) { + return binding.Kind == AllowedBindingKind::Supertypes; + }); + } + } + + // Using only optional type as a result of leading-dot syntax could + // result in a solution that implicit unwraps the base. Instead let's + // record the unwrapped type as well (if it's resolved enough) and + // allow the solver to decide on a better solution. + for (const auto &binding : Bindings) { + if (binding.Kind != AllowedBindingKind::Subtypes) + continue; + + if (auto objType = binding.BindingType->getOptionalObjectType()) { + if (objType->isTypeVariableOrMember()) + continue; + + addBinding(binding.withType(objType), /*isTransitive=*/false); + } } } } diff --git a/lib/Sema/CSStep.h b/lib/Sema/CSStep.h index 906fda9895050..396d4d0e75074 100644 --- a/lib/Sema/CSStep.h +++ b/lib/Sema/CSStep.h @@ -597,6 +597,8 @@ class TypeVariableStep final : public BindingStep { if (CS.shouldAttemptFixes()) return false; + // Check for SK_UnresolvedMemberViaOptional + // If there has been at least one solution so far // at a current batch of bindings is done it's a // success because each new batch would be less diff --git a/test/decl/enum/enumtest.swift b/test/decl/enum/enumtest.swift index 9cd20c410568a..d1948dc5000c1 100644 --- a/test/decl/enum/enumtest.swift +++ b/test/decl/enum/enumtest.swift @@ -514,10 +514,7 @@ let _: GenericEnumWithStaticNone? = .none // expected-warning {{assuming yo // expected-note@-1 {{explicitly specify 'Optional' to silence this warning}}{{42-42=Optional}} // expected-note@-2 {{use 'GenericEnumWithStaticNone.none' instead}}{{42-42=GenericEnumWithStaticNone}} let _: GenericEnumWithStaticNone? = .none // Okay - -let _: GenericEnumWithStaticNone? = .none // expected-warning {{assuming you mean 'GenericEnumWithStaticNone.none'; did you mean 'Optional>.none' instead?}} -// expected-note@-1 {{use 'Optional>.none' instead}} {{37-37=Optional>}} -// expected-note@-2 {{use 'GenericEnumWithStaticNone.none' instead}} {{37-37=GenericEnumWithStaticNone}} +let _: GenericEnumWithStaticNone? = .none // Okay enum GenericStructWithStaticNone { init() {} @@ -528,10 +525,7 @@ let _: GenericStructWithStaticNone? = .none // expected-warning {{assuming // expected-note@-1 {{explicitly specify 'Optional' to silence this warning}}{{44-44=Optional}} // expected-note@-2 {{use 'GenericStructWithStaticNone.none' instead}}{{44-44=GenericStructWithStaticNone}} let _: GenericStructWithStaticNone? = .none // Okay - -let _: GenericStructWithStaticNone? = .none // expected-warning {{assuming you mean 'GenericStructWithStaticNone.none'; did you mean 'Optional>.none' instead?}} -// expected-note@-1 {{use 'Optional>.none' instead}} {{39-39=Optional>}} -// expected-note@-2 {{use 'GenericStructWithStaticNone.none' instead}} {{39-39=GenericStructWithStaticNone}} +let _: GenericStructWithStaticNone? = .none // Okay enum GenericEnumWithoutNone { case a