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

[generator] Handle multiple @return javadoc values #836

Merged
merged 2 commits into from
May 12, 2021

Conversation

pjcollins
Copy link
Member

Context: dotnet/android#5485

In an attempt to generate updated documentation for API 30 I've noticed
a minor issue in generator. When a <javadoc> element contains
multiple @return values, we will generate multiple <returns> elements
in our C# documentation. This results in invalid C# documentation with
partially missing return information, and the second <returns> element
value is not displayed in IDE intellisense. This also causes an issue
when the xml is processed by mdoc. These extra return lines will
continue to be appended to the mdoc output every time the tool is ran
against the invalid Mono.Android.xml.

Context: dotnet/android#5485

In an attempt to generate updated documentation for API 30 I've noticed
a minor issue in generator.  When a `<javadoc>` element contains
multiple @return values, we will generate multiple `<returns>` elements
in our C# documentation.  This results in invalid C# documentation with
partially missing return information, and the second `<returns>` element
value is not displayed in IDE intellisense.  This also causes an issue
when the xml is processed by `mdoc`.  These extra return lines will
continue to be appended to the `mdoc` output every time the tool is ran
against the invalid `Mono.Android.xml`.
@pjcollins pjcollins requested review from jonpryor and jpobst May 11, 2021 21:47
@pjcollins pjcollins marked this pull request as draft May 11, 2021 22:32
@pjcollins pjcollins marked this pull request as ready for review May 11, 2021 22:52

} else {
var r = jdi.Returns.First () as XElement;
if (r != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This can't be null; .First() requires that an element be returned, and unless we have a bug and we do .Returns.Add(null), there should only be non-null elements within jdi.Returns.

If we want to allow null, use .FirstOrDefault().

Actually, on review this entire block doesn't quite make sense to me: r itself shouldn't change from r.Add(), and thus this should be a no-op (unless jdi.Returns somehow has > 1 element in it? But how could that happen?)

FinishParse (context, parseNode).Returns.Clear ();
FinishParse (context, parseNode).Returns.Add (r);

Neither of the above statements should be needed.

Copy link
Member Author

@pjcollins pjcollins May 12, 2021

Choose a reason for hiding this comment

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

The null check was intended to handle the possibility of jdi.Returns.First () not being an XElement, though I think we only ever add XElements to this ICollection<XNode> in the section above and so the check probably shouldn't be needed.

The FinishParse (context, parseNode).Returns collection will have more than zero elements in it when the <javadoc> entry that is being parsed has multiple @return values in it, as shown in the test addition below. The Returns.Clear and Returns.Add additions do look to be redundant here though, I will clean that up.

</member>",
},
new ParseResult {
Javadoc = "@return {@code true} if something else @return {@code false}.",
Copy link
Member

Choose a reason for hiding this comment

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

I'm half surprised that @tag parsing isn't requiring newlines in there, and that we don't need \n@return …

@jonpryor
Copy link
Member

Another test case I'm concerned about properties: https://github.com/xamarin/java.interop/blob/12e670a8560f69581d3a3adf0a9d91e8ce8c9afa/tools/generator/SourceWriters/BoundProperty.cs#L194-L209

Properties may contain the docs for two separate methods, the get and set methods, and if "somehow" there are > 2 @remarks blocks between the two methods, I don't think they're be "fully" merged, as there could be multiple <remarks/> elements under both mergeInto and mergeFrom!

…though perhaps now after the fix within this PR, which should limit things to only one <remarks/> per method…

@jonpryor
Copy link
Member

jonpryor commented May 12, 2021

For review:

Context: https://github.com/xamarin/xamarin-android/pull/5485

In an attempt to generate updated documentation for API-30 I noticed
a minor issue in generator Javadoc import (7574f166): when a
`<javadoc/>` element contains multiple `@return` values:

	<class name="Example" …>
	  <method name="m" …>
	    <javadoc>
	      <![CDATA[
	
	        @return some docs
	        @return additional docs
	      ]]>
	    </javadoc>
	  </method>
	</class>


We would generate multiple `<returns/>` elements, one for each
`@return`:

	partial class Example {
	    /// <returns>some docs</returns>
	    /// <returns>additional docs</returns>
	    public void M() {…}
	}

This is "fine", as far as the C# compiler is concerned, which results
in a Documentation XML file containing both `<returns/>`:

	<doc>
	  <members>
	    <member name="M:Example.M()">
	      <returns>some docs</returns>
	      <returns>additional docs</returns>
	    </member>
	  </members>
	</doc>

The problem is when we later try to *import* this documentation via
`mdoc update --import`, as xamarin/xamarin-android#5485 does;
if mdoc 5.7.4.9 is used (included with macOS Mono 6.12):

	$ mdoc update -o docs assembly.dll --import assembly.xml

The resulting imported [**mdoc**(5)][0] documentation only contains
the *first* element:

	<Docs>
	  <summary>To be added.</summary>
	  <returns>some docs</returns>
	  <remarks>To be added.</remarks>
	</Docs>

Using [mdoc 5.8.0][1] will preserve both `<returns/>` elements, but
that doesn't mean that "higher level" tooling such as HTML rendering
or IntelliSense will properly support this construct.

Update the Javadoc importer so that multiple `@returns` blocks are
*merged* into a single `<returns/>` element:

	partial class Example {
	    /// <returns>some docs additional docs</returns>
	    public void M() {…}
	}

[0]: http://docs.go-mono.com/?link=man%3amdoc(5)
[1]: https://www.nuget.org/packages/mdoc/5.8.0

@jonpryor jonpryor merged commit 0e01fb5 into main May 12, 2021
@jonpryor jonpryor deleted the javadoc-ignore-returns branch May 12, 2021 22:28
@jpobst jpobst added this to the 11.4 (16.11 / 8.11) milestone Jun 15, 2021
@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.

3 participants