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

Handling objects passed as "out parameters" #277

Merged
merged 5 commits into from
Feb 1, 2023
Merged

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Oct 6, 2022

See Passing to an out parameter by writeback.

Some methods take pointers to an instance, and, if the pointer is not NULL, write some value to the pointer. Examples include:

Note that out parameters work by autoreleasing the value - there may be a design where we embrace this, allowing us to cut down on some unnecessary retains and releases:

// Id<T, O> -> Id<T, O>
{
    let mut foo: Option<Id<T, O>>;
    // let old_foo = foo;
    msg_send![abc, validateValue: &mut foo];
    // [foo retain]
    // [old_foo release]

    drop(foo)
}

// &T -> &T (easily possible today, but not very clear that the reference is bound to the pool)
pool {
    let mut foo: Option<&T>;
    msg_send![abc, validateValue: &mut foo];

    // `foo` still usable inside the pool
}

// Id<T, O> -> &T
// Can be done using the above combined with `Option::as_deref`
pool {
    let foo: Option<Id<T, O>>;

    let mut new_foo: Option<&T> = foo1.as_deref();
    msg_send![abc, validateValue: &mut new_foo];

    // `new_foo` should still somehow be bound to the pool

    drop(foo)
}

// &T -> Id<T, O>
// Can easily be done with a manual `Id::retain`, similar to the above
pool {
    let mut foo: Option<&T>;
    msg_send![abc, validateValue: &mut foo];
    let new_foo: Option<Id<T, O>> = unsafe { Id::retain(foo) };
}

TODO

Many things, including documenting how out and inout parameters work ABI-wise They don't have an effect on the ABI:

@madsmtm madsmtm added enhancement New feature or request A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels Oct 6, 2022
@madsmtm
Copy link
Owner Author

madsmtm commented Oct 6, 2022

One implementation is the current Id::writeback (name up for bikeshedding).

But I'm wondering whether we could just allow passing &mut Option<Id<T, O>>/Option<&mut Option<Id<T, O>> in msg_send!, and let it automatically handle the required retain/release?

@madsmtm madsmtm changed the title Handling "out parameters" Handling instances as "out parameters" Oct 6, 2022
@madsmtm madsmtm changed the title Handling instances as "out parameters" Handling objects passed as "out parameters" Oct 6, 2022
madsmtm added a commit that referenced this pull request Oct 7, 2022
Assuming we find a good solution to #277
@madsmtm
Copy link
Owner Author

madsmtm commented Oct 26, 2022

Also: Can we let it be automatically handled in declare_class!?

madsmtm added a commit that referenced this pull request Nov 1, 2022
Assuming we find a good solution to #277
madsmtm added a commit that referenced this pull request Nov 24, 2022
Assuming we find a good solution to #277
madsmtm added a commit that referenced this pull request Dec 8, 2022
Assuming we find a good solution to #277
madsmtm added a commit that referenced this pull request Dec 21, 2022
See full history in 1c4c875.

Add initial header translation

Closer to usable

Mostly works with NSCursor

Mostly works with NSAlert.h

Refactor a bit

AppKit is now parse-able

handle reserved keywords

Handle protocols somewhat

Handle the few remaining entity kinds

Works with Foundation

Cleanup

Refactor

Refactor Method to (almost) be PartialEq

Parse more things

Parse NSConsumed

Verify memory management

More work

Fix reserved keywords

Refactor statements

Add initial availability

Prepare RustType

Split RustType up in parse and ToToken part

Add icrate

Add config files

Temporarily disable protocol generation

Generate files

Add initial generated files for Foundation

Skip "index" header

Add basic imports

Allow skipping methods

Properly emit `unsafe`

Make classes public

Rename objc feature flag

Improve imports somewhat

Further import improvements

Handle basic typedefs

Improve generics handling

Improve pointers to objects

Refactor RustType::TypeDef

Mostly support generics

Refactor config setup

Small fixes

Support nested generics

Comment out a bit of debug logging

Emit all files

Parse sized integer types

Parse typedefs that map to other typedefs

Appease clippy

Add `const`-parsing for RustType::Id

Parse Objective-C in/out/inout/bycopy/byref/oneway qualifiers

Fix `id` being emitted when it actually specifies a protocol

Make AppKit work again

Parse all qualifiers, in particular lifetime qualifiers

More consistent ObjCObjectPointer parsing

Validate some lifetime attributes

Fix out parameters (except for NSError)

Assuming we find a good solution to #277

Refactor Stmt objc declaration parsing

Clean up how return types work

Refactor property parsing

Fixes their order to be the same as in the source file

Add support for functions taking NSError as an out parameter

Assuming we do #276

Change icrate directory layout

Refactor slightly

Refactor file handling to allow for multiple frameworks simultaneously

Put method output inside an extern_methods! call

We'll want this no matter what, since it'll allow us to extend things with availability attributes in the future.

Use extern_methods! functionality

To cut down on the amount of code, which should make things easier to review and understand.

This uses features which are not actually yet done, see #244.

Not happy with the formatting either, but not sure how to fix that?

Manually fix the formatting of attribute macros in extern_methods!

Add AppKit bindings

Speed things up by optionally formatting at the end instead

Prepare for parsing more than one SDK

Specify a minimum deployment target

Document SDK situation

Parse headers on iOS as well

Refactor stmt parsing a bit

Remove Stmt::FileImport and Stmt::ItemImport

These are not nearly enough to make imports work well anyhow, so I'll rip it out and find a better solution

Do preprocessing step explicitly as the first thing

Refactor so that file writing is done using plain Display

Allows us to vastly improve the speed, as well as allowing us to make the output much prettier wrt. newlines and such in the future (proc_macro2 / quote output is not really meant to be consumed by human eyes)

Improve whitespace in generated files and add warning header

Don't crash on unions

Add initial enum parsing

Add initial enum expr parsing

Add very simple enum output

Fix duplicate enum check

Improve enum expr parsing

This should make it easier for things to work on 32-bit platforms

Add static variable parsing

Add a bit of WIP code

Add function parsing

Fix generic struct generation

Make &Class as return type static

Trim unnecessary parentheses

Fix generics default parameter

Remove methods that are both instance and class methods

For now, until we can solve this more generally

Skip protocols that are also classes

Improve imports setups

Bump recursion limit

Add MacTypes.h type translation

Fix int64_t type translation

Make statics public

Fix init methods

Make __inner_extern_class allowing trailing comma in generics

Attempt to improve Rust's parsing speed of icrate

Custom NSObject

TMP import

Remove NSProxy

Temporarily remove out parameter setup

Add struct support

Add partial support for "related result types"

Refactor typedef parsing a bit

Output remaining typedefs

Fix Option<Sel> and *mut bool

Fix almost all remaining type errors in Foundation

Skip statics whoose value we cannot find

Fix anonymous enum types

Fix AppKit duplicate methods

Add CoreData

Properly fix imports

Add `abstract` keyword

Put enum and static declarations behind a macro

Add proper IncompleteArray parsing

Refactor type parsing

Make NSError** handling happen in all the places that it does with Swift

Refactor Ty a bit more

Make Display for RustType always sound

Add support for function pointers

Add support for block pointers

Add extern functions

Emit protocol information

We can't parse it yet though, see #250

Make CoreData compile

Make AppKit compile

Add support for the AuthenticationServices framework

Do clang < v13 workarounds without modifying sources

Refactor Foundation fixes
madsmtm added a commit that referenced this pull request Dec 23, 2022
See full history in 1c4c875.

Add initial header translation

Closer to usable

Mostly works with NSCursor

Mostly works with NSAlert.h

Refactor a bit

AppKit is now parse-able

handle reserved keywords

Handle protocols somewhat

Handle the few remaining entity kinds

Works with Foundation

Cleanup

Refactor

Refactor Method to (almost) be PartialEq

Parse more things

Parse NSConsumed

Verify memory management

More work

Fix reserved keywords

Refactor statements

Add initial availability

Prepare RustType

Split RustType up in parse and ToToken part

Add icrate

Add config files

Temporarily disable protocol generation

Generate files

Add initial generated files for Foundation

Skip "index" header

Add basic imports

Allow skipping methods

Properly emit `unsafe`

Make classes public

Rename objc feature flag

Improve imports somewhat

Further import improvements

Handle basic typedefs

Improve generics handling

Improve pointers to objects

Refactor RustType::TypeDef

Mostly support generics

Refactor config setup

Small fixes

Support nested generics

Comment out a bit of debug logging

Emit all files

Parse sized integer types

Parse typedefs that map to other typedefs

Appease clippy

Add `const`-parsing for RustType::Id

Parse Objective-C in/out/inout/bycopy/byref/oneway qualifiers

Fix `id` being emitted when it actually specifies a protocol

Make AppKit work again

Parse all qualifiers, in particular lifetime qualifiers

More consistent ObjCObjectPointer parsing

Validate some lifetime attributes

Fix out parameters (except for NSError)

Assuming we find a good solution to #277

Refactor Stmt objc declaration parsing

Clean up how return types work

Refactor property parsing

Fixes their order to be the same as in the source file

Add support for functions taking NSError as an out parameter

Assuming we do #276

Change icrate directory layout

Refactor slightly

Refactor file handling to allow for multiple frameworks simultaneously

Put method output inside an extern_methods! call

We'll want this no matter what, since it'll allow us to extend things with availability attributes in the future.

Use extern_methods! functionality

To cut down on the amount of code, which should make things easier to review and understand.

This uses features which are not actually yet done, see #244.

Not happy with the formatting either, but not sure how to fix that?

Manually fix the formatting of attribute macros in extern_methods!

Add AppKit bindings

Speed things up by optionally formatting at the end instead

Prepare for parsing more than one SDK

Specify a minimum deployment target

Document SDK situation

Parse headers on iOS as well

Refactor stmt parsing a bit

Remove Stmt::FileImport and Stmt::ItemImport

These are not nearly enough to make imports work well anyhow, so I'll rip it out and find a better solution

Do preprocessing step explicitly as the first thing

Refactor so that file writing is done using plain Display

Allows us to vastly improve the speed, as well as allowing us to make the output much prettier wrt. newlines and such in the future (proc_macro2 / quote output is not really meant to be consumed by human eyes)

Improve whitespace in generated files and add warning header

Don't crash on unions

Add initial enum parsing

Add initial enum expr parsing

Add very simple enum output

Fix duplicate enum check

Improve enum expr parsing

This should make it easier for things to work on 32-bit platforms

Add static variable parsing

Add a bit of WIP code

Add function parsing

Fix generic struct generation

Make &Class as return type static

Trim unnecessary parentheses

Fix generics default parameter

Remove methods that are both instance and class methods

For now, until we can solve this more generally

Skip protocols that are also classes

Improve imports setups

Bump recursion limit

Add MacTypes.h type translation

Fix int64_t type translation

Make statics public

Fix init methods

Make __inner_extern_class allowing trailing comma in generics

Attempt to improve Rust's parsing speed of icrate

Custom NSObject

TMP import

Remove NSProxy

Temporarily remove out parameter setup

Add struct support

Add partial support for "related result types"

Refactor typedef parsing a bit

Output remaining typedefs

Fix Option<Sel> and *mut bool

Fix almost all remaining type errors in Foundation

Skip statics whoose value we cannot find

Fix anonymous enum types

Fix AppKit duplicate methods

Add CoreData

Properly fix imports

Add `abstract` keyword

Put enum and static declarations behind a macro

Add proper IncompleteArray parsing

Refactor type parsing

Make NSError** handling happen in all the places that it does with Swift

Refactor Ty a bit more

Make Display for RustType always sound

Add support for function pointers

Add support for block pointers

Add extern functions

Emit protocol information

We can't parse it yet though, see #250

Make CoreData compile

Make AppKit compile

Add support for the AuthenticationServices framework

Do clang < v13 workarounds without modifying sources

Refactor Foundation fixes
@madsmtm madsmtm added the A-framework Affects the framework crates and the translator for them label Dec 23, 2022
@madsmtm madsmtm added this to the objc2 v0.3 milestone Jan 18, 2023
This was referenced Jan 27, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Jan 30, 2023

For declare_class!, let's have a look at this Objective-C ARC-enabled code:

+(void) outParam: (NSObject * _Nonnull * _Nonnull) param {
    NSObject* x = *param;
    *param = [x description];
    *param = [NSObject new];
}

It compiles to roughly this non-ARC code:

+(void) outParam: (NSObject * _Nonnull * _Nonnull) param {
    NSObject* x = objc_retain(*param);
    @defer { // If such a thing existed
        objc_release(x);
    }

    NSObject* __tmp = [x description];
    retainAutoreleasedReturnValue(__tmp);
    objc_autorelease(__tmp);
    *param = __tmp;

    NSObject* __tmp2 = [NSObject new];
    objc_autorelease(__tmp2);
    *param = __tmp2;
}

The retain/release on x is simply a deficiency in ARC, it has to keep locals alive for longer, if you just did [*param description], then those would not be there. So we don't have to handle that.

To translate the rest to Rust, we could do something like this:

pub struct Helper<T, O>(ManuallyDrop<T, O>);

impl<T, O> Helper<T, O> {
    pub fn write(&mut self, obj: Id<T, O>) {
        objc_autorelease(obj.as_ptr());
        *self.0 = obj;
    }
}

impl<T, O> Deref for Helper<T, O> {
    type Target = T;
}
impl<T, O> DerefMut for Helper<T, O> {}

extern "C" fn out_param(&self, _cmd: Sel, param: &mut Helper<NSObject, Shared>) {
    // *param = ...;
    // ^ would be disallowed, instead we'd do:
    let x = &**param; // If you really wanted the retain/release that ARC does, we could've just done a `.clone()` here
    param.write(x.description());
    param.write(NSObject::new());
}

This would give us the exact same semantics, at the cost of not being able to use &mut Id<T, O> as is otherwise natural :/

I'm wondering if we could perhaps do the following instead:

extern "C" fn out_param(&self, _cmd: Sel, param: &mut Id<NSObject, Shared>) {
    objc_retain(*param);
    {
        let x = &**param;
        *param = x.description();
        *param = NSObject::new();
    }
    objc_autorelease(*param); // Maybe in Drop?
}

Which is roughly the following in Objective-C non-ARC:

+(void) outParam: (NSObject * _Nonnull * _Nonnull) param {
    objc_retain(*param);
    {
        NSObject* __old_param = *param;
        NSObject* __tmp = [__old_param description];
        retainAutoreleasedReturnValue(__tmp);
        *param = __tmp;
        objc_release(__old_param);
    
        NSObject* __old_param = *param;
        *param = [NSObject new];
        objc_release(__old_param);
    }
    objc_autorelease(*param);
}

Which I'm pretty sure would be correct, but it would also be slightly less efficient, since there would basically always be an extra retain/release call than is strictly necessary.

For ergonomics though, I think it's probably the right call to do it this way (also, doing an autorelease by far exceeds the cost of retain/release, so in the case where the variable is assigned to twice, this would very likely be more efficient).

(Note: For NSError parameters we'd already want to something else, so this does not apply there)

@madsmtm madsmtm force-pushed the out-parameters branch 3 times, most recently from 2dcfe98 to a4e1362 Compare February 1, 2023 07:22
Supported types:
- `&mut Id<_, _>`
- `&mut Option<Id<_, _>>`
- `Option<&mut Id<_, _>>`
- `Option<&mut Option<Id<_, _>>>`
@madsmtm madsmtm merged commit f6fb1b3 into master Feb 1, 2023
@madsmtm madsmtm deleted the out-parameters branch February 1, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant