Skip to content

Sema: Respect TVO_CanBindToPack #61777

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

Closed
wants to merge 2 commits into from
Closed

Sema: Respect TVO_CanBindToPack #61777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76103ce
Disable variadic_generic_types.swift in backward-deployment tests
slavapestov Mar 6, 2023
0b8927c
Sema: Respect TVO_CanBindToPack
slavapestov Oct 27, 2022
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
14 changes: 14 additions & 0 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4307,6 +4307,14 @@ ConstraintSystem::matchTypesBindTypeVar(
}
}

if (type->is<PackType>() || type->is<PackArchetypeType>()) {
if (!typeVar->getImpl().canBindToPack())
return getTypeMatchFailure(locator);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a bi-directional fix here similar to fixReferenceMismatch otherwise constraint solver is just going to fail with "failed to produce a diagnostic" because it wouldn't be able to form a valid solution due to this binding always failing.

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'll let @hborla handle this one :) I was just rebasing this PR so that she could finish it off.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm going to pick this up and address the feedback about diagnostics!

} else if (!type->is<PlaceholderType>()) {
if (typeVar->getImpl().canBindToPack())
return getTypeMatchFailure(locator);
}

// We do not allow keypaths to go through AnyObject. Let's create a fix
// so this can be diagnosed later.
if (auto loc = typeVar->getImpl().getLocator()) {
Expand Down Expand Up @@ -6536,6 +6544,12 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
// trivially solved.
return getTypeMatchSuccess();
}

// If exactly one of the type variables can bind to an pack, we
// can't merge these two type variables.
if (rep1->getImpl().canBindToPack()
!= rep2->getImpl().canBindToPack())
return getTypeMatchFailure(locator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of failing should this just from an unsolved constraint and wait until one of type variables is resolved so it could be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TVO_CanBindToPack should never change. It's set when the type variable is created in CSGen.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's reasonable but we cannot diagnose here and at the same time cannot fail right away either I think, we need to waiting into more is know about one of the types or it's marked as a placeholder...

Copy link
Member

Choose a reason for hiding this comment

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

Right, we can't unconditionally fail. When Pavel says "so it could be fixed", I think he means that this constraint needs to ultimately return SolutionKind::Solved in diagnostic mode with some kind of fix recorded. The solver should never fail solving a constraint in diagnostic mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

should never fail solving a constraint in diagnostic mode

Shouldn't fail unless it leads to a worse solution i.e. if the impact of the fix is greater than previous solutions that have been discovered.

}

switch (kind) {
Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,12 @@ void ConstraintSystem::assignFixedType(TypeVariableType *typeVar, Type type,
return;

if (!type->isTypeVariableOrMember()) {
if (type->is<PackType>() || type->is<PackArchetypeType>()) {
assert(typeVar->getImpl().canBindToPack());
} else if (!type->is<PlaceholderType>()) {
assert(!typeVar->getImpl().canBindToPack());
}

// If this type variable represents a literal, check whether we picked the
// default literal type. First, find the corresponding protocol.
//
Expand Down
3 changes: 3 additions & 0 deletions test/Interpreter/variadic_generic_types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
// Because of -enable-experimental-feature VariadicGenerics
// REQUIRES: asserts

// UNSUPPORTED: use_os_stdlib
// UNSUPPORTED: back_deployment_runtime

import StdlibUnittest

var types = TestSuite("VariadicGenericTypes")
Expand Down