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

DokkaJavadoc does not respect JVMName #1284

Closed
bishiboosh opened this issue Aug 10, 2020 · 27 comments · Fixed by #1675
Closed

DokkaJavadoc does not respect JVMName #1284

bishiboosh opened this issue Aug 10, 2020 · 27 comments · Fixed by #1675
Labels
bug feedback: Google An issue/PR submitted by colleagues at Google, most likely related to the Android API reference docs
Milestone

Comments

@bishiboosh
Copy link
Contributor

Describe the bug
When using dokkaJavadoc, the class names or method names are the Kotlin ones, instead of being the ones specified by @JvmName

Expected behaviour
Use names specified in @JvmName when rendering to Javadoc.

To Reproduce
Using this sample project https://github.com/bishiboosh/dokka-issue-sample call the dokkaJavadoc task and see the generated documentation

Dokka configuration
No specific configuration

Installation

  • Operating system: Linux
  • Build tool: Gradle v6.1.1
  • Dokka version: 1.4.0-rc
@bishiboosh bishiboosh added the bug label Aug 10, 2020
@sellmair sellmair added this to the 1.4.0 milestone Aug 10, 2020
@Kordyjan Kordyjan modified the milestones: Stable, 1.4.20 Sep 1, 2020
@bishiboosh
Copy link
Contributor Author

@Kordyjan : any pointers on how this would be done ? We really need that as our internal Kotlin libs are used by both Java and Kotlin projects so having the right names in the Javadoc is important. I'd be happy to contribute a PR, but I'm not familiar with the Dokka codebase, so if you can point me in the right direction I may be able to help.

@MarcinAman MarcinAman added the feedback: Google An issue/PR submitted by colleagues at Google, most likely related to the Android API reference docs label Nov 12, 2020
@MarcinAman
Copy link
Contributor

Hi, sorry for the long response time.
Changing this would require adding some logic in JavadocSignatureProvider and in (probably) in SearchScriptCreator. Annotations are stored in extra properties and can be retrieved like this:

fun <T : Documentable> WithExtraProperties<T>.annotations(): SourceSetDependent<List<Annotations.Annotation>> =

You would need to change the displayed name when the annotation is in this extra.

Would you like to pick up this issue? And would you mind doing it also for the other formats?

@bishiboosh
Copy link
Contributor Author

@MarcinAman I'll try to see if I manage to do it for the Javadoc format, and I'll let you know.

When you talk about the other format, you mean GFM and so, when using kotlin-as-java ? If so, I'll probably need more pointers when doing so. I'm going to see if I can manage to begin this and I'll ask you when needed :)

@MarcinAman
Copy link
Contributor

MarcinAman commented Nov 13, 2020

Yes, since Javadoc has it's own signature provider, same goes for the base plugin (KotlinSignatureProvider and JavaSignatureProvider) and searching mechanisms for this plugin. Kotlin-as-java doesn't have it's own signature provider so it should work out of the box (it uses JavaSignatureProvider from the base plugin).

If you have any questions let me know, i'll try to explain how dokka works :D

@bishiboosh
Copy link
Contributor Author

@MarcinAman ok, I've managed to wrap my head a little bit around the code, so I have some questions:

  • if I understand clearly, JavadocSignatureProvider is used by the Javadoc plugin to compute the signature (which applies kotlin-as-java, per the doc), and JavaSignatureProvider is used by the Kotlin-as-Java plugin. As @JvmName specifies what name would appear when using from Java, wouldn't my changes need to be applied both to JavadocSignatureProvider and JavaSignatureProvider ? (KotlinSignatureProvider doesn't need to change, if I understand clearly, because it output the pure-Kotlin signature)
  • @JvmName can be applied at the @file level. Is this case handled when documentable is of DClasslike type ?
  • it can also be applied to getter and setters for properties. Am I correct in assuming that this is handled by the DFunction case ?
  • I'm a bit lost on what to change in SearchScriptCreator. Could you help me on this point ?
  • as for the other formats (GFM and Jekyll), as @JVMName only is relevant when using kotlin-as-java, isn't it enough to do changes in JavaSignatureProvider ? (this is basically the same questions as the first point)

Sorry for all the questions, the codebase is a little bit hard to digest and I'm usually an Android developer so most of these concepts are still pretty alien to me (but I'm still keen on trying to do this, if you think it's worth the try for me)

@bishiboosh
Copy link
Contributor Author

I'll add just one final question (before going on week-end, so feel free to respond when you have the time, no worries) : in the signatureForProjection methods in JavaSignatureProvider or JavadocSignatureProvider, how can I be sure that the name that'll be used will be the correct one, re @JVMName ?

@asfalcone
Copy link
Contributor

Maybe it is worth adding to this bug or I can file a new one, but @JvmSynthetic should be considered here as well. Kotlin-as-java should not show @JvmSynthetic functions so this annotation needs to be included in the metadata as well

@asfalcone
Copy link
Contributor

asfalcone commented Nov 13, 2020

When debugging a simple example I'm not able to find the @JvmName annotation in the list. It appears as

image

Example used:

@JvmName("bar")
fun foo() = Unit

@MarcinAman
Copy link
Contributor

@asfalcone could you point me to the location you have placed the function? It resolves correctly for me

@bishiboosh: i'll answer later since it requires some checking on how jvmName is handled on file-level

@asfalcone
Copy link
Contributor

@MarcinAman This example is coming from our integration and unit tests (which extend AbstractCoreTest and call testInline with a minimal Dokka configuration that has sources

@MarcinAman
Copy link
Contributor

@asfalcone I think that error was displayed due to the lack of stdlib on classpath, for me a test configuration like this works:

private val testConfiguration = dokkaConfiguration {
        sourceSets {
            sourceSet {
                sourceRoots = listOf("src/")
                analysisPlatform = "jvm"
                classpath += listOfNotNull(jvmStdlibPath)
            }
        }
    }

@bishiboosh

if I understand clearly, JavadocSignatureProvider is used by the Javadoc plugin to compute the signature (which applies kotlin-as-java, per the doc), and JavaSignatureProvider is used by the Kotlin-as-Java plugin. As @JvmName specifies what name would appear when using from Java, wouldn't my changes need to be applied both to JavadocSignatureProvider and JavaSignatureProvider ? (KotlinSignatureProvider doesn't need to change, if I understand clearly, because it output the pure-Kotlin signature)

Yes, i talked with other team members and you approach is correct. JavaSignatureProvider and JavadocSignatureProvider should be changed to handle this annotation. KotlinSignatureProvider should stay the same and display annotation as usual. The reason for this is: when you use a code with this annotation from kotlin you are using it by the original name, not the annotated one.

@JvmName can be applied at the @file level. Is this case handled when documentable is of DClasslike type ?

AFAIK they are not handled. Handling it on file level would require adding it when creating documentable from Descriptors in DefaultDescriptorToDocumentableTranslator

it can also be applied to getter and setters for properties. Am I correct in assuming that this is handled by the DFunction case ?

Yes, that is correct. DProperty has a getter and setter fields that should contain corresponding annotations regarding getters/setters.

I'm a bit lost on what to change in SearchScriptCreator. Could you help me on this point ?

This depends on your implementation but i think the name of the entry should be updated there to match the one from the annotation. This is minor, don't worry with it, just check if the item is searchable with the JvmName

@bishiboosh
Copy link
Contributor Author

@MarcinAman I've begun a little bit with DefaultDescriptorToDocumentableTranslator, did you have the time to look at this question ?

@bishiboosh
Copy link
Contributor Author

(and thanks for all your answers, and sorry for all my questions, I'm slowly progressing on the subject)

@MarcinAman
Copy link
Contributor

Oh, sorry

The named used there will come from Documentable. They probably won't have names from @JvmName and they shouldn't since dokka's model is a Kotlin perspective of the code. You should take the name from the extra

@kamildoleglo
Copy link
Contributor

and they shouldn't since dokka's model is a Kotlin perspective of the code

That's true unless you use kotlin-as-java directly or indirectly (eg. in javadoc format) as there Documentables are presented from Java perspective and IMHO they should have the correct name (equal to its JvmName) there

@bishiboosh
Copy link
Contributor Author

bishiboosh commented Nov 17, 2020

@MarcinAman @kamildoleglo I'm beginning to have some progress (I've handled the @JvmName in most cases, except the return type signature for now, and even handled the @JvmSynthetic as @asfalcone suggested), but as I'm trying to handle @JvmField, there's something I find weird: in KotlinToJavaConverter, a DProperty is emitted along its getter and setter DFunctions. However, in java interop, the property is normally only shown if the field is annotated with @JvmField (and in this case getter and setter are not shown). Am I right in my assumption that this should change and I should create some logic to emit the property only in the @JvmField case ?

(yes, I know, I began with only @JvmName but as my understanding progresses I thought I could handle the other JVM-specific annotations too)

(I'm not handling @JvmMultifileClass for now however, as I think it'd a little bit too difficult for me)

@bishiboosh
Copy link
Contributor Author

It also seems that @JvmOverloads isn't handled, I'll try to see if I can handle it too

@bishiboosh
Copy link
Contributor Author

With the @JvmField work I hope I won't be clashing with other issues, you'll tell me when I make my branch.

(as I'm doing quite a bit of work in KotlinToJavaConverter I'm fixing issues in it related to the various annotations, but which may have been reported elsewhere)

@bishiboosh
Copy link
Contributor Author

bishiboosh commented Nov 18, 2020

(I admit it's slowly becoming a "fix issues in KotlinToJavaConverter if I can too" PR, I hope that's not too much of an issue)

I'll list all issues I've tried to fix in the PR when I launch it, or I can list them here if needed

@bishiboosh
Copy link
Contributor Author

Sorry, I have quite a bit of work right now, I'll try to make the PR next week if I can. In the end, I'm making most of the work in KotlinToJavaConverter, so not only are the names changing but the DRI as well, in order to avoid various linking issues. It's a bit longer but I think it'll be more stable in the end (it probably also avoids changing anything in the search)

@MarcinAman
Copy link
Contributor

@bishiboosh will you have energy and time to work on this issue? If not i can do it ☺️

@bishiboosh
Copy link
Contributor Author

@MarcinAman I don't have the time right now unfortunately, but I'll post what I've done as a WIP PR and try to explain all I've started, hoping you'll be able to go from there. I'll send that to you in a few hours if that's okay.

(sorry, huge project just began at work and I thought I'd have more time)

@MarcinAman
Copy link
Contributor

No problem ☺️ Thanks for that 🥇

@bishiboosh
Copy link
Contributor Author

@MarcinAman added the WIP PR, I've left you as many comments as I could on it

@MarcinAman
Copy link
Contributor

JvmName should be right now handled. It is published in build: 1.4.20.2-dev-57 on space packages. To use it, add a custom repository: maven("https://maven.pkg.jetbrains.space/kotlin/p/dokka/dev")

@MarcinAman MarcinAman linked a pull request Jan 4, 2021 that will close this issue
@bishiboosh
Copy link
Contributor Author

@MarcinAman any idea when this will land in an official release ? I'm okay with waiting a little bit more and avoiding using dev repo in production code

@MarcinAman
Copy link
Contributor

i think it will be released within 1-2 weeks. I don't have an exact date set right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feedback: Google An issue/PR submitted by colleagues at Google, most likely related to the Android API reference docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants