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.

Sign up for GitHub

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

Jump to bottom

generator should pull parameter names from Javadoc #767

Closed
jonpryor opened this issue Dec 18, 2020 · 1 comment · Fixed by dotnet/android#5468
Closed

generator should pull parameter names from Javadoc #767

jonpryor opened this issue Dec 18, 2020 · 1 comment · Fixed by dotnet/android#5468
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Related? PR #687.

Parameter names remains an ongoing concern. "Best" is if the Java code is built with javac -parameters, which allows class-parse to extract parameter names, and everything is good.

Unfortunately, that still appears to be a rarity in the Java world.

Lacking javac -parameters, how should bindings get decent parameter names? A "reasonable" answer is to integrate @(JavaSourceJar) support, which with PR #687 and dotnet/android#5253 will parse @(JavaSourceJar) with java-source-utils.jar (69e1b80) to extract Javadoc and parameter names.

However, PR #687 is focused on parsing the Javadoc comments and translating them into C# XML Documentation Comments.

We should also look into using generator --with-javadoc-xml=FILE to determine parameter names as well.

Then there's the "priority" question: I think it would be most "sensible" and documentable if the last "source of truth" for parameter names was used in the bindings:

  1. class-parse output.
  2. java-source-utils.jar output
  3. Metadata.xml updates

This would allow Metadata.xml to always override.
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Dec 18, 2020
@jonpryor
Copy link
Member Author

jonpryor commented Jan 5, 2021

On further consideration, I don't think this requires any changes to generator.

Instead, we should update the _ExportJarToXml target so that the output of java-source-utils.jar is provided to ClassParse.DocumentationPaths. This will cause java-source-utils.jar output to be treated identically to Javadoc HTML input files regarding parameter names.

jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jan 6, 2021
@jonpryor
Rethink <JavaSourceUtils/> ordering
4d937fc 
Context: dotnet/java-interop#767
Context: dotnet#5253 (comment)

dotnet/java-interop#767 suggests updating `generator` to use Javadoc
output to determine parameter names.  However, we don't need to do
that, as `class-parse` *already* has (partial) support for that.

Split the `<JavaSourceUtils/>` invocation out into a new
`_ExtractJavadocsFromJavaSourceJars` target invocation, and give it
a "proper" output item group.  This allows it to be executed in
 an incremental manner, which wasn't previously the case.

Additionally, the previous `<JavaSourceUtils/>` invocation "minimized"
the number of Javadoc XML output files based on the "unique" values
of `%(CopyrightFile)`/etc.  Thus, if you had multiple
`@(JavaSourceJar)`s with the same (or no) `%(CopyrightFile)` file,
they'd all be present in the same output XML.

While this was "nice" in that it reduced the number of files running
around, it complicated coming up with a separate item group for
incremental build purposes.

Remove the "nicety" and go for "simplicity": one Javadoc XML per
`@(JavaSourceJar)` file.  Period.

Add a `$(AndroidJavaSourceUtilsJar)` MSBuild property which controls
where `java-source-utils.jar` is present.  This is consistent with
other tasks.

However, *don't* allow `class-parse` to use `java-source-utils` output,
as that doesn't actually work yet.  Doh.
jonpryor added a commit to jonpryor/java.interop that referenced this issue Jan 6, 2021
@jonpryor
[Xamarin.Android.Tools.Bytecode] Relax _ApiXml check
996692d 
Context: dotnet#767
Context: https://github.com/xamarin/xamarin-android/blob/a7413a2389886082c3d3422c50a7e6cc84f43d8f/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Bindings.ClassParse.targets#L23

Commit 69e1b80 added `java-source-utils.jar`, which can parse Java
source code to extract parameter names, storing them into a
"`class-parse`-like XML file".

A benefit to a "`class-parse`-like XML file" is that it should be
possible to use this same file with `class-parse --docspath`,
overriding parameter names present in Java bytecode (which typically
*isn't* present, as `javac -parameters` is rarely used).

Unfortunately, this doesn't work: if you attempt to use
`java-source-utils.jar` output w/ `class-parse --docspath`, it fails:

	System.Exception: Directory 'example.xml' does not exist
	  at Xamarin.Android.Tools.Bytecode.AndroidDocScraper..ctor (System.String dir, System.String patternHead, System.String resetPatternHead, System.String parameterPairSplitter, System.Boolean continuousParamLines, System.String openMethod, System.String paramSep, System.String closeMethod, System.String postCloseMethodParens) [0x00093] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.AndroidDocScraper..ctor (System.String dir, System.String patternHead, System.String resetPatternHead, System.String parameterPairSplitter, System.Boolean continuousParamLines) [0x00000] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.DroidDocScraper..ctor (System.String dir) [0x00000] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.ClassPath.CreateDocScraper (System.String src) [0x00037] in <e7fad12ab9c24ce08dac8732f18e1d85>:0

The problem is that we shouldn't be creating a `DroidDocScraper`
for `java-source-utils.jar` output, it should be creating a
`ApiXmlDocScraper`!

We're creating the wrong `*Scraper` type because
`JavaMethodParameterNameProvider.GetDocletType()` doesn't detect
`java-source-utils.jar` output as being `JavaDocletType._ApiXml`;
instead, it treats it as the default of `JavaDocletType.DroidDoc`.

`JavaMethodParameterNameProvider.GetDocletType()` doesn't detect
`java-source-utils.jar` output as being `JavaDocletType._ApiXml`,
because it requires that the XML contain:

	<api>

while `java-source-utils.jar` instead emits:

	<api api-source="java-source-utils">

Relax `JavaMethodParameterNameProvider.GetDocletType()` to instead
check for `<api`.  This allows `class-parse` to use
`java-source-utils.jar` output for parameter names.
@jonpryor jonpryor mentioned this issue Jan 6, 2021
Merged
jonpryor added a commit that referenced this issue Jan 6, 2021
@jonpryor
[Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (#772)
fdc200c 
Context: #767
Context: https://github.com/xamarin/xamarin-android/blob/a7413a2389886082c3d3422c50a7e6cc84f43d8f/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Bindings.ClassParse.targets#L23

Commit 69e1b80 added `java-source-utils.jar`, which can parse Java
source code to extract parameter names, storing them into a
"`class-parse`-like XML file".

A benefit to a "`class-parse`-like XML file" is that it should be
possible to use this same file with `class-parse --docspath`,
overriding parameter names present in Java bytecode (which typically
*isn't* present, as `javac -parameters` is rarely used).

Unfortunately, this doesn't work: if you attempt to use
`java-source-utils.jar` output w/ `class-parse --docspath`, it fails:

	System.Exception: Directory 'example.xml' does not exist
	  at Xamarin.Android.Tools.Bytecode.AndroidDocScraper..ctor (System.String dir, System.String patternHead, System.String resetPatternHead, System.String parameterPairSplitter, System.Boolean continuousParamLines, System.String openMethod, System.String paramSep, System.String closeMethod, System.String postCloseMethodParens) [0x00093] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.AndroidDocScraper..ctor (System.String dir, System.String patternHead, System.String resetPatternHead, System.String parameterPairSplitter, System.Boolean continuousParamLines) [0x00000] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.DroidDocScraper..ctor (System.String dir) [0x00000] in <e7fad12ab9c24ce08dac8732f18e1d85>:0
	  at Xamarin.Android.Tools.Bytecode.ClassPath.CreateDocScraper (System.String src) [0x00037] in <e7fad12ab9c24ce08dac8732f18e1d85>:0

The problem is that we shouldn't be creating a `DroidDocScraper`
for `java-source-utils.jar` output, it should be creating a
`ApiXmlDocScraper`!

We're creating the wrong `*Scraper` type because
`JavaMethodParameterNameProvider.GetDocletType()` doesn't detect
`java-source-utils.jar` output as being `JavaDocletType._ApiXml`;
instead, it treats it as the default of `JavaDocletType.DroidDoc`.

`JavaMethodParameterNameProvider.GetDocletType()` doesn't detect
`java-source-utils.jar` output as being `JavaDocletType._ApiXml`,
because it requires that the XML contain:

	<api>

while `java-source-utils.jar` instead emits:

	<api api-source="java-source-utils">

Relax `JavaMethodParameterNameProvider.GetDocletType()` to instead
check for `<api`.  This allows `class-parse` to use
`java-source-utils.jar` output for parameter names.
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jan 6, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc
a0ea50d 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (dotnet#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (dotnet#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (dotnet#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (dotnet#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.
@jonpryor jonpryor mentioned this issue Jan 6, 2021
Merged
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jan 6, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc
01fcfed 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (dotnet#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (dotnet#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (dotnet#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (dotnet#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.
jonpryor added a commit to dotnet/android that referenced this issue Jan 7, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc (#5468)
0e95ec7 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.

We can see the benefits of this change in
`tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs`,
which required changes because the parameter names in the Java
`DataListener.onDataReceived()` method could now be determined;
previously they couldn't, resulting in the `P0`/`P1`/etc. names.
With the provision of `@(JavaSourceJar)` -- a7413a2 updated
`Xamarin.Android.McwGen-Tests.csproj` to have `@(JavaSourceJar)` --
the parameter names can now be determined, improving the binding.
@jonpryor jonpryor mentioned this issue Nov 6, 2021
Merged
@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.
Assignees
No one assigned
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Bump to xamarin/Java.Interop/master@fdc200cc jonpryor/xamarin-android
2 participants
@jonpryor @jpobst