Retain source language information in Documentable tree #2474
Conversation
This will allow dackka to figure out if a DParameter is from Kotlin or from Java Which is necessary to determine if a bare parameter is non-null K or platform J Test: added assertions to translator tests and the multilanguage linker test
|
Still TODO is creating a kotlin-inheriting-from-Java test and verifying that |
There was a problem hiding this comment.
Some premature review to give early feedback :)
Overall LGTM, thanks! I've only got some minor comments
- Can you run
./gradlew apiDumpand commit generated changes? Otherwise it fails the build - I see that you've added language-related asserts to existing tests. While that's good, having a separate test suite to check for this particular behaviour would be much better imo. Ideally, in a separate class
SourceLanguageResolutionTest(or something like this), with simple checks (java, kotlin, groovy for unknown) and some corner cases like inheritance, maybe an extension function on a java type, etc. Just as a sanity check and so that the problem is blatant in case it does break
(sorry if the review was way sooner than you wanted, FYI there's Draft status for Pull Requests for when changes are not ready to be reviewed, use that and I promise I won't touch it until it's ready :) )
| enum class Language { | ||
| JAVA, KOTLIN, UNKNOWN | ||
| } |
There was a problem hiding this comment.
What do you think about renaming it to something like SourceLanguage (preferred) or DocumentableLanguage as before?
My concern is that this enum will be used by someone as a feature/language toggle somewhere on a higher level, and I'd like to try to narrow down the scope of it's usage just to the source/documentable level
If it clashes with data class SourceLanguage, it can be renamed to SourceLanguageExtra
There was a problem hiding this comment.
This could still be done, though there might also be a pre-existing alternative that is more comprehensive, perhaps in the compiler.
| / | ||
|
|
||
| # Sample project | ||
|
|
||
| Sample documentation with [external link](https://www.google.pl) | ||
|
|
||
| ## All modules: | ||
|
|
||
| | Name | | ||
| |---| |
There was a problem hiding this comment.
This keeps appearing after building dokka, very annoying, fixed in #2475. Can you remove this file from this PR?
| pluginOverrides = listOf(OnlyPsiPlugin()) // suppress a descriptor translator because of psi and descriptor translators work in parallel | ||
| //pluginOverrides = listOf(OnlyPsiPlugin()) // suppress a descriptor translator because of psi and descriptor translators work in parallel |
There was a problem hiding this comment.
Is this intentional or a leftover? If intentional, we might have a problem -- this is a recent test from #2400 that checks resolution of ultralight classes specifically, and from what I understand this plugin is needed to avoid conurrency issues
There was a problem hiding this comment.
This is intentional. Without this, the DPackage does not contain the KotlinSubClass.
Concurrency issues sound problematic--wouldn't these two plugins both run under most conditions?
There was a problem hiding this comment.
I might be wrong on the concurrency side of things, @vmishenev knows best
There was a problem hiding this comment.
This test reproduces a fixed bug #2187 that appeared under specific conditions. If it does not suppress a descriptor translator and is broken, it will be lead to the flaky test.
I suppose you can copy the test for your goals without the plugin OnlyPsiPlugin.
| } | ||
|
|
||
| internal fun WithSources.documentableLanguage(sourceSet: DokkaConfiguration.DokkaSourceSet): DocumentableLanguage = | ||
| internal fun WithSources.documentableLanguage(sourceSet: DokkaConfiguration.DokkaSourceSet): Language = |
There was a problem hiding this comment.
Why do you not use this function to determine a source language?
It works with Classlikes, DFunction, DProperty.
There was a problem hiding this comment.
Yes, but it does not work on DParameter, which is where we have been having issues.
There was a problem hiding this comment.
Actually, withSources does something similar to what I am doing here: descriptor.toSourceElement.containingFile.toString()
Making DParameter withSources might be a cleaner solution. I'll take a look. I think the problem is that there's no good way to get sources for a PsiType which is where I'm falling back to determining via whether it's a Psi or Descriptor, though I'm still looking for a better way there.
(Also, that function is internal, though I can certainly just duplicate it.)
There was a problem hiding this comment.
Arguably, other plugin developers might need something similar, and having a HasSourceLanguage marker interface and some way to easily get the source language (like an extra) would be better than some internal function on WithSources that you don't know about
I would consider this to be an API improvement more than solving a specific problem for Dackka :)
Test: still needs dedicated tests
|
Still has TODOs, especially adding dedicated tests. |
…gFile attribute This is not complete.
|
I have been looking around, and KSP is doing something similar to what I was trying here with their I've landed a mitigation downstream for us, which determines source location and language in the cases of parameters defined in or inherited from source, but not anything from compiled code, which is the trickiest case. What I have in this PR covers most compiled code cases, but there are a lot of edge cases and I kept running into more. If you were to choose to try to get this implemented fully and correctly in dokka (thereby allowing determination of nonobvious default-nullability cases), this might be of help. I will be speaking with people on the Google side to see if we and KSP can ask for handling of this to get into the compiler itself. |
This will allow dackka to figure out if a DParameter is from Kotlin or from Java
Which is necessary to determine if a bare parameter is non-null K or platform J
Test: added assertions to translator tests and the multilanguage linker test