Skip to content

[AST] Enforce that TupleExprs and ParenExprs dont have param flags #39193

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 4 commits into from
Sep 10, 2021
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: 4 additions & 5 deletions include/swift/AST/Types.h
Original file line number Diff line number Diff line change
Expand Up @@ -3005,12 +3005,11 @@ class AnyFunctionType : public TypeBase {

public:
/// Take an array of parameters and turn it into a tuple or paren type.
///
/// \param wantParamFlags Whether to preserve the parameter flags from the
/// given set of parameters.
static Type composeTuple(ASTContext &ctx, ArrayRef<Param> params,
bool canonicalVararg);
static Type composeTuple(ASTContext &ctx, CanParamArrayRef params,
bool canonicalVararg) {
return composeTuple(ctx, params.getOriginalArray(), canonicalVararg);
}
bool wantParamFlags = true);

/// Given two arrays of parameters determine if they are equal in their
/// canonicalized form. Internal labels and type sugar is *not* taken into
Expand Down
3 changes: 1 addition & 2 deletions lib/APIDigester/ModuleAnalyzerNodes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1519,8 +1519,7 @@ SwiftDeclCollector::constructTypeNode(Type T, TypeInitInfo Info) {
Root->addChild(constructTypeNode(Fun->getResult()));

auto Input = AnyFunctionType::composeTuple(Fun->getASTContext(),
Fun->getParams(),
/*canonicalVararg=*/false);
Fun->getParams());
Root->addChild(constructTypeNode(Input));
return Root;
}
Expand Down
8 changes: 4 additions & 4 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3457,12 +3457,12 @@ Type AnyFunctionType::Param::getParameterType(bool forCanonical,
}

Type AnyFunctionType::composeTuple(ASTContext &ctx, ArrayRef<Param> params,
bool canonicalVararg) {
bool wantParamFlags) {
SmallVector<TupleTypeElt, 4> elements;
for (const auto &param : params) {
Type eltType = param.getParameterType(canonicalVararg, &ctx);
elements.emplace_back(eltType, param.getLabel(),
param.getParameterFlags());
auto flags = wantParamFlags ? param.getParameterFlags()
: ParameterTypeFlags();
elements.emplace_back(param.getParameterType(), param.getLabel(), flags);
}
return TupleType::get(elements, ctx);
}
Expand Down
16 changes: 14 additions & 2 deletions lib/AST/ASTVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,7 @@ class Verifier : public ASTWalker {
}

void verifyChecked(TupleExpr *E) {
PrettyStackTraceExpr debugStack(Ctx, "verifying TupleExpr", E);
const TupleType *exprTy = E->getType()->castTo<TupleType>();
for_each(exprTy->getElements().begin(), exprTy->getElements().end(),
E->getElements().begin(),
Expand All @@ -1148,8 +1149,14 @@ class Verifier : public ASTWalker {
Out << elt->getType() << "\n";
abort();
}
if (!field.getParameterFlags().isNone()) {
Out << "TupleExpr has non-empty parameter flags?\n";
Out << "sub expr: \n";
elt->dump(Out);
Out << "\n";
abort();
}
});
// FIXME: Check all the variadic elements.
verifyCheckedBase(E);
}

Expand Down Expand Up @@ -1970,10 +1977,15 @@ class Verifier : public ASTWalker {

void verifyChecked(ParenExpr *E) {
PrettyStackTraceExpr debugStack(Ctx, "verifying ParenExpr", E);
if (!isa<ParenType>(E->getType().getPointer())) {
auto ty = dyn_cast<ParenType>(E->getType().getPointer());
if (!ty) {
Out << "ParenExpr not of ParenType\n";
abort();
}
if (!ty->getParameterFlags().isNone()) {
Out << "ParenExpr has non-empty parameter flags?\n";
abort();
}
verifyCheckedBase(E);
}

Expand Down
3 changes: 1 addition & 2 deletions lib/SILGen/SILGenApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4023,8 +4023,7 @@ RValue CallEmission::applyEnumElementConstructor(SGFContext C) {
std::move(*callSite).forward());

auto payloadTy = AnyFunctionType::composeTuple(SGF.getASTContext(),
resultFnType.getParams(),
/*canonicalVararg*/ true);
resultFnType->getParams());
auto arg = RValue(SGF, argVals, payloadTy->getCanonicalType());
payload = ArgumentSource(uncurriedLoc, std::move(arg));
formalResultType = cast<FunctionType>(formalResultType).getResult();
Expand Down
3 changes: 1 addition & 2 deletions lib/SILGen/SILGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2659,8 +2659,7 @@ loadIndexValuesForKeyPathComponent(SILGenFunction &SGF, SILLocation loc,

auto indexLoweredTy =
SGF.getLoweredType(
AnyFunctionType::composeTuple(SGF.getASTContext(), indexParams,
/*canonicalVararg=*/false));
AnyFunctionType::composeTuple(SGF.getASTContext(), indexParams));

auto addr = SGF.B.createPointerToAddress(loc, pointer,
indexLoweredTy.getAddressType(),
Expand Down
49 changes: 32 additions & 17 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2002,8 +2002,7 @@ static bool fixMissingArguments(ConstraintSystem &cs, ASTNode anchor,
// synthesized arguments to it.
if (argumentTuple) {
cs.addConstraint(ConstraintKind::Bind, *argumentTuple,
FunctionType::composeTuple(ctx, args,
/*canonicalVararg=*/false),
FunctionType::composeTuple(ctx, args),
cs.getConstraintLocator(anchor));
}

Expand Down Expand Up @@ -2206,21 +2205,39 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
// take multiple arguments to be passed as an argument in places
// that expect a function that takes a single tuple (of the same
// arity);
auto canImplodeParams = [&](ArrayRef<AnyFunctionType::Param> params) {
auto canImplodeParams = [&](ArrayRef<AnyFunctionType::Param> params,
const FunctionType *destFn) {
if (params.size() == 1)
return false;

for (auto param : params)
if (param.isVariadic() || param.isInOut() || param.isAutoClosure())
return false;
// We do not support imploding into a @differentiable function.
if (destFn->isDifferentiable())
return false;

for (auto &param : params) {
// We generally cannot handle parameter flags, though we can carve out an
// exception for ownership flags such as __owned, which we can thunk, and
// flags that can freely dropped from a function type such as
// @_nonEphemeral. Note that @noDerivative can also be freely dropped, as
// we've already ensured that the destination function is not
// @differentiable.
auto flags = param.getParameterFlags();
flags = flags.withValueOwnership(
param.isInOut() ? ValueOwnership::InOut : ValueOwnership::Default);
flags = flags.withNonEphemeral(false)
.withNoDerivative(false);
if (!flags.isNone())
return false;
}
return true;
};

auto implodeParams = [&](SmallVectorImpl<AnyFunctionType::Param> &params) {
// Form an imploded tuple type, dropping the parameter flags as although
// canImplodeParams makes sure we're not dealing with vargs, inout, etc,
// we may still have e.g ownership flags left over, which we can drop.
auto input = AnyFunctionType::composeTuple(getASTContext(), params,
/*canonicalVararg=*/false);

/*wantParamFlags*/ false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't canImplodeParams() return false in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only currently returns false for vargs, inout, and autoclosure, but there may still be other parameter flags like __owned or @_nonEphemeral to strip.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the question is whether canImplodeParams should return false if any of the parameters here have flags? I think the answer is yes if it's only valid to convert "simple" parameters lists into tuples otherwise there is a risk that some new parameter flag would produce a valid solution but then crash in runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point, I believe we should support stripping for ownership flags such as __owned, as we can handle the function conversion through a thunk, and we have test cases for that in the test suite. I believe we should also support stripping for @_nonEphemeral, as the attribute can be freely stripped off a function type anyways. Talking with @rxwei, it seems we should also support stripping @noDerivative (as long as the destination isn't @differentiable).

However I do agree that defaulting to returning false from canImplodeParams for any other parameter flag makes sense. For isolated in particular that seems like the right behavior.

I've gone ahead and updated the PR to default to returning false, but with some exceptions for the above mentioned cases.

params.clear();
// If fixes are disabled let's do an easy thing and implode
// tuple directly into parameters list.
Expand Down Expand Up @@ -2255,12 +2272,12 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
if (last != path.rend()) {
if (last->getKind() == ConstraintLocator::ApplyArgToParam) {
if (isSingleTupleParam(ctx, func2Params) &&
canImplodeParams(func1Params)) {
canImplodeParams(func1Params, /*destFn*/ func2)) {
implodeParams(func1Params);
increaseScore(SK_FunctionConversion);
} else if (!ctx.isSwiftVersionAtLeast(5) &&
isSingleTupleParam(ctx, func1Params) &&
canImplodeParams(func2Params)) {
canImplodeParams(func2Params, /*destFn*/ func1)) {
auto *simplified = locator.trySimplifyToExpr();
// We somehow let tuple unsplatting function conversions
// through in some cases in Swift 4, so let's let that
Expand Down Expand Up @@ -2296,11 +2313,11 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
// 2. `case .bar(let tuple) = e` allows to match multiple
// parameters with a single tuple argument.
if (isSingleTupleParam(ctx, func1Params) &&
canImplodeParams(func2Params)) {
canImplodeParams(func2Params, /*destFn*/ func1)) {
implodeParams(func2Params);
increaseScore(SK_FunctionConversion);
} else if (isSingleTupleParam(ctx, func2Params) &&
canImplodeParams(func1Params)) {
canImplodeParams(func1Params, /*destFn*/ func2)) {
implodeParams(func1Params);
increaseScore(SK_FunctionConversion);
}
Expand All @@ -2311,7 +2328,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
auto *anchor = locator.trySimplifyToExpr();
if (isa_and_nonnull<ClosureExpr>(anchor) &&
isSingleTupleParam(ctx, func2Params) &&
canImplodeParams(func1Params)) {
canImplodeParams(func1Params, /*destFn*/ func2)) {
auto *fix = AllowClosureParamDestructuring::create(
*this, func2, getConstraintLocator(anchor));
if (recordFix(fix))
Expand Down Expand Up @@ -6073,8 +6090,7 @@ ConstraintSystem::simplifyConstructionConstraint(

// Tuple construction is simply tuple conversion.
Type argType = AnyFunctionType::composeTuple(getASTContext(),
fnType->getParams(),
/*canonicalVararg=*/false);
fnType->getParams());
Type resultType = fnType->getResult();

ConstraintLocatorBuilder builder(locator);
Expand Down Expand Up @@ -7006,8 +7022,7 @@ ConstraintSystem::simplifyFunctionComponentConstraint(

if (kind == ConstraintKind::FunctionInput) {
type = AnyFunctionType::composeTuple(getASTContext(),
funcTy->getParams(),
/*canonicalVararg=*/false);
funcTy->getParams());
locKind = ConstraintLocator::FunctionArgument;
} else if (kind == ConstraintKind::FunctionResult) {
type = funcTy->getResult();
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckDeclOverride.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ static bool noteFixableMismatchedTypes(ValueDecl *decl, const ValueDecl *base) {
auto *fnType = baseTy->getAs<AnyFunctionType>();
baseTy = fnType->getResult();
Type argTy = FunctionType::composeTuple(
ctx, baseTy->getAs<AnyFunctionType>()->getParams(), false);
ctx, baseTy->getAs<AnyFunctionType>()->getParams());
auto diagKind = diag::override_type_mismatch_with_fixits_init;
unsigned numArgs = baseInit->getParameters()->size();
return computeFixitsForOverridenDeclaration(
Expand Down
3 changes: 1 addition & 2 deletions lib/Sema/TypeCheckProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2164,8 +2164,7 @@ static Type getTypeForDisplay(ModuleDecl *module, ValueDecl *decl) {
return AnyFunctionType::composeTuple(module->getASTContext(),
ctor->getMethodInterfaceType()
->castTo<FunctionType>()
->getParams(),
/*canonicalVararg=*/false);
->getParams());
}

Type type = decl->getInterfaceType();
Expand Down
21 changes: 21 additions & 0 deletions test/AutoDiff/Sema/differentiable_attr_type_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -726,3 +726,24 @@ struct Accessors: Differentiable {
// expected-error @+1 {{cannot differentiate functions returning opaque result types}}
@differentiable(reverse)
func opaqueResult(_ x: Float) -> some Differentiable { x }

// Test the function tupling conversion with @differentiable.
func tuplify<Ts, U>(_ fn: @escaping (Ts) -> U) -> (Ts) -> U { fn }
func tuplifyDifferentiable<Ts : Differentiable, U>(_ fn: @escaping @differentiable(reverse) (Ts) -> U) -> @differentiable(reverse) (Ts) -> U { fn }

func testTupling(withoutNoDerivative: @escaping @differentiable(reverse) (Float, Float) -> Float,
withNoDerivative: @escaping @differentiable(reverse) (Float, @noDerivative Float) -> Float) {
// We support tupling of differentiable functions as long as they drop @differentiable.
let _: ((Float, Float)) -> Float = tuplify(withoutNoDerivative)
let fn1 = tuplify(withoutNoDerivative)
_ = fn1((0, 0))

// In this case we also drop @noDerivative.
let _: ((Float, Float)) -> Float = tuplify(withNoDerivative)
let fn2 = tuplify(withNoDerivative)
_ = fn2((0, 0))

// We do not support tupling into an @differentiable function.
let _ = tuplifyDifferentiable(withoutNoDerivative) // expected-error {{cannot convert value of type '@differentiable(reverse) (Float, Float) -> Float' to expected argument type '@differentiable(reverse) (Float) -> Float'}}
let _ = tuplifyDifferentiable(withNoDerivative) // expected-error {{cannot convert value of type '@differentiable(reverse) (Float, @noDerivative Float) -> Float' to expected argument type '@differentiable(reverse) (Float) -> Float'}}
}
10 changes: 10 additions & 0 deletions test/Concurrency/isolated_parameters.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,13 @@ actor TestActor {

func redecl(_: TestActor) { } // expected-note{{'redecl' previously declared here}}
func redecl(_: isolated TestActor) { } // expected-error{{invalid redeclaration of 'redecl'}}

func tuplify<Ts>(_ fn: (Ts) -> Void) {} // expected-note {{in call to function 'tuplify'}}

@available(SwiftStdlib 5.5, *)
func testTuplingIsolated(_ a: isolated A, _ b: isolated A) {
// FIXME: We shouldn't duplicate the "cannot convert value of type" diagnostic (SR-15179)
tuplify(testTuplingIsolated)
// expected-error@-1 {{generic parameter 'Ts' could not be inferred}}
// expected-error@-2 2{{cannot convert value of type '(isolated A, isolated A) -> ()' to expected argument type '(Ts) -> Void'}}
}
Comment on lines +106 to +114
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DougGregor Mind double checking that we indeed shouldn't support tupling for isolated parameters?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't support this, since isolated parameters are new.

13 changes: 13 additions & 0 deletions test/Constraints/function.swift
Original file line number Diff line number Diff line change
Expand Up @@ -250,3 +250,16 @@ func test_passing_noescape_function_ref_to_generic_parameter() {
func SR14784<T>(_ fs: () -> T..., a _ : Int) -> T {
fs.first! // expected-error{{function produces expected type 'T'; did you mean to call it with '()'?}} {{11-11=()}}
}

func tuplify<Ts>(_ fn: (Ts) -> Void) {}

func testInvalidTupleImplosions() {
func takesVargs(_ x: Int, _ y: String...) {}
tuplify(takesVargs) // expected-error {{cannot convert value of type '(Int, String...) -> ()' to expected argument type '(Int) -> Void'}}

func takesAutoclosure(_ x: @autoclosure () -> Int, y: String) {}
tuplify(takesAutoclosure) // expected-error {{cannot convert value of type '(@autoclosure () -> Int, String) -> ()' to expected argument type '(@escaping () -> Int) -> Void'}}

func takesInout(_ x: Int, _ y: inout String) {}
tuplify(takesInout) // expected-error {{cannot convert value of type '(Int, inout String) -> ()' to expected argument type '(Int) -> Void'}}
}
13 changes: 13 additions & 0 deletions test/Sema/diag_non_ephemeral.swift
Original file line number Diff line number Diff line change
Expand Up @@ -525,3 +525,16 @@ func ambiguous_fn(_ ptr: UnsafeRawPointer) {} // expected-note {{found this cand
func test_ambiguity_with_function_instead_of_argument(_ x: inout Int) {
ambiguous_fn(&x) // expected-error {{ambiguous use of 'ambiguous_fn'}}
}

func tuplify<Ts>(_ fn: @escaping (Ts) -> Void) -> (Ts) -> Void { fn }

func testTuplingNonEphemeral(_ ptr: UnsafePointer<Int>) {
// Make sure we drop @_nonEphemeral when imploding params. This is to ensure
// we don't accidently break any potentially valid code.
let fn = tuplify(takesTwoPointers)
fn((ptr, ptr))

// Note we can't perform X-to-pointer conversions in this case even if we
// wanted to.
fn(([1], ptr)) // expected-error {{tuple type '([Int], UnsafePointer<Int>)' is not convertible to tuple type '(UnsafePointer<Int>, UnsafePointer<Int>)'}}
}