Skip to content

Conversation

@xedin
Copy link
Contributor

@xedin xedin commented Dec 15, 2025

Instead of using a flag in addBinding, let's record what type variable does the binding belong to in PotentialBinding itself. This is going to help remove bindings introduced transitively when the inference is done lazily.

Instead of using a flag in `addBinding`, let's record what type variable
does the binding belong to in `PotentialBinding` itself. This is going
to help remove bindings introduced transitively when the inference is
done lazily.
@xedin xedin requested a review from hborla as a code owner December 15, 2025 21:59
@xedin xedin requested review from slavapestov and removed request for hborla December 15, 2025 22:00
@xedin
Copy link
Contributor Author

xedin commented Dec 15, 2025

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Dec 15, 2025

@swift-ci please test source compatibility

auto type = binding.BindingType;

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

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

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

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

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...

@xedin
Copy link
Contributor Author

xedin commented Dec 16, 2025

@swift-ci please test Windows platform

@xedin xedin merged commit 6dece8a into swiftlang:main Dec 16, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants