Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SR-522][SR-629][SR-4161] Allow covariance in protocol conformance. #8718

Closed
wants to merge 1 commit into from
Closed

[SR-522][SR-629][SR-4161] Allow covariance in protocol conformance. #8718

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/SILGen/SILGenPoly.cpp
View file
Open in desktop
Expand Up @@ -399,6 +399,12 @@ ManagedValue Transform::transform(ManagedValue v,
inputOTK = OTK_None;
}

if (inputOTK != OTK_None && outputOTK == OTK_None) {
SILValue result = SGF.B.createUncheckedBitCast(Loc, v.getValue(),
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct -- why are you ending up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert is firing in this function, because requirement is Optional, and witness isn't.

I'd love to avoid ending up here :D

Copy link
Member

Choose a reason for hiding this comment

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

If the input is optional and the output is not, you're forcing off a level of optionality. Maybe something is going in the wrong direction?

Also in general, Optional<T> is not bitwise-castable to T -- this only holds for classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, anyway there's more ambiguity issues after switching constraint to Subtype, it will take me some time to figure this out, I'll make an update when ready.

loweredResultTy);
return ManagedValue(result, v.getCleanup());
}

// Optional-to-optional conversion.
if (inputOTK != OTK_None && outputOTK != OTK_None) {
// If the conversion is trivial, just cast.
Expand Down
10 changes: 7 additions & 3 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Expand Up @@ -1038,9 +1038,13 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,

increaseScore(ScoreKind::SK_FunctionConversion);

// Input types can be contravariant (or equal).
SolutionKind result = matchTypes(func2->getInput(), func1->getInput(),
subKind, subflags,
// Input types can be contravariant (or equal),
// but witness constranit might be covarian (or equal).
bool isWitnessConstraint = locator.last() &&
locator.last()->getKind() == ConstraintLocator::Witness;
Type type1 = isWitnessConstraint ? func1->getInput() : func2->getInput();
Type type2 = isWitnessConstraint ? func2->getInput() : func1->getInput();
Copy link
Member

Choose a reason for hiding this comment

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

The caller should instead call matchTypes() with the right order, instead of flipping them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

馃憤

SolutionKind result = matchTypes(type1, type2, subKind, subflags,
locator.withPathElement(
ConstraintLocator::FunctionArgument));
if (result == SolutionKind::Error)
Expand Down
28 changes: 16 additions & 12 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Expand Up @@ -1170,9 +1170,8 @@ matchWitness(TypeChecker &tc,

// Initialized by the setup operation.
Optional<ConstraintSystem> cs;
ConstraintLocator *locator = nullptr;
ConstraintLocator *reqLocator = nullptr;
ConstraintLocator *witnessLocator = nullptr;
ConstraintLocator *locator = nullptr;
Type witnessType, openWitnessType;
Type openedFullWitnessType;
Type reqType, openedFullReqType;
Expand Down Expand Up @@ -1225,19 +1224,17 @@ matchWitness(TypeChecker &tc,

// Open up the witness type.
witnessType = witness->getInterfaceType();
// FIXME: witness as a base locator?
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the FIXME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously consranits were added with nullptr locator, I've changed it to witness locator, I thought that's what FIXME was about, am I wrong here?

locator = cs->getConstraintLocator(nullptr);
...
cs->addConstraint(ConstraintKind::Equal, reqType, witnessType, locator);

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. I would make the change in an earlier patch so we can test it independently. I think @DougGregor should look at this since I don't understand why we need the locator here.

locator = cs->getConstraintLocator(nullptr);
witnessLocator = cs->getConstraintLocator(
static_cast<Expr *>(nullptr),
LocatorPathElt(ConstraintLocator::Witness, witness));
locator = cs->getConstraintLocator(
static_cast<Expr *>(nullptr),
LocatorPathElt(ConstraintLocator::Witness, witness));
OpenedTypeMap witnessReplacements;
if (witness->getDeclContext()->isTypeContext()) {
std::tie(openedFullWitnessType, openWitnessType)
= cs->getTypeOfMemberReference(selfTy, witness, dc,
/*isTypeReference=*/false,
/*isDynamicResult=*/false,
FunctionRefKind::DoubleApply,
witnessLocator,
locator,
/*base=*/nullptr,
&witnessReplacements);
} else {
Expand All @@ -1246,7 +1243,7 @@ matchWitness(TypeChecker &tc,
/*isTypeReference=*/false,
/*isSpecialized=*/false,
FunctionRefKind::DoubleApply,
witnessLocator,
locator,
/*base=*/nullptr);
}
openWitnessType = openWitnessType->getRValueType();
Expand All @@ -1255,9 +1252,16 @@ matchWitness(TypeChecker &tc,
};

// Match a type in the requirement to a type in the witness.
auto matchTypes = [&](Type reqType, Type witnessType)
auto matchTypes = [&](Type reqType, Type witnessType)
-> Optional<RequirementMatch> {
cs->addConstraint(ConstraintKind::Equal, reqType, witnessType, locator);
ConstraintKind kind = ConstraintKind::Conversion;
Copy link
Member

Choose a reason for hiding this comment

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

'Conversion' is too general, because there are conversions that SILGenPoly cannot express. At most this should be 'Subtype'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, probably that's way I'm getting an assert in SILGenPoly.cpp.
I'll see how Subtype would work.

Copy link
Member

Choose a reason for hiding this comment

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

What was the assert you were getting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

llvm_unreachable("Unhandled transform?")

if (auto func = dyn_cast<FuncDecl>(req)) {
auto name = req->getFullName().getBaseName();
Copy link
Contributor

Choose a reason for hiding this comment

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

should this perhaps be const

if (func->isOperator() && name.str() == "==") {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this restriction specifically in place? Going by name alone isn't going to only pick out Equatable requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is bunch of errors specifically to Equtable conformance after relaxing the constraint due to multiple candidate may exactly match the requirement, I'll add test for this.

I guess you are right, but I coudn't come up with a failing case. Probably we could use getKnownProtocolKind() here, but it's doing the same :-/.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like that's your answer then: Variance constraints inside protocols with typealiases or generic requirements in general are going to cause inconsistencies.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please add a test so we can take a look. Once we figure out the problem in more detail, we need to solve it in a general way -- special-casing Equatable is not going to work (imagine if the user defines their own protocol with a similar property).

kind = ConstraintKind::Equal;
}
}
cs->addConstraint(kind, witnessType, reqType, locator);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the order of arguments in the constraint here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because relaxing ConstraintKind isn't enough.

In this example:

protocol P {
    var a: Int? { get }
}

struct S: P {
    var a: Int
}

Constraint system would try to match Optional<Int> (as the requirement) to Int (as witness) and it would fail.

In contrast to function type convertion:

func f() -> Int { return 3 }
let g : ()->Int? = f

Constraint system would try to match Int to Optional<Int>, which works fine.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. So I think the problem is for function arguments, we need to match the witness against the requirement; for results, we need to match the requirement against the witness. This should give you the correct variance and eliminate the need to swap the arguments in matchTypes() above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-/ Why? I thought to get covariant behaviour we should always match witness agains requirement.

Also, matchFunctionTypes flips function arguments itself, as comment states:

// Input types can be contravariant (or equal)

but can they? Seems there is some inconsistency:

func subtypeArgument1(_ x: (_ fn: ((String) -> Int)) -> Int) { }
func testSubtypeArgument1(x2: (_ fn: ((String) throws -> Int)) -> Int) {
  subtypeArgument1(x2)
}

This works fine.

func subtypeArgument1(_ x:(Int) -> Int) {}
func testSubtypeArgument1(x2: (Int) throws -> Int) {
  subtypeArgument1(x2)
}

This gives an error:

error: invalid conversion from throwing function of type '(Int) throws -> Int' to non-throwing function type '(Int) -> Int'

Is this behavior intended and should we persist it?

That's why I've used locator to disable that flipping for witness function arguments and always match witness agains requirement.

Copy link
Member

Choose a reason for hiding this comment

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

The second example is correct. Passing a throwing function as a non-throwing function does not work, because the caller does not expect it to throw. If the function does throw (and we admitted this conversion) you would encounter undefined behavior.

Think of (T) -> U as a subtype of (T) throws -> U, but not the other way around.

// FIXME: Check whether this has already failed.
return None;
};
Expand Down Expand Up @@ -1286,7 +1290,7 @@ matchWitness(TypeChecker &tc,
// Compute the set of substitutions we'll need for the witness.
solution->computeSubstitutions(
witness->getInnermostDeclContext()->getGenericSignatureOfContext(),
witnessLocator,
locator,
result.WitnessSubstitutions);

return result;
Expand Down
3 changes: 1 addition & 2 deletions test/ClangImporter/objc_parse.swift
View file
Open in desktop
Expand Up @@ -486,8 +486,7 @@ func testWeakVariable() {
}

class IncompleteProtocolAdopter : Incomplete, IncompleteOptional { // expected-error {{type 'IncompleteProtocolAdopter' cannot conform to protocol 'Incomplete' because it has requirements that cannot be satisfied}}
// expected-error@-1{{type 'IncompleteProtocolAdopter' does not conform to protocol 'Incomplete'}}
@objc func getObject() -> AnyObject { return self } // expected-note{{candidate has non-matching type '() -> AnyObject'}}
@objc func getObject() -> AnyObject { return self }
}

func testNullarySelectorPieces(_ obj: AnyObject) {
Expand Down
64 changes: 64 additions & 0 deletions test/TypeCoercion/protocol_covariance.swift
View file
Open in desktop
@@ -0,0 +1,64 @@
// RUN: %target-typecheck-verify-swift

class A {}
Copy link
Member

Choose a reason for hiding this comment

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

Please put this test in test/decl/protocol, and add some error cases as well.

Also you need a SILGen test and an executable test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

馃憤

class B: A {}

protocol P {
func f1(o: Int?)
func f2() -> Int?
}

protocol P1 {
var a: A { get set }
func f() -> A
}

struct S : P1 {
func f() -> B {
return B()
}
var a = B()
}

extension S: P {
func f1(o: Int) {
print(o)
}
func f2() -> Int {
return 1
}
}

class C<G: A>: P1 {
func f() -> G {
return B() as! G
}
var a: G = B() as! G
}


protocol P2 {
func f(_:(Int?)->())
}
struct X : P2 {
func f(_:(Int)->()) {}
}

protocol Q {
var i: Int? { get }
}
struct Y : Q {
var i: Int = 0
}

protocol A1 {}
protocol B2 {}

protocol HasA1 {
var a: A1 { get }
}

struct Baz: HasA1 {
var a: A1 & B2
}