[AST] Enforce that TupleExprs and ParenExprs dont have param flags #39193
Conversation
| auto input = AnyFunctionType::composeTuple(getASTContext(), params, | ||
| /*canonicalVararg=*/false); | ||
|
|
||
| /*wantParamFlags*/ false); |
There was a problem hiding this comment.
Wouldn't canImplodeParams() return false in this case?
There was a problem hiding this comment.
It only currently returns false for vargs, inout, and autoclosure, but there may still be other parameter flags like __owned or @_nonEphemeral to strip.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Remove the canonicalVararg parameter and CanParamArrayRef wrapper. Almost none of the callers want canonicalVararg, and the one that does calls `getCanonicalType` on the result anyway.
By default avoid imploding params that have parameter flags, but carve out exceptions for ownership flags, which can be thunked, and `@_nonEphemeral` which can be freely dropped without issue.
Reject tupling into a `@differentiable` function, and allow the stripping of `@noDerivative` given we're also losing `@differentiable`.
Tuple and paren types may still have parameter type flags in certain cases, but they should never make it into expressions.
hamishknight
force-pushed
the
less-toil-more-tuple
branch
from
f9142ed to
2e82367
Compare
| 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'}} | ||
| } |
There was a problem hiding this comment.
@DougGregor Mind double checking that we indeed shouldn't support tupling for isolated parameters?
There was a problem hiding this comment.
We shouldn't support this, since isolated parameters are new.
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
Tuple and paren types may still have parameter type flags in certain cases, but they should never make it into expressions.
This requires formalizing the rules around whether we can convert a function with N parameters into a function with a single N-element tuple parameter. Previously, only functions with variadic, autoclosure, or inout parameter flags were rejected. Other parameter flags were preserved on the resulting tuple, which could cause issues in downstream code, as tuples shouldn't have parameter flags. This patch ensures we either reject the parameter flags, or strip them from the tuple type.
By default, a parameter with non-empty parameter flags will now be rejected, with an exception for ownership flags,
@_nonEphemeral, and@noDerivative.This means that tupling
isolatedparameters is now forbidden. An additional rule has also been added that forbids tupling into a@differentiablefunction.