Skip to content

[generator] Ensure we don't generate unexpected NRT types, like 'void?'. #856

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
Jul 12, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jul 9, 2021

Context: #850

When a user binds a library that contains an interface that implements another interface, we pull in all invokable members from both interfaces to create the Invoker type for the user's interface. If the base interface is a referenced assembly, the information we are working from has been read in via Cecil. Due to a bug caused by the combination of Cecil-read info and NRT generation, we will interpret the return types on the imported methods as always nullable, even for types like void, which results in the uncompilable void?.

Example from AndroidX.Core.Core:

// In AndroidX.Core.Core
public interface AndroidX.Core.Internal.View.ISupportMenu : Android.Views.IMenu { ... }

// In Mono.Android.dll
public interface Android.Views.IMenu {
  public void Clear ();
}

This generates the ISupportMenuInvoker type:

internal partial class ISupportMenuInvoker : global::Java.Lang.Object, ISupportMenu {
  public unsafe void? Clear () { ... }
}

Which cause these following CSC errors:

Error CS1519 Invalid token '?' in class, record, struct, or interface member declaration
Error CS1520 Method must have a return type
Error CS0535 'ISupportMenuInvoker' does not implement interface member 'IMenu.Clear()'

The culprit is twofold:

  • In CecilApiImporter, we set the Method.ManagedReturn to System.Void instead of void.
  • In CodeGenerationOptions, we only check for void to omit the null operator, not System.Void.

Note this also happens for all primitive types like System.Int32/int.

This commit fixes both aspects:

  • Change Method.ManagedReturn to contain primitive types instead of fully qualified types.
  • Update GetNullable to correctly handle fully qualified types if one slips through.

With this change, all of AndroidX/GPS/FB/MLKit can be successfully compiled with <Nullable>enable</Nullable>.

@jpobst jpobst marked this pull request as ready for review July 9, 2021 18:56
@jonpryor jonpryor merged commit 855ecfa into main Jul 12, 2021
@jonpryor jonpryor deleted the nullable-cecil-assemblies branch July 12, 2021 20:05
jpobst added a commit that referenced this pull request Sep 30, 2021
Context: #850 (comment)

When a user binds a Java library that contains a Java interface which
implements another Java interface:

	// Java; android.jar
	public interface /* android.view. */ Menu {…}

	// Java; androidx.core.core.jar
	public interface /* androidx.core.internal.view. */ SupportMenu implements android.view.Menu {…}

then when we create the binding for the "leaf" interface:

	// C# binding for androidx.core.core
	namespace AndroidX.Core.Internal.View {
	    public interface ISupportMenu : Android.Views.IMenu {…}

	    internal partial class ISupportMenuInvoker : Java.Lang.Object, ISupportMenu {
	        // …
	    }
	}

The generated `*Invoker` type implements all methods for all
implemented interfaces, e.g. methods from both `IMenu` and
`ISupportMenu`.

When:

 1. The base interface (e.g. `IMenu`) comes from a referenced
    assembly, *and*

 2. `$(Nullable)`=Enable when binding the derived interface

then we interpret the return types on imported interface methods as
*always* being of a nullable type, even for types such as `void`:

	// C# binding for androidx.core.core with $(Nullable)=Enable
	partial class ISupportMenuInvoker : Java.Lang.Object, ISupportMenu {

	    public unsafe void? Clear () {…}
	}

This results in errors from the C# compiler:

	Error CS1519: Invalid token '?' in class, record, struct, or interface member declaration
	Error CS1520: Method must have a return type
	Error CS0535: 'ISupportMenuInvoker' does not implement interface member 'IMenu.Clear()'

The culprit is twofold:

  - In `CecilApiImporter`, we set `Method.ManagedReturn` to
    `System.Void` instead of `void`.

  - In `CodeGenerationOptions`, we only check for `void` to omit the
    null operator, not `System.Void`.

Note this also happens for all primitive types like
`System.Int32`/`int`.

This commit fixes both aspects:

  - Change `Method.ManagedReturn` to contain primitive types instead
    of fully qualified types.

  - Update `CodeGenerationOptions.GetNullable()` to correctly handle
    fully qualified types if one slips through.

With this change, all of AndroidX/GooglePlayServices/FaceBook/MLKit
can be successfully compiled with `<Nullable>enable</Nullable>`.
@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