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

Don't suppress obvious members in kotlin.Enum and kotlin.Any #3349

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Nov 15, 2023

Fixes #2863

This is draft because of 2 questions:

  1. To allow Analysis API to analyse kotlin package, we need to provide flag AnalysisFlags.allowKotlinPackage - should we hide it under a flag at our side? And if so, what is the best way provide it to analysis? Or we can just set it true - Descriptor based analysis doesn't need any additional configuration
  2. There is a test failing for Enum. During getting functions of class for Enum from test it fetches also methods from Enum.java: clone, finalize, getDeclaringClass. Somewhere later first 2 are filtered, but the last one is not, and so present in the documentation in the end. I was failed "to find why" and "what to do next" during debugging for 2 hours of FIR compiler internals... So asking for some help here :)

@whyoleg
Copy link
Contributor Author

whyoleg commented Nov 16, 2023

Behaviour is the same after rebase on master (with new Analysis API with fix for https://youtrack.jetbrains.com/issue/KT-63223/Analysis-API-reference-to-declarations-with-kotlin-package-are-not-resolved) - so it's unrelated

@vmishenev
Copy link
Member

vmishenev commented Nov 16, 2023

To allow Analysis API to analyse kotlin package, we need to provide flag AnalysisFlags.allowKotlinPackage - should we hide it under a flag at our side? And if so, what is the best way provide it to analysis? Or we can just set it true - Descriptor based analysis doesn't need any additional configuration

It is only important for stdlib. AnalysisFlags.allowKotlinPackage seems to be kind of ignoreCommonBuiltIns in K1 (for more detail see JetBrains/kotlin@3572a96).
But for now, Dokka K2 does not support stdlib and we have not planned it yet. I have filed #3354 to have an opportunity to refer to it.

There is a test failing for Enum. During getting functions of class for Enum from test it fetches also methods from Enum.java: clone, finalize, getDeclaringClass. Somewhere later first 2 are filtered, but the last one is not, and so present in the documentation in the end. I was failed "to find why" and "what to do next" during debugging for 2 hours of FIR compiler internals... So asking for some help here :)

#3196 #3196 (comment)

I think you can suppress these tests by the OnlyDescriptors annotation with the references to these issues.
It also would be nice to know about the tests in the issues.

@whyoleg
Copy link
Contributor Author

whyoleg commented Nov 20, 2023

@vmishenev thanks! I've removed analysis flag, suppressed test and added comments to the issues
PR is ready for review @IgnatBeresnev / @vmishenev

@IgnatBeresnev
Copy link
Member

I'm ready to approve it as it is, but it would be nice to have additional tests for this bug specifically in analysis-kotlin-api. I think once #3312 is merged, you should even be able to just get documentables for the real kotlin.Enum and kotlin.Any, and verify that their members don't have the ObviousMember extra. Do you wanna do that?

just to reiterate: the tests in dokka-base are good, so I'm only proposing to add additional tests and only for checking obvious members in kotlin.Enum and kotlin.Any; I understand that covering all cases for obvious members falls outside the scope of this issue/PR (although if you wanna add them too - I won't mind 🙂 )

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

👍 👍 Nice work with the tests and corner cases, ObviousMember feels much safer now :)

@whyoleg whyoleg merged commit 417b17b into master Nov 24, 2023
11 checks passed
@whyoleg whyoleg deleted the kotlin-any-enum-suppress branch November 24, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should not suppress obvious members declared in Any, Enum or Object themselves
3 participants