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

Expand managedOverride for none #981

Closed
jpobst opened this issue May 9, 2022 · 4 comments · Fixed by #1000
Closed

Expand managedOverride for none #981

jpobst opened this issue May 9, 2022 · 4 comments · Fixed by #1000
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Milestone

Comments

@jpobst
Copy link
Contributor

jpobst commented May 9, 2022

In #701, we created the managedOverride metadata which allows a user to override which of virtual/override keywords are placed on a method:

<attr path="//method[@name='example']" name="managedOverride">override</attr>
<attr path="//method[@name='example']" name="managedOverride">virtual</attr>

However, sometimes the desired fix is "none of the above". For example, there may not be a method to override, however the class is sealed so virtual is not a valid option either. Both keywords must be omitted.

We should add support for a third none option:

<attr path="//method[@name='example']" name="managedOverride">none</attr>
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels May 9, 2022
@jonpryor
Copy link
Member

Tangentially related is this TODO from a65d6fb:

The problem here is that we have:

public partial interface IAnnotatedType {
    // Contains default interface method
    virtual unsafe IAnnotatedType? AnnotatedOwnerType =>;
}
public partial interface IAnnotatedArrayType : IAnnotatedType {
    // Contains *no* method body; re-abstracted
    IAnnotatedType? AnnotatedOwnerType {get;}
}

TODO: figure out how to properly fix this. managedOverride
metadata (5a0e37e) doesn't seem useful to "re-abstract" a default
interface member.

The solution is to "reabstract" the member:

public partial interface IAnnotatedType {
    // Contains default interface method
    virtual IAnnotatedType? AnnotatedOwnerType => null;
}
public partial interface IAnnotatedArrayType : IAnnotatedType {
    // Contains *no* method body; re-abstracted
    abstract IAnnotatedType? IAnnotatedType.AnnotatedOwnerType {get;}
}

The question is how to "reabstract" IAnnotatedType.AnnotatedOwnerType within generator & Metadata? A plausible answer may be to use propertyName to change AnnotatedArrayType.getAnnotatedOwnerType() to IAnnotatedType.AnnotatedOwnerType:

  <attr path="/api/package[@name='java.lang.reflect']/interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType' and count(parameter)=0]"
      name="propertyName">IAnnotatedType.AnnotatedOwnerType</attr>

This partially works:

public partial interface IAnnotatedArrayType : global::Java.Lang.Reflect.IAnnotatedType {
	global::Java.Lang.Reflect.IAnnotatedType? IAnnotatedType.AnnotatedOwnerType {
		// Metadata.xml XPath method reference: path="/api/package[@name='java.lang.reflect']/interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType' and count(parameter)=0]"
		[global::Java.Interop.JniMethodSignature ("getAnnotatedOwnerType", "()Ljava/lang/reflect/AnnotatedType;")]
		get; 
	}
}

However, it fails to compile:

Java.Lang.Reflect.IAnnotatedArrayType.cs(19,4): error CS0501: 
'IAnnotatedArrayType.IAnnotatedType.AnnotatedOwnerType.get' must declare a body because it is not marked abstract, extern, or partial

Thus, a fourth managedOverride value of abstract could be useful:

  <attr path="/api/package[@name='java.lang.reflect']/interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType' and count(parameter)=0]"
      name="managedOverride">abstract</attr>

Which, in combination with the propertyName override, would emit:

public partial interface IAnnotatedArrayType : global::Java.Lang.Reflect.IAnnotatedType {
	abstract global::Java.Lang.Reflect.IAnnotatedType? IAnnotatedType.AnnotatedOwnerType {
		// Metadata.xml XPath method reference: path="/api/package[@name='java.lang.reflect']/interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType' and count(parameter)=0]"
		[global::Java.Interop.JniMethodSignature ("getAnnotatedOwnerType", "()Ljava/lang/reflect/AnnotatedType;")]
		get; 
	}
}

(Another question on our call earlier today was whether or not this would be "enough". These two fixes are "enough" to get the current src/Java.Base to build, which is a data point, but I don't know for certain if setting propertyName to IAnnotatedType.AnnotatedOwnerType will always work.)

@jpobst
Copy link
Contributor Author

jpobst commented May 10, 2022

Thus, a fourth managedOverride value of abstract could be useful:

Is this not fixed by setting abstract to true?

<attr path="/api/package[@name='java.lang.reflect']/interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType' and count(parameter)=0]"
      name="abstract">true</attr>

@jonpryor
Copy link
Member

@jpobst asked:

Is this not fixed by setting abstract to true?

It's already true; obj/Debug-net6.0/mcw/api.xml.adjusted.fixed contains:

    <interface abstract="true" deprecated="not deprecated" final="false" name="AnnotatedArrayType" static="false" visibility="public" jni-signature="Ljava/lang/reflect/AnnotatedArrayType;">
      <implements name="java.lang.reflect.AnnotatedType" name-generic-aware="java.lang.reflect.AnnotatedType" jni-type="Ljava/lang/reflect/AnnotatedType;" />
      <method abstract="true" deprecated="not deprecated" final="false" name="getAnnotatedGenericComponentType" jni-signature="()Ljava/lang/reflect/AnnotatedType;" bridge="false" native="false" return="java.lang.reflect.AnnotatedType" jni-return="Ljava/lang/reflect/AnnotatedType;" static="false" synchronized="false" synthetic="false" visibility="public" />
      <method abstract="true" deprecated="not deprecated" final="false" name="getAnnotatedOwnerType" jni-signature="()Ljava/lang/reflect/AnnotatedType;" bridge="false" native="false" return="java.lang.reflect.AnnotatedType" jni-return="Ljava/lang/reflect/AnnotatedType;" static="false" synchronized="false" synthetic="false" visibility="public" propertyName="IAnnotatedType.AnnotatedOwnerType" />
    </interface>

Yet abstract isn't emitted, presumably because this is on an interface, and thus members are already assumed to be implicitly abstract?

@jpobst
Copy link
Contributor Author

jpobst commented Jun 28, 2022

I went with reabstract to denote this is only needed/supported for interface methods/properties. Normal class members should be controlled with the usual abstract='true' XML.

jonpryor pushed a commit that referenced this issue Jul 7, 2022
…1000)

Fixes: #981

Context: 5a0e37e

Add support for `//class/method[@managedOverride = 'none']` and
`//interface/method[@managedOverride = 'reabstract']`.

Setting `@managedOverride` to `none` ensures that the member is not
marked with `virtual` or `override`.  This is useful for `sealed`
classes, to avoid a [CS0549 error][0].  Previously, given the Java:

	// Java
	public final class MyClass {
	    public void doThing() {/* … */ }
	}

The `class-parse` XML would specify that `MyClass.doThing()` was
"virtual" -- `@abstract` is false, `@final` is false:

	<class name="MyClass …>
	  <method
	      abstract="false"
	      final="false"
	      name="doThing"
	      return="void"
	      … />
	</class>

This would result in the C# binding:

	// C#
	public sealed partial class MyClass {
	    public virtual void DoThing() => …
	}

which would error out with a CS0549:

	error CS0549: 'MyClass.DoThing()' is a new virtual member in sealed type 'MyClass'

This can now be resolved by setting `@managedOverride` to `none`:

	<attr path="//class[@name='MyClass']/method[@name='doThing']"
	    name="managedOverride">none</attr>

which will result in the compilable binding:

	// C#
	public sealed partial class MyClass {
	    public void DoThing() => …
	}

Setting `@managedOverride` to `reabstract` is part of support for
re-abstracting interface members; see also a65d6fb.  Currently in
`src/Java.Base`, we have Java:

	// Java
	public interface AnnotatedType {
	    default AnnotatedType getAnnotatedOwnerType() {…}
	}
	public interface AnnotatedArrayType implements AnnotatedType {
	    AnnotatedType getAnnotatedOwnerType(); // re-abstract interface default method
	}

which results in the C# binding:

	// C#
	public partial interface IAnnotatedType {
	    virtual IAnnotatedType? AnnotatedOwnerType {
	        get => …
	    }
	}
	public partial interface IAnnotatedArrayType : IAnnotatedType {
	    IAnnotatedType? AnnotatedOwnerType { get; }     // CS0108
	}

which results in a [warning CS0108][1]:

	warning CS0108: 'IAnnotatedArrayType.AnnotatedOwnerType' hides inherited member
	'IAnnotatedType.AnnotatedOwnerType'. Use the new keyword if hiding was intended.

Fixing this requires two steps.  First, we can now set
`//interface/method/@managedOverride` to `reabstract`:

	<attr path="//interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType']"
	    name="managedOverride">reabstract</attr>

What we *also* need to do is make the member *explicitly qualified*.
This can be "forced" for *properties* by setting `@propertyName`:

	<attr path="//interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType']"
	    name="propertyName">IAnnotatedType.AnnotatedOwnerType</attr>

`@managedOverride` and `@propertyName` work together to emit:

	public partial interface IAnnotatedArrayType : IAnnotatedType {
	    abstract IAnnotatedType? IAnnotatedType.AnnotatedOwnerType {get;}
	}

The problem is that this combination probably breaks Android output,
and it can't be used for *method* overrides, e.g. having
`java.io.Closeable.close()` re-abstract `java.lang.AutoCloseable.close()`.

TODO: complete the "interface reabstract" case, possibly via
`//interface/method[@explicitInterface='ManagedInterfaceName']`:

	<attr path="//interface[@name='AnnotatedArrayType']/method[@name='getAnnotatedOwnerType']"
	    name="explicitInterface">IAnnotatedType</attr>

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0549
[1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0108
jonpryor added a commit to dotnet/android that referenced this issue Jul 8, 2022
Fixes: dotnet/java-interop#335
Fixes: dotnet/java-interop#954
Fixes: dotnet/java-interop#976
Fixes: dotnet/java-interop#981
Fixes: dotnet/java-interop#992

Changes: dotnet/java-interop@7716ae5...c942ab6

  * dotnet/java-interop@c942ab68: [java-source-utils] Build one `$(TargetFramework)` (#1007)
  * dotnet/java-interop@b7caa788: [Byecode] Hide `internal` Kotlin fields marked with `@JvmField` (#1004)
  * dotnet/java-interop@22d5687b: [generator] Add `@managedOverride` values `none` and `reabstract`. (#1000)
  * dotnet/java-interop@7f1d2d7a: [generator] Fix <remove-attr/> metadata (#999)
  * dotnet/java-interop@265ad762: [Java.Interop.Tools.JavaSource] Fix tag parsing errors (#997)
  * dotnet/java-interop@99c68a86: [Java.Interop] JniTypeSignature & CultureInfo, empty strings (#1002)
  * dotnet/java-interop@920ea648: [Java.Interop] optimize JniTypeManager.AssertSimpleReference() (#1001)
  * dotnet/java-interop@4b4fedd3: [generator] Process Javadoc for nested types (#996)

Ever since commit 2197a45, CI builds for the `main` branch have been
incredibly flakey: 918613d through 27647d2 all failed to complete
the **Mac** stage -- which *must* pass in order for most unit tests
to run -- then dbadf13 built, then f149c25 failed.

Nine commits in the past week have failed to adequately build.
(PR builds, meanwhile, were largely fine.)

Most of these failures are from `make all-tests`:

	_ExtractJavadocsFromJavaSourceJars:
	  /Users/runner/android-toolchain/jdk-11/bin/java -jar /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar @/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp1cf0947e.tmp 
	JAVASOURCEUTILS : warning : Invalid or corrupt jarfile /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj]
	…
	BINDINGSGENERATOR : warning BG8600: Invalid XML file '/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml': Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml".  For details, see verbose output. [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj]
	  System.IO.FileNotFoundException: Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml"
	…
	"/Users/runner/work/1/s/xamarin-android/Xamarin.Android-Tests.sln" (default target) (1:2) ->
	"/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj" (default target) (12:6) ->
	(CoreCompile target) -> 
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(78,37): error CS1061: 'DataEventArgs' does not contain a definition for 'FromNode' and no accessible extension method 'FromNode' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(79,40): error CS1061: 'DataEventArgs' does not contain a definition for 'FromChannel' and no accessible extension method 'FromChannel' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(80,40): error CS1061: 'DataEventArgs' does not contain a definition for 'PayloadType' and no accessible extension method 'PayloadType' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(81,28): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(82,29): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(84,51): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(85,10): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]

`java-source-utils.jar` is (apparently) corrupt, so
`javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml` is never created,
which means no parameter name information is present, which means the
generated `DataEventArgs` type doesn't have the appropriate property
names, as the property names come from parameter names, and since we
don't have parameter names we instead have property names like `P0`,
`P1`, `P2`, etc.

Why, then, is `java-source-utils.jar` corrupt?  The current guess is
that it is being built *concurrently*; from a
`msbuild-20220706T175333-leeroy-all.binlog` build log file from one
of the offending builds, we see:

	17:53:44.526 59:11>Target "_BuildJava: (TargetId:490)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	17:53:44.526 59:11>Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/33.0.0/tools/java-source-utils.jar" does not exist.
	…
	17:54:19.564 59:12>Target "DispatchToInnerBuilds: (TargetId:1637)" in file "/Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/sdk/7.0.100-preview.7.22354.2/Microsoft.Common.CrossTargeting.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (target "Build" depends on it):
	  Task "MSBuild" (TaskId:1099)
	    Task Parameter:BuildInParallel=True (TaskId:1099)
	    Task Parameter:Targets=Build (TaskId:1099)
	    Task Parameter:
	        Projects=
	            java-source-utils.csproj
	                    AdditionalProperties=TargetFramework=net7.0 (TaskId:1099)
	    Additional Properties for project "java-source-utils.csproj": (TaskId:1099)
	      TargetFramework=net7.0 (TaskId:1099)
	…
	17:54:19.592 59:12>Target "_BuildJava: (TargetId:1640)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	  Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/../../bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar" does not exist.
	…
	17:54:41.034 59:11>Done building target "_BuildJava" in project "java-source-utils.csproj".: (TargetId:490)
 
What appears to be happening -- and there is lots of conjecture here,
as the build log is enormous and it's difficult to piece everything
together -- is that `java-source-utils.csproj` is built *twice*:
Once via `Xamarin.Android.sln`, and once via `apksigner.csproj`, as
it appears that setting `%(PackageReference.AdditionalProperties)`
triggers a *nested build*; note the `DispatchToInnerBuilds` target
which invokes the `<MSBuild/>` Task with the specified
`AdditionalProperties`.

However, that's not the only potential source of concurrent builds;
`java-source-utils.csproj` used `$(TargetFrameworks)` (plural), which
can *also* result in concurrent builds between the "outer" and "inner"
builds used as part of `$(TargetFrameworks)`.

Fix this problem by bumping to dotnet/java-interop@c942ab68, which
updates `java-source-utils.csproj` to use the singular
`$(TargetFramework)` property -- avoiding "outer" and "inner" build
problems -- and "for good measure" update `apksigner.csproj` to
remove `%(ProjectReference.AdditionalProperties)` on
`java-source-utils.csproj`.  This should ensure that
`java-source-utils.csproj` is only built *once*, which in turn should
ensure that `java-source-utils.jar` isn't corrupted, and our CI
builds become more reliable
@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
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants