Skip to content

[generator] Ensure DIM from Cecil imported references get correctly marked. #770

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 1 commit into from
Jan 5, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jan 5, 2021

Context: dotnet/android-libraries#235

Given the following API:

public interface IMyInterface {
  void DoSomething () => throw new Exception ();
}

public abstract class MyClass : IMyInterface { }

We need to ensure that Method.IsInterfaceDefaultMethod gets set for DoSomething so that our BoundClass.AddInterfaceAbstractMembers logic knows it does not need to create an abstract MyClass.DoSomething ().

This works correctly if the interface is in the assembly we are binding (because it uses XmlApiImporter). However if the interface is in a reference assembly it uses CecilApiImporter. Our Cecil importer only marks the DIM if it has [JavaInterfaceDefaultMethodAttribute] which we don't appear to ever emit.

So if the interface is in a reference assembly (like Mono.Android), the following gets generated...

public abstract class MyClass : IMyInterface {
  public abstract void DoSomething ();
}

...which causes most bindings that inherit MyClass to fail since they don't think they need to implement the DIM.

This commit marks the interface method as default if it is neither abstract nor static.

Testing

Unfortunately we cannot easily write a unit test for this, as we test Cecil importing by compiling types into the test library (source), and the test library is still net472 to support running on Mono, which does not support DIM.

I ran this against the library in the Context and it fixed that case.

@jpobst jpobst marked this pull request as ready for review January 5, 2021 19:20
@jonpryor
Copy link
Member

jonpryor commented Jan 5, 2021

Commit summary:

[generator] Ensure DIM from assembly refs are correctly marked (#770)

Message:

Context: https://github.com/xamarin/AndroidX/pull/235

Consider the following Java API:

	// Java
	public interface MyInterface {
	    default void doSomething() {}
	}

	public class MyAbstractClass implements MyInterface {
	}

which is then bound in C# as:

	// C#
	[Register (…)]
	public interface IMyInterface {
	    [Register(…)]
	    public void DoSomething() => …; // default interface method
	}

	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // Doesn't override IMyInterface.DoSomething()
	}

`generator` needs to ensure that `Method.IsInterfaceDefaultMethod()`
gets set for `IMyInterface.DoSomething()` so that the
`BoundClass.AddInterfaceAbstractMembers()` logic knows it does not
need to create an `abstract` method `MyAbstractClass.DoSomething()`.

This works correctly if the interface is in the assembly we are
binding, because it uses `XmlApiImporter`.  That is, `generator`
output is valid if `MyInterface` and `MyAbstractClass` are in the
same `.jar`.

However, if the interface is in a reference assembly -- `MyInterface`
and `MyAbstractClass` are in different assemblies -- then `generator`
uses `CecilApiImporter`, and our Cecil importer only marks Java
Interface Default Methods if the method has a
`[JavaInterfaceDefaultMethodAttribute]` custom attribute, which we
don't appear to ever emit.

Consequently, when `MyInterface` and `MyAbstractClass` are in
separate libraries, then `MyAbstractClass` is bound as:

	// C#
	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    public abstract void DoSomething();
	}

which causes most bindings that inherit `MyAbstractClass` to fail,
as they typically don't override the interface default method:

	// C#
	[Register (…)]
	public class MyClass : MyAbstractClass {
	}
	// error CS0534: 'MyClass' does not implemented inherited abstract member 'MyAbstractClass.DoSomething()'


Update `generator` and `CecilApiImporter` to properly detect C#8
default interface methods without requiring the presence of the
(unused!) `[JavaInterfaceDefaultMethodAttribute]` custom attribute,
so that classes don't re-declare C#8 interface default methods as
"new" abstract methods.  This fixes the generated `MyAbstractClass`
declaration to be:

	// C*
	[Register (…)]
	public class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // No `DoSomething()` declaration
	}

@jonpryor jonpryor merged commit 2244407 into master Jan 5, 2021
@jonpryor jonpryor deleted the cecil-dim branch January 5, 2021 19:45
jonpryor pushed a commit that referenced this pull request Jan 5, 2021
Context: dotnet/android-libraries#235

Consider the following Java API:

	// Java
	public interface MyInterface {
	    default void doSomething() {}
	}

	public class MyAbstractClass implements MyInterface {
	}

which is then bound in C# as:

	// C#
	[Register (…)]
	public interface IMyInterface {
	    [Register(…)]
	    public void DoSomething() => …; // default interface method
	}

	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // Doesn't override IMyInterface.DoSomething()
	}

`generator` needs to ensure that `Method.IsInterfaceDefaultMethod()`
gets set for `IMyInterface.DoSomething()` so that the
`BoundClass.AddInterfaceAbstractMembers()` logic knows it does not
need to create an `abstract` method `MyAbstractClass.DoSomething()`.

This works correctly if the interface is in the assembly we are
binding, because it uses `XmlApiImporter`.  That is, `generator`
output is valid if `MyInterface` and `MyAbstractClass` are in the
same `.jar`.

However, if the interface is in a reference assembly -- `MyInterface`
and `MyAbstractClass` are in different assemblies -- then `generator`
uses `CecilApiImporter`, and our Cecil importer only marks Java
Interface Default Methods if the method has a
`[JavaInterfaceDefaultMethodAttribute]` custom attribute, which we
don't appear to ever emit.

Consequently, when `MyInterface` and `MyAbstractClass` are in
separate libraries, then `MyAbstractClass` is bound as:

	// C#
	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    public abstract void DoSomething();
	}

which causes most bindings that inherit `MyAbstractClass` to fail,
as they typically don't override the interface default method:

	// C#
	[Register (…)]
	public class MyClass : MyAbstractClass {
	}
	// error CS0534: 'MyClass' does not implemented inherited abstract member 'MyAbstractClass.DoSomething()'


Update `generator` and `CecilApiImporter` to properly detect C#8
default interface methods without requiring the presence of the
(unused!) `[JavaInterfaceDefaultMethodAttribute]` custom attribute,
so that classes don't re-declare C#8 interface default methods as
"new" abstract methods.  This fixes the generated `MyAbstractClass`
declaration to be:

	// C*
	[Register (…)]
	public class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // No `DoSomething()` declaration
	}
jonpryor added a commit that referenced this pull request Jan 5, 2021
jonpryor pushed a commit that referenced this pull request Jan 5, 2021
Context: dotnet/android-libraries#235

Consider the following Java API:

	// Java
	public interface MyInterface {
	    default void doSomething() {}
	}

	public class MyAbstractClass implements MyInterface {
	}

which is then bound in C# as:

	// C#
	[Register (…)]
	public interface IMyInterface {
	    [Register(…)]
	    public void DoSomething() => …; // default interface method
	}

	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // Doesn't override IMyInterface.DoSomething()
	}

`generator` needs to ensure that `Method.IsInterfaceDefaultMethod()`
gets set for `IMyInterface.DoSomething()` so that the
`BoundClass.AddInterfaceAbstractMembers()` logic knows it does not
need to create an `abstract` method `MyAbstractClass.DoSomething()`.

This works correctly if the interface is in the assembly we are
binding, because it uses `XmlApiImporter`.  That is, `generator`
output is valid if `MyInterface` and `MyAbstractClass` are in the
same `.jar`.

However, if the interface is in a reference assembly -- `MyInterface`
and `MyAbstractClass` are in different assemblies -- then `generator`
uses `CecilApiImporter`, and our Cecil importer only marks Java
Interface Default Methods if the method has a
`[JavaInterfaceDefaultMethodAttribute]` custom attribute, which we
don't appear to ever emit.

Consequently, when `MyInterface` and `MyAbstractClass` are in
separate libraries, then `MyAbstractClass` is bound as:

	// C#
	[Register (…)]
	public abstract class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    public abstract void DoSomething();
	}

which causes most bindings that inherit `MyAbstractClass` to fail,
as they typically don't override the interface default method:

	// C#
	[Register (…)]
	public class MyClass : MyAbstractClass {
	}
	// error CS0534: 'MyClass' does not implemented inherited abstract member 'MyAbstractClass.DoSomething()'


Update `generator` and `CecilApiImporter` to properly detect C#8
default interface methods without requiring the presence of the
(unused!) `[JavaInterfaceDefaultMethodAttribute]` custom attribute,
so that classes don't re-declare C#8 interface default methods as
"new" abstract methods.  This fixes the generated `MyAbstractClass`
declaration to be:

	// C*
	[Register (…)]
	public class MyAbstractClass : Java.Lang.Object, IMyInterface {
	    // No `DoSomething()` declaration
	}
@jpobst jpobst added this to the 11.1 (16.9 / 8.9) milestone Jan 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants