Skip to content

Add sending to subscript sending result #79560

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 2 commits into from
Mar 28, 2025

Conversation

stzn
Copy link
Contributor

@stzn stzn commented Feb 23, 2025

Resolves #79559

When the sending is applied to the return type in a subscript and SIL is emitted, the sending disappears from the output.

Here's an example to demonstrate this behavior:

class NonSendableKlass {}

struct S {
    subscript(_: sending NonSendableKlass) -> sending NonSendableKlass { NonSendableKlass() }
}

The SIL output(the result of emit-silgen) shows :

sil_stage raw
...
struct S {
  subscript(_: sending NonSendableKlass) -> NonSendableKlass { get } // no sending for the return value
  init()
}

...

sil hidden [ossa] @$s4main1SVyAA16NonSendableKlassCAEncig : $@convention(method) (@sil_sending @owned NonSendableKlass, S) -> @owned NonSendableKlass { // no @sil_sending for the return value

Note that while the parameter retains its sending in the SIL output, the return type's sending is missing.

@stzn stzn changed the title Add sending to subscript getter sending result Add sending to subscript sending result Feb 23, 2025
@stzn stzn marked this pull request as ready for review February 23, 2025 10:39
@hborla hborla requested a review from gottesmm February 24, 2025 17:33
@xedin
Copy link
Contributor

xedin commented Feb 28, 2025

LGTM but @gottesmm should take a look as well!

@xedin
Copy link
Contributor

xedin commented Feb 28, 2025

@swift-ci please test

@stzn
Copy link
Contributor Author

stzn commented Mar 6, 2025

Is there any additional action required from me?

@xedin
Copy link
Contributor

xedin commented Mar 7, 2025

I don't think there is anything for you to do. I'll ping @gottesmm about this again.


// CHECK: sil hidden [ossa] @$s17sending_subscript1SVyAA16NonSendableKlassCAEncig : $@convention(method) (@sil_sending @owned NonSendableKlass, S) -> @sil_sending @owned NonSendableKlass {
struct S {
subscript(_: sending NonSendableKlass) -> sending NonSendableKlass { NonSendableKlass() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case with multiple results. IIRC there was some sort of code I put in that forced all result types to also need to have sending result as well. The idea is that with time, top level sending result means all results have sending results... but once we support sending on specific results, then we allow for functions not to set the top level sending result flag and vary that specific sending result flag on results. I at least want some more test cases with a tuple result. Let me see if I can find the code in question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, now that I think about it... that property is actually something at the SIL level. Not something at the AST level where we just IIRC represent results with a type instead of using ResultInfo sort of things. Can you just out of an abundance of caution add a subscript with a tuple result test 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.

@gottesmm
Thank you for the comment.
I added a test case with a tuple result. 98209cd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gottesmm
Is anything else needed?

@xedin
Copy link
Contributor

xedin commented Mar 25, 2025

@swift-ci please test

@stzn
Copy link
Contributor Author

stzn commented Mar 26, 2025

The Windows test failed. Do we need special handling for Windows?

@xedin
Copy link
Contributor

xedin commented Mar 27, 2025

@swift-ci please test Windows platform

1 similar comment
@xedin
Copy link
Contributor

xedin commented Mar 27, 2025

@swift-ci please test Windows platform

@xedin xedin merged commit 2bb098c into swiftlang:main Mar 28, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing sending for subscript sending result
3 participants