Skip to content

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

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
merged 1 commit into from
Oct 22, 2024
Merged

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

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
8 changes: 6 additions & 2 deletions lib/Sema/CSBindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1215,8 +1215,12 @@ bool BindingSet::isViable(PotentialBinding &binding, bool isTransitive) {
// as a binding because `$T0` could be inferred to
// `(key: String, value: Int)` and binding `$T1` to `Array<(String, Int)>`
// eagerly would be incorrect.
if (existing->Kind != binding.Kind)
continue;
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 •
edited
Loading

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 •
edited
Loading

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

continue;
}

// If new type has a type variable it shouldn't
// be considered viable.
Expand Down
15 changes: 15 additions & 0 deletions test/Constraints/generics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,3 +1072,18 @@ do {
// expected-error@-1 {{referencing instance method 'compute()' on 'Dictionary' requires the types 'any P' and 'Any' be equivalent}}
}
}

// https://github.com/swiftlang/swift/issues/77003
do {
func f<T, U>(_: T.Type, _ fn: (T) -> U?, _: (U) -> ()) {}

struct Task<E> {
init(_: () -> ()) where E == Never {}
init(_: () throws -> ()) where E == Error {}
}

func test(x: Int?.Type) {
// Note that it's important that Task stays unused, using `_ = ` changes constraint generation behavior.
f(x, { $0 }, { _ in Task {} }) // expected-warning {{result of 'Task<E>' initializer is unused}}
}
}
4 changes: 2 additions & 2 deletions validation-test/Sema/SwiftUI/rdar98862079.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ struct MyView : View {

var body: some View {
test(value: true ? $newBounds.maxBinding : $newBounds.minBinding, in: bounds)
// expected-error@-1 {{cannot convert value of type 'Binding<Binding<Double>?>' to expected argument type 'Binding<Double>'}}
// expected-note@-2 {{arguments to generic parameter 'Value' ('Binding<Double>?' and 'Double') are expected to be equal}}
// expected-error@-1 2 {{result values in '? :' expression have mismatching types 'Binding<Binding<Double>?>' and 'Binding<Double>'}}
// expected-note@-2 2 {{arguments to generic parameter 'Value' ('Binding<Double>?' and 'Double') are expected to be equal}}
}
}