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

Get information about expect/actual for properties in K2 via Analysis API and not via PSI #3617

Merged
merged 3 commits into from
May 22, 2024

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented May 20, 2024

fixes #3360

After short investigation I found out that the issue is on our side - we had our own extension properties for expect/actual handling which were using just PSI information.
Accessors via Analysis API were added in https://youtrack.jetbrains.com/issue/KT-54846 (Fix in builds: 1.9.20-dev-7565).
There were no issues with functions, because isExpect/isActual were used there after update to Analysis API 1.9.20-* (because they were introduced as member properties and so they come before our extension properties during overload resolution). With properties the story is a bit different, because there is no isExpect/isActual for KtPropertySymbol which is used in our code, the exists only on KtKotlinPropertySymbol

So looks like upstream issue can be closed

@whyoleg whyoleg self-assigned this May 20, 2024
@@ -740,8 +740,8 @@ private class DokkaDescriptorVisitor(
): DFunction {
val dri = parent.copy(callable = Callable.from(descriptor))
val isGetter = descriptor is PropertyGetterDescriptor
val isExpect = descriptor.isExpect
val isActual = descriptor.isActual
val isExpect = propertyDescriptor.isExpect
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a minor fix for K1 compiler (just to make test easier).
descriptor of getter/setter in properties doesn't contain information about expect/actual (the same is true in K2, there is no such properties there).
So I've decided to make behaviour consistent. Not sure, that will affect anything rather than the model itself

Copy link
Member

@vmishenev vmishenev May 20, 2024

Choose a reason for hiding this comment

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

descriptor of getter/setter in properties doesn't contain information about expect/actual

isExpect/isActual always returns false for accessors.
What is about keeping the same behavior in K2 to avoid the change in Documentable model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's also fine I believe. Do you mind if I will change it for K1 also to just use false instead of isExpect/isActual properties to make it more clear?

multiplatformConfiguration.sourceSets.single { it.displayName == "common" }.sourceSetID

@Test
fun `should recognize expect property in class`() = testInline(
Copy link
Member

Choose a reason for hiding this comment

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

Can you emphasize that a declaration should be without the expect keyword, please?

Copy link
Contributor Author

@whyoleg whyoleg May 21, 2024

Choose a reason for hiding this comment

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

what do you mean? It's not possible to declare expect val/fun inside expect class/interface/etc
I've updated the test name, please take a look

is KtKotlinPropertySymbol -> propertySymbol.isExpect to propertySymbol.isActual
is KtSyntheticJavaPropertySymbol -> false to false
}
val generics = propertySymbol.typeParameters.mapIndexed { index, symbol ->
Copy link
Member

Choose a reason for hiding this comment

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

Can you left the formatting unchanged for convenience, please?

@@ -740,8 +740,8 @@ private class DokkaDescriptorVisitor(
): DFunction {
val dri = parent.copy(callable = Callable.from(descriptor))
val isGetter = descriptor is PropertyGetterDescriptor
val isExpect = descriptor.isExpect
val isActual = descriptor.isActual
val isExpect = propertyDescriptor.isExpect
Copy link
Member

@vmishenev vmishenev May 20, 2024

Choose a reason for hiding this comment

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

descriptor of getter/setter in properties doesn't contain information about expect/actual

isExpect/isActual always returns false for accessors.
What is about keeping the same behavior in K2 to avoid the change in Documentable model?

@whyoleg whyoleg requested a review from vmishenev May 21, 2024 14:18
@whyoleg whyoleg merged commit 72d32b0 into master May 22, 2024
13 checks passed
@whyoleg whyoleg deleted the k2-expect-actual-property branch May 22, 2024 12:52
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.

[K2] KMP: Platform tabs missed common for properties
2 participants