Skip to content

[CSBindings] Detect situations when type variable bindings could be i… #20205

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.

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
Nov 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions lib/Sema/CSBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,15 @@ ConstraintSystem::getPotentialBindings(TypeVariableType *typeVar) {
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentConversion:
case ConstraintKind::OptionalObject: {
// If there is a `bind param` constraint associated with
// current type variable, result should be aware of that
// fact. Binding set might be incomplete until
// this constraint is resolved, because we currently don't
// look-through constraints expect to `subtype` to try and
// find related bindings.
if (constraint->getKind() == ConstraintKind::BindParam)
result.PotentiallyIncomplete = true;

auto binding = getPotentialBindingForRelationalConstraint(
result, constraint, hasDependentMemberRelationalConstraints,
hasNonDependentMemberRelationalConstraints,
Expand Down
17 changes: 15 additions & 2 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -2806,6 +2806,12 @@ class ConstraintSystem {
/// Whether the bindings of this type involve other type variables.
bool InvolvesTypeVariables = false;

/// Whether the bindings represent (potentially) incomplete set,
/// there is no way to say with absolute certainty if that's the
/// case, but that could happen when certain constraints like
/// `bind param` are present in the system.
bool PotentiallyIncomplete = false;

/// Whether this type variable has literal bindings.
LiteralBindingKind LiteralBinding = LiteralBindingKind::None;

Expand Down Expand Up @@ -2850,9 +2856,14 @@ class ConstraintSystem {
if (formBindingScore(y) < formBindingScore(x))
return false;

// If the only difference is default types,
// If there is a difference in number of default types,
// prioritize bindings with fewer of them.
return x.NumDefaultableBindings < y.NumDefaultableBindings;
if (x.NumDefaultableBindings != y.NumDefaultableBindings)
return x.NumDefaultableBindings < y.NumDefaultableBindings;

// As a last resort, let's check if the bindings are
// potentially incomplete, and if so, let's de-prioritize them.
return x.PotentiallyIncomplete < y.PotentiallyIncomplete;
Copy link
Member

Choose a reason for hiding this comment

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

It's Yet Another ranking step for potential bindings, and I agree with you that this should be merged back with FullyBound (or something like it) at some point. If you need this PR to make progress... okay...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually tried to make it so FullyBound is set when there is a bind param constraint but it breaks ordering even worse (tried a bunch of different combinations), I’m hoping to re-visit fully bound soon. Trying to unblock some code with this PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the best possible answer here is to re-work bind param itself, so we don’t get into situations like that, or make some of the choices retry-able

}

void foundLiteralBinding(ProtocolDecl *proto) {
Expand Down Expand Up @@ -2885,6 +2896,8 @@ class ConstraintSystem {
void dump(llvm::raw_ostream &out,
unsigned indent = 0) const LLVM_ATTRIBUTE_USED {
out.indent(indent);
if (PotentiallyIncomplete)
out << "potentially_incomplete ";
if (FullyBound)
out << "fully_bound ";
if (SubtypeOfExistentialType)
Expand Down
8 changes: 8 additions & 0 deletions test/Constraints/closures.swift
Original file line number Diff line number Diff line change
Expand Up @@ -797,3 +797,11 @@ func test<Instances : Collection>(
) { fatalError() }

test([1]) { _, _ in fatalError(); () }

// rdar://problem/45659733
func rdar_45659733() {
func foo<T : BinaryInteger>(_: AnyHashable, _: T) {}
func bar(_ a: Int, _ b: Int) {
_ = (a ..< b).map { i in foo(i, i) } // Ok
}
}