Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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