-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
af14bba
cab39bf
77e6c08
2e82367
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DougGregor Mind double checking that we indeed shouldn't support tupling for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't support this, since |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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. Forisolated
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.