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

Should KSPropertyDeclarationImpl.findOverridee use singleOrNull? #174

Closed
mvdbos opened this issue Nov 27, 2020 · 2 comments · Fixed by #176
Closed

Should KSPropertyDeclarationImpl.findOverridee use singleOrNull? #174

mvdbos opened this issue Nov 27, 2020 · 2 comments · Fixed by #176

Comments

@mvdbos
Copy link

mvdbos commented Nov 27, 2020

return ResolverImpl.instance.resolvePropertyDeclaration(this)?.original?.overriddenDescriptors?.single { it.overriddenDescriptors.isEmpty() }

This now fails for me on an overridden val. The way this code is written now, looks like the last '?' in this part of the statement .single { it.overriddenDescriptors.isEmpty() }? is never going to happen, since single never returns null.

@mvdbos
Copy link
Author

mvdbos commented Nov 27, 2020

After changing this to singleOrNull I no longer get the NoSuchElementException and looks like all properties are correctly found.

@yigit
Copy link
Collaborator

yigit commented Nov 27, 2020

i think we need to move it to be similar to how functions are implemented:
#151
I totally missed properties while implementing that :/

yigit added a commit to yigit/ksp that referenced this issue Nov 27, 2020
This CL updates KSPropertyDecl.findOverridee to use the same infra
that is used by KSFunctionDeclaration.

Fixes: google#174
neetopia pushed a commit that referenced this issue Nov 30, 2020
This CL updates KSPropertyDecl.findOverridee to use the same infra
that is used by KSFunctionDeclaration.

Fixes: #174
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Dec 1, 2020
This CL fixes another case on variance inheritance for overrides where
if there is an overridden method w/ star projected type argument,
we should not use start projection but instead pick covariant (out).

To reproduce it, i've expanded method spec helper test to cover methods
that are not overridden. Unfortunately, it revelaled an issue where
if a kotlin interface overrides another interface with generics, it is
not possible to generate kotlin override for it if the type argument is
a final type. (b/174313780). Only one test case hits it so I've added
a way to ignore them for now (we cannot fix it until room generates
kotlin sources).

I've also hit some KSP bugs:
google/ksp#175
Overrides might fail when overriding with generics. Luckily,
findOverridee method finds it so I've added a workaround by calling
findOverridee. We could possibly only call it instead of calling both
but resolver bug will be eventually fixed in KSP hence I kept that call.

google/ksp#174
Since we call findOverridee for properties now, it might trigger another
issue in KSP. Added a try catch for it now.

google/ksp#173
KSP might throw when resolving deserialized type alias descriptors. Added
a try catch for it where we hit the issue. (reading annotations from an
element that also has @throws annotation).

google/ksp#164
This issue also affects get getJvmName for which we have safeGetJvmName.
In this CL, it happens when finding overridee for getter/setter methods in
java. Added a try catch for it for now.

Bug: 174313780
Bug: 160322705
Test: MethodSpecHelperTest
Change-Id: Ie6347f46027c317ed7784e8d2a0c834012fb4b52
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Dec 9, 2020
This version is important because it also moves KSP's
kotlin dependency to 1.4.20.

I had to specify more versions for testing because we were
getting kotlin compiler dependencies from kotlin compile
testing, which is still on 1.4.10. Now compiler-processing-testing
artifact explicitly specifies those version to match whatever
AndroidX is using.

I've also removed workarounds for the following issues
as they are fixed.

google/ksp#170
google/ksp#174

Bug: 160322705
Test: existing tests
Change-Id: If948bcb762d1fad4911cf833a85cda6a36e7c787
neetopia pushed a commit that referenced this issue Dec 23, 2020
This CL updates KSPropertyDecl.findOverridee to use the same infra
that is used by KSFunctionDeclaration.

Fixes: #174
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 a pull request may close this issue.

2 participants