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.

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

[SR-12723][Sema] Validate function type param representations thick-to-thin conversions #31814

Merged
39 changes: 36 additions & 3 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1349,20 +1349,53 @@ ConstraintSystem::matchTupleTypes(TupleType *tuple1, TupleType *tuple2,
return getTypeMatchSuccess();
}

// Determine whether conversion is allowed between two function types
// based on their representations.
static bool
isConversionAllowedBetween(FunctionTypeRepresentation rep1,
FunctionTypeRepresentation rep2) {
auto isThin = [](FunctionTypeRepresentation rep) {
return rep == FunctionTypeRepresentation::CFunctionPointer ||
rep == FunctionTypeRepresentation::Thin;
};

// Allowing "thin" (c, thin) to "thin" conventions
if (isThin(rep1) && isThin(rep2))
return true;

// Allowing all to "thick" (swift, block) conventions
// "thin" (c, thin) to "thick" or "thick" to "thick"
if (rep2 == FunctionTypeRepresentation::Swift ||
rep2 == FunctionTypeRepresentation::Block)
return true;

return rep1 == rep2;
}

// Returns 'false' (i.e. no error) if it is legal to match functions with the
// corresponding function type representations and the given match kind.
static bool matchFunctionRepresentations(FunctionTypeRepresentation rep1,
LucianoPAlmeida marked this conversation as resolved.
Show resolved Hide resolved
FunctionTypeRepresentation rep2,
ConstraintKind kind) {
ConstraintKind kind,
ConstraintLocatorBuilder locator) {
switch (kind) {
case ConstraintKind::Bind:
case ConstraintKind::BindParam:
case ConstraintKind::BindToPointerType:
case ConstraintKind::Equal:
return rep1 != rep2;

case ConstraintKind::Subtype: {
auto last = locator.last();
LucianoPAlmeida marked this conversation as resolved.
Show resolved Hide resolved
if (!(last && last->is<LocatorPathElt::FunctionArgument>()))
return false;

// Inverting the result because matchFunctionRepresentations
// returns false in conversions are allowed.
return !isConversionAllowedBetween(rep1, rep2);
}

case ConstraintKind::OpaqueUnderlyingType:
case ConstraintKind::Subtype:
case ConstraintKind::Conversion:
case ConstraintKind::BridgingConversion:
case ConstraintKind::ArgumentConversion:
Expand Down Expand Up @@ -1657,7 +1690,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,

if (matchFunctionRepresentations(func1->getExtInfo().getRepresentation(),
func2->getExtInfo().getRepresentation(),
kind)) {
kind, locator)) {
return getTypeMatchFailure(locator);
}

Expand Down
81 changes: 81 additions & 0 deletions validation-test/compiler_crashers_2_fixed/sr12723.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// RUN: %target-swift-emit-silgen %s -verify

func SR12723_thin(_: (@convention(thin) () -> Void) -> Void) {}
func SR12723_block(_: (@convention(block) () -> Void) -> Void) {}
func SR12723_c(_: (@convention(c) () -> Void) -> Void) {}

func SR12723_function(_: () -> Void) {}

func context() {
SR12723_c(SR12723_function)

SR12723_block(SR12723_function)

SR12723_thin(SR12723_function)
}

struct SR12723_C {
let function: (@convention(c) () -> Void) -> Void
}

struct SR12723_Thin {
let function: (@convention(thin) () -> Void) -> Void
}

struct SR12723_Block {
let function: (@convention(block) () -> Void) -> Void
}

func proxy(_ f: (() -> Void) -> Void) {
let a = 1
f { print(a) }
}

func cContext() {
let c = SR12723_C { app in app() }

proxy(c.function)
// expected-error@-1 {{cannot convert value of type '(@convention(c) () -> Void) -> Void' to expected argument type '(() -> Void) -> Void'}}

let _ : (@convention(block) () -> Void) -> Void = c.function
// expected-error@-1 {{cannot convert value of type '(@convention(c) () -> Void) -> Void' to specified type '(@convention(block) () -> Void) -> Void'}}

let _ : (@convention(c) () -> Void) -> Void = c.function // OK

let _ : (@convention(thin) () -> Void) -> Void = c.function // OK

let _ : (() -> Void) -> Void = c.function
Copy link
Contributor

Choose a reason for hiding this comment

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

This and some other conversions here are thin-to-thick which should be allowed. I think this is actually meant to work in reverse - thin-to-thick is allowed but thick-to-thin isn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least this is what Joe mentioned in his comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @xedin :)
Hum, that's actually one of the questions I had, because on the param function type there is the fact that subtype relation is contravariant. So my question is if this affects the conversion in some way?
In this case, for example, converting (@convention(c)()->Void)->Void to (()->Void)->Void on A -> B to A'-> B' respectivelly, this means the subtype relation is A' is subtype of A so the ()->Void is a subtype of @convention(c)()->Void, does this means the conversion is thick-to-thin? ... at the time this makes sense to me, but that is actually a question I have, do you know if this makes sense or am I missing something? Let me know if I this makes sense :)

Also, the logic on matchFunctionTypes is only allowing exactly thin-to-thick and thick-to-thick any other conversions are being deferred to check if representations are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are correct, sorry for the confusion. I was just merely trying to point out that we might want to change diagnostic or add a custom note which explains precisely why these types couldn't be converted (related to contravariance) because it appears that it's passing thick-to-thin.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got tripped by this myself because I didn't notice that diagnostic mentioned whole function type...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks for the clarification :)
I've added in the last commit, thin to thin which seems to be allowed, so now we strictly only validating thick-to-thin.

we might want to change diagnostic or add a custom note which explains precisely why these types couldn't be converted (related to contravariance) because it appears that it's passing

About the note, I think it makes sense, but I'm not sure how to phrase it because I'm not sure if we should mention variance, or only mention that the representations are not convertible in a parameter subtype context ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a good question how note should be worded exactly...

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about this some more - I think we should just open a follow up SR for the note and merge what we have now.

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 think it makes sense :)

// expected-error@-1 {{cannot convert value of type '(@convention(c) () -> Void) -> Void' to specified type '(() -> Void) -> Void'}}

}

func thinContext() {
let thin = SR12723_Thin { app in app() }

proxy(thin.function)
// expected-error@-1 {{cannot convert value of type '(@convention(thin) () -> Void) -> Void' to expected argument type '(() -> Void) -> Void'}}

let _ : (@convention(block) () -> Void) -> Void = thin.function
// expected-error@-1 {{cannot convert value of type '(@convention(thin) () -> Void) -> Void' to specified type '(@convention(block) () -> Void) -> Void'}}

let _ : (@convention(c) () -> Void) -> Void = thin.function // OK

let _ : (@convention(thin) () -> Void) -> Void = thin.function // OK

let _ : (() -> Void) -> Void = thin.function
// expected-error@-1 {{cannot convert value of type '(@convention(thin) () -> Void) -> Void' to specified type '(() -> Void) -> Void'}}
}

func blockContext() {
let block = SR12723_Block { app in app() }

proxy(block.function)

let _ : (@convention(block) () -> Void) -> Void = block.function // OK

let _ : (@convention(c) () -> Void) -> Void = block.function // OK

let _ : (@convention(thin) () -> Void) -> Void = block.function // OK

let _ : (() -> Void) -> Void = block.function // OK
}