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

[Java.Base, generator] Bind all of package java.io #968

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Apr 13, 2022

Context: bc5bcf4

Bind all classes and interfaces in the java.io package.

Binding the java.io.StringBufferInputStream.buffer field
exposed a generator bug: property setters for String fields would
not compile, producing:

partial class StringBufferInputStream {
    protected string? Buffer {
        get { ... }
        set {
            const string __id = "buffer.Ljava/lang/String;";

            var native_value = global::Java.Interop.JniEnvironment.Strings.NewString (value);
            try {
                _members.InstanceFields.SetValue (__id, this, native_value?.PeerReference ?? default);
            } finally {
                global::Java.Interop.JniObjectReference.Dispose (ref native_value);
                GC.KeepAlive (value);
            }
        }
    }
}

Specifically, native_value?.PeerReference is not valid, and
unnecessary. This was due to a bug in BoundFieldAsProperty.cs,
as String field marshaling hit the "Object" marshaling path, which
was not warranted in this case, as String is treated specially.

Fix this bug.

Binding [java.io.ObjectOutputStream.PutField.write(ObjectOutput)][2]
presented a larger binding problem: it's an abstract method on an
abstract class, but the PutFieldInvoker.Write() method was
[Obsolete] when PutField.Write() was not!

public partial class /* ObjectOutputStream. */ PutField {
    public abstract void Write (Java.IO.IObjectOutput? p0);
}
internal partial class /* ObjectOutputStream. */ PutFieldInvoker : PutField {
    [Obsolete (@"deprecated")]
    public override void Write (Java.IO.IObjectOutput? p0) => ...
}

This state of affairs triggers a CS0809 warning, which is an error in
Release configuration builds:

##[error]src/Java.Base/obj/Release-net6.0/mcw/Java.IO.ObjectOutputStream.cs(311,32)
Error CS0809: Obsolete member 'ObjectOutputStream.PutFieldInvoker.Write(IObjectOutput?)' overrides non-obsolete member 'ObjectOutputStream.PutField.Write(IObjectOutput?)'

Turns Out that BoundMethodAbstractDeclaration.cs never "forwarded"
deprecated attributes? Meaning that abstract methods would never
be [Obsolete], even if their Java peer was @deprecated?

Update BoundMethodAbstractDeclaration so that if the Java method is
deprecated, the bound abstract method is [Obsolete]. This fixes
the CS0809, as now PutField.Write() and PutFieldInvoker.Write()
are consistent with each other.

TODO: should this change be done for all codegen targets?

Finally, in order to make it easier to view and review the
current Java.Base.dll API -- without requiring that it be built
locally -- add a _GenerateRefApi target to Java.Base.targets
which uses the Microsoft.DotNet.GenAPI NuGet package to generate
"reference assembly source" for Java.Base.dll. This is stored in
src\Java.Base-ref.cs.

@jonpryor jonpryor force-pushed the jonp-java.base-java.io branch 2 times, most recently from 4ed40ed to a080535 Compare April 13, 2022 19:31
Context: bc5bcf4

Bind all classes and interfaces in the `java.io` package.

Binding the [`java.io.StringBufferInputStream.buffer`][0] field
exposed a `generator` bug: property setters for `String` fields would
not compile, producing:

	partial class StringBufferInputStream {
	    protected string? Buffer {
	        get { ... }
	        set {
	            const string __id = "buffer.Ljava/lang/String;";

	            var native_value = global::Java.Interop.JniEnvironment.Strings.NewString (value);
	            try {
	                _members.InstanceFields.SetValue (__id, this, native_value?.PeerReference ?? default);
	            } finally {
	                global::Java.Interop.JniObjectReference.Dispose (ref native_value);
	                GC.KeepAlive (value);
	            }
	        }
	    }
	}

Specifically, `native_value?.PeerReference` is not valid, and
unnecessary.  This was due to a bug in `BoundFieldAsProperty.cs`,
as `String` field marshaling hit the "Object" marshaling path, which
was not warranted in this case, as `String` is treated specially.

Fix this bug.

Binding [`java.io.ObjectOutputStream.PutField.write(ObjectOutput)`][2]
presented a larger binding problem: it's an `abstract` method on an
`abstract` class, *but* the `PutFieldInvoker.Write()` method was
`[Obsolete]` when `PutField.Write()` was not!

	public partial class /* ObjectOutputStream. */ PutField {
	    public abstract void Write (Java.IO.IObjectOutput? p0);
	}
	internal partial class /* ObjectOutputStream. */ PutFieldInvoker : PutField {
	    [Obsolete (@"deprecated")]
	    public override void Write (Java.IO.IObjectOutput? p0) => ...
	}

This state of affairs triggers a CS0809 warning, which is an error in
Release configuration builds:

	##[error]src/Java.Base/obj/Release-net6.0/mcw/Java.IO.ObjectOutputStream.cs(311,32)
	Error CS0809: Obsolete member 'ObjectOutputStream.PutFieldInvoker.Write(IObjectOutput?)' overrides non-obsolete member 'ObjectOutputStream.PutField.Write(IObjectOutput?)'

Turns Out that `BoundMethodAbstractDeclaration.cs` never "forwarded"
deprecated attributes?  Meaning that `abstract` methods would *never*
be `[Obsolete]`, even if their Java peer was `@deprecated`?

Update `BoundMethodAbstractDeclaration` so that if the Java method is
deprecated, the bound `abstract` method is `[Obsolete]`.  This fixes
the CS0809, as now `PutField.Write()` and `PutFieldInvoker.Write()`
are consistent with each other.

TODO: should this change be done for *all* codegen targets?

Finally, in order to make it easier to view and review the
current `Java.Base.dll` API -- without requiring that it be built
locally -- add a `_GenerateRefApi` target to `Java.Base.targets`
which uses the `Microsoft.DotNet.GenAPI` NuGet package to generate
"reference assembly source" for `Java.Base.dll`.  This is stored in
`src\Java.Base-ref.cs`.

[0]: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/StringBufferInputStream.html#buffer
[1]: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/ObjectOutputStream.PutField.html#write(java.io.ObjectOutput)
@jonpryor jonpryor force-pushed the jonp-java.base-java.io branch from a080535 to 333c201 Compare April 13, 2022 19:37
@jonpryor jonpryor requested a review from jpobst April 13, 2022 19:58
@@ -52,6 +52,11 @@ public BoundMethodAbstractDeclaration (GenBase gen, Method method, CodeGeneratio
if (method.DeclaringType.IsGeneratable)
Comments.Add ($"// Metadata.xml XPath method reference: path=\"{method.GetMetadataXPathReference (method.DeclaringType)}\"");

// TODO: shouldn't `[Obsolete]` be added for *all* CodeGenerationTargets?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would make sense to always prevent that warning. In the real world it's nearly impossible to bind libraries with <TreatWarningsAsErrors>, but it's something we should continue working towards.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpobst: The implicit question here is whether all CodeGenerationTargets should add [Obsolete] to abstract methods, or just CodeGenerationTarget.JavaInterop1.

I opted to only update CodeGenerationTarget.JavaInterop1 for expediency, but I suspect this should be done for everything.

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