Fix gathering inherited properties #2353

BarkingBad · 2022-02-10T15:29:26Z

Enables gathering super fields/properties by PSI's. This PR depends on model changes introduced in #2326 so I leave it as a draft (it is rebased onto that branch in order to build correctly. Nonetheless, it can be reviewed as a single commit.

vmishenev

I have reviewed only 122c611 here.
Do you want to test InheritedMember in an extra?

IgnatBeresnev · 2022-02-17T15:28:23Z

I've merged the changes this PR is based on, can you resolve conflicts please?

BarkingBad · 2022-02-17T15:34:17Z

Rebased, should be good now

BarkingBad · 2022-02-17T17:38:35Z

I've observed that the inhertited property is missing annotations, I will try to fix it that and include it in the tests

BarkingBad · 2022-02-22T11:56:46Z

I know changes introduced in secondary commit are not directly coupled with the subject of the PR, but introduce small bugfixes that are related to the problem:

minor changes with parameters of functions transforming kotlin to java (context to logger, since it is more accurate)
fixed 2 smaller bugs with annotations and with names of accessors
fixed issue with top-level Property TagWrapper for java beans functions that inherit description from the kotlin property, since it doesn't make any sense

I can part it to smaller PRs if demanded

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

plugins/base/src/test/kotlin/model/PropertyTest.kt

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

IgnatBeresnev · 2022-03-01T14:33:40Z

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

+        return regularMethods.toList() to accessors.map { (dProperty, dFunctions) ->
+            if (dProperty.visibility.values.all { it is KotlinVisibility.Private }) {
+                dFunctions.flatMap { it.visibility.values }.toSet().singleOrNull()?.takeIf {
+                    it in listOf(KotlinVisibility.Public, KotlinVisibility.Protected)


I think this can be extracted into a variable with a name that explains intention (something like overridableVisibilities if I got the idea right), plus will avoid unnecessary allocation

Will look into that

It's not necesarily overridability that we put emphasis on here, but rather we struggle with the proper visibility for property. The problem is, the java field will most likely be private and its accessors will be public/protected. We have to replace the visibility of the DProperty as it would be not visible in Kotlin. On the other hand I am now thinking about situation where we have public getter and private setter. That way we would like to have our DProperty to be public, yet be presented as val. Probably will have to rewrite the logic here...
I am also curious how dokka is handling private setter vars, is it rendering it like vals? I have to double check that

Do you need time to investigate it before we merge this PR? Can what you're talking about break something or introduce bugs?

nice comments btw, thanks

I think we should hardcode the configurations like these manually... I don't know if I have time to do it this week.

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

plugins/base/src/main/kotlin/translators/psi/DefaultPsiToDocumentableTranslator.kt

plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt

plugins/javadoc/src/test/kotlin/org/jetbrains/dokka/javadoc/JavadocIndexTest.kt

plugins/kotlin-as-java/src/main/kotlin/transformToJava.kt

…setters.

…m, DInterface in KaJ.

IgnatBeresnev · 2022-03-09T12:56:56Z

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

+        return regularMethods.toList() to accessors.map { (dProperty, dFunctions) ->
+            if (dProperty.visibility.values.all { it is KotlinVisibility.Private }) {
+                dFunctions.flatMap { it.visibility.values }.toSet().singleOrNull()?.takeIf {
+                    it in listOf(KotlinVisibility.Public, KotlinVisibility.Protected)


Do you need time to investigate it before we merge this PR? Can what you're talking about break something or introduce bugs?

nice comments btw, thanks

IgnatBeresnev · 2022-03-09T13:56:18Z

plugins/base/src/main/kotlin/transformers/documentables/PropertiesMergerTransformer.kt

+
+/**
+ * This transformer is used to merge the backing fields and accessors (getters and setters)
+ * obtained from Java sources. This way, we could generate more coherent documentation,


Is this transformer only applicable to java sources? If so:

what do you think about renaming it to JavaPropertiesMergerTransformer?

Can we somehow filter processed items and avoid processing kotlin classes? Does it make sense? Although I don't know if we have the ability to extract that info from DClass. Or maybe you already have that filter somewhere, e.g via KotlinVisibility?

My main concern is that if we don't limit it, then kotlin classes might satisfy the same conditions as Java and it'll introduce bugs (i.e not display getters/setters of a kotlin class for explicit declarations). If it's only java classes that are processed (somehow), should be safer.

But maybe this can never happen?

Technically it can happen, I am not sure how I could limit it to only Java classes. Do you have any idea?

if you mean how is the source language defined https://github.com/Kotlin/dokka/blob/master/plugins/base/src/main/kotlin/translators/documentables/documentableLanguage.kt#L1

BarkingBad · 2022-03-10T10:04:44Z

I'll take a look tomorrow on that

vmishenev

I have discovered the same logic to merge accessors with fields in the different places: PropertiesMergerTransformer and DefaultPsiToDocumentableTranslator. What do you about extraction this logic from DefaultPsiToDocumentableTranslator into PropertiesMergerTransformer?

plugins/base/src/test/kotlin/superFields/PsiSuperFieldsTest.kt

vmishenev · 2022-03-09T23:44:33Z

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

+         * declared and inheritedFrom as the same DRI but truncated callable part.
+         * Therefore, we set callable to null and take the DRI only if it is indeed coming from different class.
+         */
+        val inheritedFrom = dri.copy(callable = null).takeIf { parent.dri.classNames != dri.classNames }


What is about classes with the same name but from the different packages?

Good point. Should it be parent.dri.classNames != dri.classNames || parent.dri.packageName != dri.packageName? If the classnames are equal we check if the package is different.

Yes, it should.

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

vmishenev · 2022-03-10T11:13:49Z

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt

+             *
+             * Actually, the problem impacts more of these tags, yet this particular tag was blocker for
+             * some opens-source plugin creators.
+             * TODO: Should rethink if we could fix it globally in dokka or in compiler itself.


What solution do you suggest?

It could be fixed in the compiler. On the other hand, the compiler returns corresponding KDoc for some property etc. that is based on the positions of the original KDoc string, and therefore it is wrapped in the Property section, which is somehow correct. There could be some service in dokka to translate all of these, though one should rethink all the possibilities and cover them manually. Probably it won't be so hard afterall

BarkingBad · 2022-03-14T08:42:46Z

I have discovered the same logic to merge accessors with fields in the different places: PropertiesMergerTransformer and DefaultPsiToDocumentableTranslator. What do you about extraction this logic from DefaultPsiToDocumentableTranslator into PropertiesMergerTransformer?

I honestly don't remember why I copied it (I know it is redundant, I warned about that), but don't remember the case when the properties won't be merged in PSI translator already.|

EDIT: At least, it is flipping the visibility of the property basing on its accessors, though I don't remember why I have to do it here, not in PSI translator.

EDIT 2: Ah, I finally recalled it! It is for things we inherit from Java but are from Kotlin class. The problem is, we get things from Descriptors but Java properties are not merged/processed. This transformer is for merging such properties. It also answers @IgnatBeresnev question about limiting this transformer to only Java sources. We cannot, since it is mainly for Kotlin sources, apparently.

vmishenev · 2022-03-15T14:30:25Z

So are there some blockers to merge accessors with fields only in PropertiesMergerTransformer?
I do not like we have the same code in the different places.
I suggested keeping all the fields and methods in DefaultPsiToDocumentableTranslator as is. Then PropertiesMergerTransformer can process them to merge accessors with properties.

It also answers @IgnatBeresnev question about limiting this transformer to only Java sources.

We need to merge properties only inside a Java class. Is it right? So is it possible to limit the transformer to only Java class?

BarkingBad · 2022-03-15T14:33:45Z

Yes, we could limit it to be only in transformer and remove it from PSIs I think.

We need to merge properties only inside a Java class. Is it right? So is it possible to limit the transformer to only Java class?

Yes, we want to merge Java properties, but these can be coming from Descriptors, by inheriting from Java class in Kotlin. We cannot determine afaik from where the Documentable is coming, and if so, we should do it in DescriptorsToDocumentableTransformer then when we are on PSI / Descriptors layer of abstraction.

BarkingBad · 2022-03-15T14:37:13Z

The reason I didn't refactor it was because I was short on time. I don't know if I will manage to fix it soon, so if you have time to remove the merging mechanisms from PSI please feel welcomed. It should not be that hard after all.

IgnatBeresnev · 2022-04-29T18:00:19Z

Hi!

The PR is starting to rot a little, there are some conflicts and new build errors. I've checked out your changes locally and rebased onto latest master, and I'm planning to remove duplicated code as we discussed above -- I know it's hard to find time for this, so we don't want to burden you too much with this :)

I'm closing this PR to preserve history (don't want to force-push over your commits and comments), but your changes have migrated to #2481 and will be merged after some additional changes. Thanks for your work!

BarkingBad force-pushed the fix-gathering-inherited-properties branch from cb2360a to 122c611 Compare February 10, 2022 16:21

vmishenev requested review from vmishenev and IgnatBeresnev February 14, 2022 01:10

vmishenev approved these changes Feb 14, 2022

View reviewed changes

BarkingBad force-pushed the fix-gathering-inherited-properties branch 2 times, most recently from 2f42d93 to 516ecaa Compare February 15, 2022 15:51

IgnatBeresnev approved these changes Feb 16, 2022

View reviewed changes

BarkingBad force-pushed the fix-gathering-inherited-properties branch from 516ecaa to c8f45ed Compare February 17, 2022 13:50

BarkingBad marked this pull request as ready for review February 17, 2022 13:50

BarkingBad force-pushed the fix-gathering-inherited-properties branch from c8f45ed to 925d45b Compare February 17, 2022 15:33

BarkingBad force-pushed the fix-gathering-inherited-properties branch 3 times, most recently from 19a1bb7 to 0bd5b63 Compare February 22, 2022 09:23

BarkingBad force-pushed the fix-gathering-inherited-properties branch 4 times, most recently from ac524d9 to f240e59 Compare February 23, 2022 18:35

BarkingBad commented Feb 23, 2022

View reviewed changes

BarkingBad commented Feb 24, 2022

View reviewed changes

...ns/base/src/main/kotlin/translators/descriptors/DefaultDescriptorToDocumentableTranslator.kt Outdated Show resolved Hide resolved

IgnatBeresnev requested review from IgnatBeresnev and vmishenev February 24, 2022 10:26

BarkingBad force-pushed the fix-gathering-inherited-properties branch from a97934e to 668b707 Compare February 24, 2022 11:45

Kordyjan mentioned this pull request Feb 25, 2022

Kotlin-as-Java fixes for external invocation #2377

Closed

IgnatBeresnev reviewed Mar 1, 2022

View reviewed changes

BarkingBad added 2 commits March 2, 2022 12:13

Fix gathering inherited properties in PSI

a6b4650

Refacotr KaJ transformer. Change wrapping TagWrapper for getters and …

23c95db

…setters.

BarkingBad added 4 commits March 2, 2022 12:13

Add logic to merge inherited properties in kotlin from java sources.

50d00e7

Remove getters and setters from JvmField properties for DObject, DEnu…

dd7dc6b

…m, DInterface in KaJ.

Unify InheritedMember DRI logic.

efdd42f

Fix gathering docs obtained from inheriting java sources in descriptors

3097193

BarkingBad force-pushed the fix-gathering-inherited-properties branch from 13b1ab0 to bba621b Compare March 2, 2022 11:13

Apply requested changes.

bb746c9

BarkingBad force-pushed the fix-gathering-inherited-properties branch from bba621b to bb746c9 Compare March 2, 2022 11:29

IgnatBeresnev approved these changes Mar 9, 2022

View reviewed changes

vmishenev requested changes Mar 10, 2022

View reviewed changes

BarkingBad force-pushed the fix-gathering-inherited-properties branch from fed49b3 to fecb1c2 Compare March 14, 2022 13:42

Apply requested changes.

255925b

BarkingBad force-pushed the fix-gathering-inherited-properties branch from fecb1c2 to 255925b Compare March 14, 2022 13:42

vmishenev self-requested a review March 15, 2022 15:19

vmishenev approved these changes Mar 15, 2022

View reviewed changes

IgnatBeresnev mentioned this pull request Apr 29, 2022

Fix gathering inherited properties #2481

Merged

IgnatBeresnev closed this Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gathering inherited properties #2353

Fix gathering inherited properties #2353

BarkingBad commented Feb 10, 2022

vmishenev left a comment •

edited

Loading

IgnatBeresnev commented Feb 17, 2022

BarkingBad commented Feb 17, 2022

BarkingBad commented Feb 17, 2022

BarkingBad commented Feb 22, 2022

IgnatBeresnev Mar 1, 2022 •

edited

Loading

BarkingBad Mar 2, 2022

BarkingBad Mar 2, 2022

IgnatBeresnev Mar 9, 2022

BarkingBad Mar 14, 2022

IgnatBeresnev Mar 9, 2022

IgnatBeresnev Mar 9, 2022 •

edited

Loading

BarkingBad Mar 14, 2022

vmishenev Mar 15, 2022

BarkingBad commented Mar 10, 2022

vmishenev left a comment •

edited

Loading

vmishenev Mar 9, 2022

BarkingBad Mar 14, 2022

vmishenev Mar 15, 2022

vmishenev Mar 10, 2022

BarkingBad Mar 14, 2022

BarkingBad commented Mar 14, 2022 •

edited

Loading

vmishenev commented Mar 15, 2022

BarkingBad commented Mar 15, 2022

BarkingBad commented Mar 15, 2022

IgnatBeresnev commented Apr 29, 2022

Fix gathering inherited properties #2353

Fix gathering inherited properties #2353

Conversation

BarkingBad commented Feb 10, 2022

vmishenev left a comment • edited Loading

Choose a reason for hiding this comment

IgnatBeresnev commented Feb 17, 2022

BarkingBad commented Feb 17, 2022

BarkingBad commented Feb 17, 2022

BarkingBad commented Feb 22, 2022

IgnatBeresnev Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgnatBeresnev Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BarkingBad commented Mar 10, 2022

vmishenev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BarkingBad commented Mar 14, 2022 • edited Loading

vmishenev commented Mar 15, 2022

BarkingBad commented Mar 15, 2022

BarkingBad commented Mar 15, 2022

IgnatBeresnev commented Apr 29, 2022

vmishenev left a comment •

edited

Loading

IgnatBeresnev Mar 1, 2022 •

edited

Loading

IgnatBeresnev Mar 9, 2022 •

edited

Loading

vmishenev left a comment •

edited

Loading

BarkingBad commented Mar 14, 2022 •

edited

Loading