Bug 58792 - [generator] Disallow the use of [Async] when the signature contains ref/out parameters
Summary: [generator] Disallow the use of [Async] when the signature contains ref/out p...
Status: IN_PROGRESS
Alias: None
Product: iOS
Classification: Xamarin
Component: Tools (show other bugs)
Version: XI 10.99 (xcode9)
Hardware: PC Mac OS
: Normal normal
Target Milestone: Future Cycle (TBD)
Assignee: Alex Soto [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2017-08-15 21:09 UTC by Mike Norman
Modified: 2017-11-28 23:33 UTC (History)
2 users (show)

See Also:
Tags:
Is this bug a regression?: No
Last known good build:


Attachments

Description Mike Norman 2017-08-15 21:09:10 UTC
Actual:

In MonoTouch.UIKit.UIPrinterPickerController, the method:

public virtual System.Threading.Tasks.Task<MonoTouch.UIKit.UIPrinterPickerCompletionResult>; PresentAsync (bool animated, out bool result);

contains an `out bool` parameter that captures (I think) whether or not the user selected a printer. The UIPrinterPickerCompletionResult that is passed to the Task<T> contains a field that does exactly this. There are other versions of PresentAsync that make more sense to asyncify.

This is not an isolated issue, as there are other places (in UIKit, alone) where this occurs.

Expected:
Maybe only asyncify the PresentAsync overload that does not contain an `out bool` parameter that is captured in the return value. (Though that sounded like snark, I meant only to convey that, while I can't see a reason for these bindings, there may be one of which I am not aware.)
Comment 1 Sebastien Pouliot 2017-08-15 21:32:06 UTC
Thanks @Mike!

There might be other cases but both `out` and `ref` are good signs of API that are not really usable with async.

What we need to do, in no particular order

1. btouch should emit a warning if `[Async]` is used on such API; it cannot be an error since it would break binary compatibility for anyone who made that mistake. It's possible to use `-warnaserror` to avoid future mistakes;

2. Our introspection tests should spot those conditions and report errors on the bots;

3. Obsolete our existing API that expose `out` or `ref` parameters and exclude them from XAMCORE_4_0;


@Alex please start with #1. I'll try to handle #2 and #3 independently.
Comment 2 Alex Soto [MSFT] 2017-11-28 23:13:20 UTC
PR: https://github.com/xamarin/xamarin-macios/pull/3059
Comment 3 Alex Soto [MSFT] 2017-11-28 23:29:21 UTC
Hello Mike I want to explain a bit more about the why this is not a bug in the current exposed API

This is the selector that 'UIPrinterPickerController.PresentAsync (bool animated, out bool result)' uses under the hood [1]

- (BOOL)presentAnimated:(BOOL)animated 
      completionHandler:(UIPrinterPickerCompletionHandler)completion;

The 'out bool result' is the actual boolean "Return Value" that the native selector returns, the documentations says that this value is

> YES if the picker was displayed or NO if the picker was already visible.

There is no other way to fetch this return value from our current [Async] implementation now answering your actual concern

> contains an `out bool` parameter that captures (I think) whether or not the user
> selected a printer. The UIPrinterPickerCompletionResult that is passed to the Task<T> 
> contains a field that does exactly this.

This is not completely true, as I mentioned above the 'out bool result' returns the selector result, true or false depending if picker was already visible according to docs [1], the actual bool you get from UIPrinterPickerCompletionResult[2] is the one that captures if the user selected a printer or if the user canceled the selection process.

So to recap a little if you use [Async] on a member that returns something we will generate a *Async method variant that takes an 'out var foo' so there is a way to fetch the actual return value of a selector and we'll also generate another variant of the same *Async method without the 'out var foo' so you can ignore the return value if you are not interested in it.

[1]: https://developer.apple.com/documentation/uikit/uiprinterpickercontroller/1620514-presentanimated?language=objc

[2]: https://developer.apple.com/documentation/uikit/uiprinterpickercompletionhandler?language=objc
Comment 4 Alex Soto [MSFT] 2017-11-28 23:30:58 UTC
Now after all the above explanation I must also thank you Mike for this bug report, we will now generate an error if someone tries to use [Async] with a method signature that takes an out or ref parameter.
Comment 5 Alex Soto [MSFT] 2017-11-28 23:33:15 UTC
Oh I should also mention something to @Sebastien: fortunately we do not have these mistakes on the API so there is no need for Obsolete'ing API or introspection tests, we generated uncompilable code so there is no way we are breaking someone :D

Note You need to log in before you can comment on or make changes to this bug.