Skip to content

[CSBindings] Store the originator type variable in a transitive binding #86072

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

Merged
xedin merged 1 commit into from
Dec 16, 2025
+58 −34
Merged

[CSBindings] Store the originator type variable in a transitive binding #86072

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
37 changes: 24 additions & 13 deletions include/swift/Sema/CSBindings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,20 @@ struct PotentialBinding {
/// because they are synthetic, they have a locator instead.
PointerUnion<Constraint *, ConstraintLocator *> BindingSource;

/// When the binding is transferred through a subtype chain, this
/// marks a type variable for which it was originally inferred.
TypeVariableType *Originator;

PotentialBinding(Type type, AllowedBindingKind kind,
PointerUnion<Constraint *, ConstraintLocator *> source)
: BindingType(type), Kind(kind), BindingSource(source) {}
PointerUnion<Constraint *, ConstraintLocator *> source,
TypeVariableType *originator)
: BindingType(type), Kind(kind), BindingSource(source),
Originator(originator) {}

PotentialBinding(Type type, AllowedBindingKind kind, Constraint *source)
: PotentialBinding(
type, kind,
PointerUnion<Constraint *, ConstraintLocator *>(source)) {}
type, kind, PointerUnion<Constraint *, ConstraintLocator *>(source),
/*originator=*/nullptr) {}

bool isDefaultableBinding() const {
if (auto *constraint = BindingSource.dyn_cast<Constraint *>())
Expand Down Expand Up @@ -124,13 +130,20 @@ struct PotentialBinding {
Constraint *getSource() const { return cast<Constraint *>(BindingSource); }

PotentialBinding withType(Type type) const {
return {type, Kind, BindingSource};
return {type, Kind, BindingSource, Originator};
}

PotentialBinding withSameSource(Type type, AllowedBindingKind kind) const {
return {type, kind, BindingSource};
return {type, kind, BindingSource, Originator};
}

PotentialBinding asTransitiveFrom(TypeVariableType *originator) const {
ASSERT(originator);
return {BindingType, Kind, BindingSource, originator};
}

bool isTransitive() const { return bool(Originator); }

/// Determine whether this binding could be a viable candidate
/// to be "joined" with some other binding. It has to be at least
/// a non-default r-value supertype binding with no type variables.
Expand All @@ -140,12 +153,13 @@ struct PotentialBinding {
ConstraintLocator *locator) {
return {PlaceholderType::get(typeVar->getASTContext(), typeVar),
AllowedBindingKind::Exact,
/*source=*/locator};
/*source=*/locator, /*originator=*/nullptr};
}

static PotentialBinding forPlaceholder(Type placeholderTy) {
return {placeholderTy, AllowedBindingKind::Exact,
PointerUnion<Constraint *, ConstraintLocator *>()};
PointerUnion<Constraint *, ConstraintLocator *>(),
/*originator=*/nullptr};
}

void print(llvm::raw_ostream &out, const PrintOptions &PO) const;
Expand Down Expand Up @@ -473,7 +487,7 @@ class BindingSet {
/// \param isTransitive Indicates whether this binding has been
/// acquired through transitive inference and requires extra
/// checking.
bool isViable(PotentialBinding &binding, bool isTransitive);
bool isViable(PotentialBinding &binding);

/// Determine whether this set has any "viable" (or non-hole) bindings.
///
Expand Down Expand Up @@ -602,10 +616,7 @@ class BindingSet {
/// Add a new binding to the set.
///
/// \param binding The binding to add.
/// \param isTransitive Indicates whether this binding has been
/// acquired through transitive inference and requires validity
/// checking.
void addBinding(PotentialBinding binding, bool isTransitive);
void addBinding(PotentialBinding binding);

void addLiteralRequirement(Constraint *literal);

Expand Down
1 change: 1 addition & 0 deletions include/swift/Sema/CSTrail.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class SolverTrail {
/// of a PotentialBinding.
Type BindingType;
PointerUnion<Constraint *, ConstraintLocator *> BindingSource;
TypeVariableType *Originator;
} Binding;

ConstraintFix *TheFix;
Expand Down
48 changes: 30 additions & 18 deletions lib/Sema/CSBindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ BindingSet::BindingSet(ConstraintSystem &CS, TypeVariableType *TypeVar,
: CS(CS), TypeVar(TypeVar), Info(info) {

for (const auto &binding : info.Bindings)
addBinding(binding, /*isTransitive=*/false);
addBinding(binding);

for (auto *constraint : info.Constraints) {
switch (constraint->getKind()) {
Expand Down Expand Up @@ -596,7 +596,7 @@ void BindingSet::inferTransitiveKeyPathBindings() {

// Copy the bindings over to the root.
for (const auto &binding : bindings.Bindings)
addBinding(binding, /*isTransitive=*/true);
addBinding(binding.asTransitiveFrom(contextualRootVar));

// Make a note that the key path root is transitively adjacent
// to contextual root type variable and all of its variables.
Expand All @@ -606,9 +606,9 @@ void BindingSet::inferTransitiveKeyPathBindings() {
bindings.AdjacentVars.end());
}
} else {
addBinding(
binding.withSameSource(inferredRootTy, AllowedBindingKind::Exact),
/*isTransitive=*/true);
auto newBinding = binding.withSameSource(
inferredRootTy, AllowedBindingKind::Exact);
addBinding(newBinding.asTransitiveFrom(keyPathTy));
}
}
}
Expand Down Expand Up @@ -679,8 +679,9 @@ void BindingSet::inferTransitiveSupertypeBindings() {
if (ConstraintSystem::typeVarOccursInType(TypeVar, type))
continue;

addBinding(binding.withSameSource(type, AllowedBindingKind::Supertypes),
/*isTransitive=*/true);
auto newBinding =
binding.withSameSource(type, AllowedBindingKind::Supertypes);
addBinding(newBinding.asTransitiveFrom(entry.first));
}
}
}
Expand Down Expand Up @@ -713,8 +714,7 @@ void BindingSet::inferTransitiveUnresolvedMemberRefBindings() {
continue;
}

addBinding({protocolTy, AllowedBindingKind::Exact, constraint},
/*isTransitive=*/false);
addBinding({protocolTy, AllowedBindingKind::Exact, constraint});
}
}
}
Expand Down Expand Up @@ -829,8 +829,8 @@ bool BindingSet::finalizeKeyPathBindings() {
// better diagnostics.
auto keyPathTy = getKeyPathType(ctx, *capability, rootTy,
CS.getKeyPathValueType(keyPath));
updatedBindings.insert(
{keyPathTy, AllowedBindingKind::Exact, locator});
updatedBindings.insert({keyPathTy, AllowedBindingKind::Exact, locator,
/*originator=*/nullptr});
} else if (CS.shouldAttemptFixes()) {
auto fixedRootTy = CS.getFixedType(rootTy);
// If key path is structurally correct and has a resolved root
Expand Down Expand Up @@ -889,11 +889,11 @@ void BindingSet::finalizeUnresolvedMemberChainResult() {
}
}

void BindingSet::addBinding(PotentialBinding binding, bool isTransitive) {
void BindingSet::addBinding(PotentialBinding binding) {
if (Bindings.count(binding))
return;

if (!isViable(binding, isTransitive))
if (!isViable(binding))
return;

SmallPtrSet<TypeVariableType *, 4> referencedTypeVars;
Expand Down Expand Up @@ -957,14 +957,26 @@ void BindingSet::addBinding(PotentialBinding binding, bool isTransitive) {
for (auto existingBinding = Bindings.begin();
existingBinding != Bindings.end();) {
if (existingBinding->isViableForJoin()) {
auto join =
auto joinType =
Type::join(existingBinding->BindingType, binding.BindingType);

if (join && isAcceptableJoin(*join)) {
if (joinType && isAcceptableJoin(*joinType)) {
// Result of the join has to use new binding because it refers
// to the constraint that triggered the join that replaced the
// existing binding.
joined.push_back(binding.withType(*join));
//
// For "join" to be transitive, both bindings have to be as
// well, otherwise we consider it a refinement of a direct
// binding.
auto *origintor =
binding.isTransitive() && existingBinding->isTransitive()
? binding.Originator
: nullptr;

PotentialBinding join(*joinType, binding.Kind, binding.BindingSource,
origintor);

joined.push_back(join);
// Remove existing binding from the set.
// It has to be re-introduced later, since its type has been changed.
existingBinding = Bindings.erase(existingBinding);
Expand Down Expand Up @@ -1448,15 +1460,15 @@ static bool hasConversions(Type type) {
type->is<BuiltinType>() || type->is<ArchetypeType>());
}

bool BindingSet::isViable(PotentialBinding &binding, bool isTransitive) {
bool BindingSet::isViable(PotentialBinding &binding) {
// Prevent against checking against the same opened nominal type
// over and over again. Doing so means redundant work in the best
// case. In the worst case, we'll produce lots of duplicate solutions
// for this constraint system, which is problematic for overload
// resolution.
auto type = binding.BindingType;

if (isTransitive && !checkTypeOfBinding(TypeVar, type))
if (binding.isTransitive() && !checkTypeOfBinding(TypeVar, type))
Copy link
Contributor

@slavapestov slavapestov Dec 15, 2025

Choose a reason for hiding this comment

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

I know this isn't the focus of this PR, but checkTypeOfBinding() seems to be generally applicable and not just in the case of transitive bindings, for example there's an occurs check. Why do we only do this check for transitive bindings? Are they always true by construction for non-transitive bindings? Should we ASSERT(checkTypeOfBinding(TypeVar, type)) then?

Copy link
Contributor Author

@xedin xedin Dec 15, 2025

Choose a reason for hiding this comment

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

checkTypeOfBinding for a direct binding happens as part of inferFromRelational, here we double-check that it's indeed okay to transfer because it didn't get checked for the current type variable.

Copy link
Contributor

@slavapestov slavapestov Dec 15, 2025

Choose a reason for hiding this comment

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

ah ok, is it worth adding that assert then?

Copy link
Contributor Author

@xedin xedin Dec 16, 2025

Choose a reason for hiding this comment

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

It's somewhat expensive if type has type variables because they have to be collected, so I'm not sure whether it's really worth it...

return false;

auto *NTD = type->getAnyNominal();
Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/CSTrail.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ SolverTrail::Change::RetractedBinding(TypeVariableType *typeVar,
result.Binding.TypeVar = typeVar;
result.Binding.BindingType = binding.BindingType;
result.Binding.BindingSource = binding.BindingSource;
result.Binding.Originator = binding.Originator;
result.Options = unsigned(binding.Kind);

return result;
Expand Down Expand Up @@ -563,9 +564,8 @@ void SolverTrail::Change::undo(ConstraintSystem &cs) const {
break;

case ChangeKind::RetractedBinding: {
PotentialBinding binding(Binding.BindingType,
AllowedBindingKind(Options),
Binding.BindingSource);
PotentialBinding binding(Binding.BindingType, AllowedBindingKind(Options),
Binding.BindingSource, Binding.Originator);

auto &bindings = cg[BindingRelation.TypeVar].getPotentialBindings();
bindings.Bindings.push_back(binding);
Expand Down