Skip to content

[CSBindings] Limit BindingSet::isViable binding skipping to stdlib … #77153

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Oct 21, 2024

…collection types

This is follow-up to #76487

It's reasonable to coalesce bindings of different kind if they don't allow implicit conversions like stdlib collection types do.

Resolves: #77003

…collection types

This is follow-up to swiftlang#76487

It's reasonable to coalesce bindings of different kind if they don't
allow implicit conversions like stdlib collection types do.

Resolves: swiftlang#77003
@xedin
Copy link
Contributor Author

xedin commented Oct 21, 2024

@swift-ci please test

if (existing->Kind != binding.Kind) {
// Array, Set and Dictionary allow conversions, everything else
// requires their generic arguments to match exactly.
if (existingType->isKnownStdlibCollectionType())
Copy link
Contributor

Choose a reason for hiding this comment

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

What about Optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this check should also call hasConversions()?

Copy link
Contributor Author

@xedin xedin Oct 21, 2024

Choose a reason for hiding this comment

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

We allow injections into optional but not conversions of its generic arguments, this only covers situations like Int? conv $T and $T conv $U?.

Copy link
Contributor Author

@xedin xedin Oct 21, 2024

Choose a reason for hiding this comment

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

This spot is a lot narrower so I don't think we can use hasConversions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We allow injections into optional but not conversions of its generic arguments,

The optional-to-optional conversion is a lot like Array upcast isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somewhat but it wasn’t allowed previously but the logic here so we are not changing the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expressions like:

func test<T>(_: T, _: () -> T?) {}

class A {
}

class B : A {}

test(A()) {
    Optional(B())
}

Still work fine because optional conversion differ from collection upcast in that solver doesn't introduce a separate type variable when they are bound so even if we infer result type to be B? it would still much fine against A? from context, unlike what happens for collections where we'd infer the element and match it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also allowing both Int? and $T? bindings results in the same solution multiple times, something that CSRanking doesn't properly handle today because we don't expect that to happen without score or binding differences, that's most likely the logic didn't have change check, I'd rather we revisit this whole method in the future than try to fix it now...

@xedin
Copy link
Contributor Author

xedin commented Oct 22, 2024

@swift-ci please test source compatibility

@xedin xedin merged commit 6754918 into swiftlang:main Oct 22, 2024
7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: ambiguous use of 'Task.init(priority:operation:)'
2 participants