This is Xamarin's bug tracking system. For product support, please use the support links listed in your Xamarin Account.
Bug 43579 - Generator emits invalid code when using the same method name (overload) in @delegates using events and C# delegates
Summary: Generator emits invalid code when using the same method name (overload) in @d...
Status: VERIFIED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: General (show other bugs)
Version: XI 9.99 (iOS 10 previews)
Hardware: PC Mac OS
: --- normal
Target Milestone: (C9)
Assignee: Alex Soto
URL:
Depends on:
Blocks:
 
Reported: 2016-08-22 02:04 UTC by Alex Soto
Modified: 2016-11-07 21:01 UTC (History)
6 users (show)

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


Attachments
Test Case (5.28 KB, application/zip)
2016-08-22 02:04 UTC, Alex Soto
Details
Fixed Test Case (8.21 KB, application/zip)
2016-09-15 19:21 UTC, Alex Soto
Details

Description Alex Soto 2016-08-22 02:04:01 UTC
Created attachment 17152 [details]
Test Case

Generator will emit invalid code when using the same name in a method inside a @delegate (protocol) that is decorated either with EventArgs or DelegateName like in the following example 

> interface IFooDelegate { }
> 
> [BaseType (typeof (NSObject))]
> [Model]
> [Protocol]
> interface FooDelegate {
> 
> 	// Events
> 	[Export ("FooDidEndEditing:"), EventArgs ("FooDelegateEnded"), EventName ("Ended")]
> 	void EditingEnded (Foo foo);
> 
> 	[Export ("FooDidEndEditing:after:"), EventArgs ("FooDelegateEndedTime"), EventName ("Ended2")]
> 	void EditingEnded (Foo foo, double time);
> 
> 
> 	// Using Func
> 	[Export ("foo:dobar:"), DelegateName ("Func<Foo,nint,bool>"), DefaultValue ("true")]
> 	bool DoBar (Foo foo, nint bar);
> 
> 	[Export ("foo:doBar:after:"), DelegateName ("Func<Foo,nint,double,bool>"), DefaultValue ("true")]
> 	bool DoBar (Foo foo, nint bar, double time);
> 
> 
> 	// Using an actual DelegateName
> 	[Export ("foo:doAnotherBar:"), DelegateName ("FooDelegateAnotherBar"), DefaultValue ("true")]
> 	bool DoAnotherBar (Foo foo, nint bar);
> 
> 	[Export ("foo:doAnotherBar:after:"), DelegateName ("FooDelegateAnotherBarTime"), DefaultValue ("true")]
> 	bool DoAnotherBar (Foo foo, nint bar, double time);
> }
> 
> [BaseType (typeof (NSObject), Delegates = new string [] { "Delegate" }, Events = new Type [] { typeof (FooDelegate) })]
> interface Foo {
> 
> 	[Export ("delegate", ArgumentSemantic.Assign), NullAllowed]
> 	IFooDelegate Delegate { get; set; }
> }

attached test case
Comment 1 Sebastien Pouliot 2016-09-02 13:25:00 UTC
I have a fix for the first errors (duplicated internal field names) but the next part of this problem cannot totally be fixed - the case where [EventName] is used do work :)

E.g.

	[Export ("foo:dobar:"), DelegateName ("Func<Foo,nint,bool>"), DefaultValue ("true")]
	bool DoBar (Foo foo, nint bar);

	[Export ("foo:doBar:after:"), DelegateName ("Func<Foo,nint,double,bool>"), DefaultValue ("true")]
	bool DoBar (Foo foo, nint bar, double time);

as it would generate two identically named events

	public Func<Foo,nint,bool> DoBar {
		
	public Func<Foo,nint,double,bool> DoBar {

which is not allowed by C# (only the return type is different). 

If you have to change the name then it's not an issue anymore (for the generator).

@Alex what was the original case (in the bindings) ? I want to see if we can avoid the `2` suffix.
Comment 3 Alex Soto 2016-09-07 03:53:20 UTC
Sebastien's patch: https://gist.github.com/spouliot/a5477c4cf19b33bf3682216e3ea30316

Once we fix this bug we need to go to uikit.cs and fix the following members

- UITextFieldDelegate.EditingEnded2 (UITextField textField, UITextFieldDidEndEditingReason reason)

- UITextViewDelegate.ShouldInteractWithUrl2 (UITextView textView, NSUrl url, NSRange characterRange, UITextItemInteraction interaction)

- UITextViewDelegate.ShouldInteractWithTextAttachment2 (UITextView textView, NSTextAttachment textAttachment, NSRange characterRange, UITextItemInteraction interaction) 

We do need to find a better name for the above events ^
Comment 4 Alex Soto 2016-09-13 06:57:43 UTC
Propposed PR: https://github.com/xamarin/xamarin-macios/pull/817
Comment 5 Sebastien Pouliot 2016-09-14 13:24:34 UTC
Merged in xamarin-macios/master e397849108d9e019369d52538f61226689d32372

@Alex please re-enable the API that were commented (on master) then create a trello card to backport both the generator and the API fixes for SR1. Thanks!

[1] https://trello.com/b/ddOX7IOt/platform-cycle-8-service-release-1
Comment 6 Alex Soto 2016-09-14 21:56:13 UTC
Resolved and fixed in: 

xamarin-macios/master e397849108d9e019369d52538f61226689d32372

and

xamarin-macios/master 1da975dbf468bf0e1a22e74c1f613db1ab525cc5
Comment 7 Mohit Kheterpal 2016-09-15 10:53:01 UTC
As per comment 6, this issue has been fixed for Alex Soto.

So, closing this issue by marking it as Verified Fixed.

thanks
Comment 8 Mohit Kheterpal 2016-09-15 10:59:17 UTC
I am able to reproduce this issue with test sample provided in bug description with stable builds of cycle 8.

I am observing a different error with fixed build of master xamarin.ios-10.1.0.23_1da975dbf468bf0e1a22e74c1f613db1ab525cc5

Error : /OverloadsBug/OverloadsBug/BTOUCH: Error BI1043: btouch: Repeated overload DoAnotherBar and no [DelegateApiNameAttribute] provided to generate property name on host class. (BI1043) (OverloadsBug)

But I am not sure about the expected behaviour, could you please let me know is this the expected behaviour? 

so that I can verify this issue accordingly.

As of now keeping this bug in Resolved fixed state.

thanks
Comment 9 Alex Soto 2016-09-15 19:21:42 UTC
Created attachment 17515 [details]
Fixed Test Case

This test case adds the new required DelegateApiName where it is needed so the binding builds successfully
Comment 10 Alex Soto 2016-09-15 19:33:09 UTC
Hello Mohit

The behaviour you are seeing is now the expected, whenever we get wrong bindings like the ones in the test case now instead of generating invalid code we will issue an error  like BI1043 so what you are seeing is correct because the bindings lack the DelegateApiName attribute.

I have attached a fixed test case where the DelegateApiNameAttribute is present and the bindings are generated correctly.

Cheers!
Comment 11 Mohit Kheterpal 2016-09-16 10:14:51 UTC
Thanks @Alex for the test case.

Now I have checked it with the test case provided by you in comment 9 and it builds successfully.

Using latest build of master xamarin.ios-10.1.0.28_4a87ccf948295309dbe42acf6a2f8b3a5aeab900

Screencast : http://www.screencast.com/t/ks0PndRsUhjn

Hence I am closing this issue by marking it as Verified fixed.

thanks
Comment 12 Brendan Zagaeski (Xamarin Support) 2016-10-20 02:04:27 UTC
Apologies in advance for using the "REOPENED" status as the best available option to raise a question about release inclusion.

Comment 5 indicates that there was a plan to backport this change to Cycle 8 for Cycle 8 – Service Release 1.  But at the moment, the changes have not yet been backported.

If it has been decided to leave this change until Cycle 9, then feel free to just update the target milestone accordingly and reset the resolution/verification status.


Thanks!
Comment 13 Sebastien Pouliot 2016-10-20 02:10:05 UTC
Good catch, milestone was not updated when the bug was closed

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