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
Implement Resolver.getSubpackagesOf() #1796
Conversation
Added tests. Interestingly, I get different results in the tests but I also don't fully understand the test package structure in these tests, and curious for feedback on if I'm doing something wrong in setup here. I'm not following how there's a |
It is likely limitation from the PSI handling of incorrectly placed files (file path & package mismatch), KSP2's behavior still looks more correct to me. We probably won't have a good solution for this behavior since it is inherited from PSI. |
Gotcha, so do you think it's just an artifact of the test infra in this case? |
|
it is passing for me on mac as well. failed only on linux, it seems that the problematic part is from the files where file path mismatches package names, it is a known issue for PSI as we discussed in this PR, at this moment there is probably no good solution for this behavior, maybe we can consider remove the misplaced files ? |
Can you expand on what this would entail? |
it looks that the diffs are
which seems to be a result from the files being put in wrong directory with respect to the package names
and if this behavior is inherited from PSI to cause the inconsistency, and there is no fix on KSP side, then we might have to fix the test case to avoid it, after all it is the getSubPackage functionality that we want to test in this case. |
5dedee2
to
8689773
Compare
@neetopia @ting-yuan were either of you able to look further into the PSI discrepancies here? |
.getContributedDescriptors(DescriptorKindFilter.PACKAGES) | ||
.asSequence() | ||
.filterIsInstance<PackageViewDescriptor>() | ||
.filterNot { it == this@subPackages } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I know why .filterIsInstance<PackageViewDescriptor>()
and .filterNot { it == this@subPackages }
are needed?
return analyze { | ||
fun KtPackageSymbol.subPackages(): Sequence<KtPackageSymbol> = getPackageScope() | ||
.getPackageSymbols() | ||
.distinct() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needing to distinct()
may indicate some issues in the underlying Analysis API. Have you tried removing distinct()
? If it works then it works. If not, feel free to leave it there and we'll investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix the test case. IMHO the behavior on KSP1 and KSP2/Mac are weird, even though the directories don't match. Package names should always be consistent with the package
directive.
@TestMetadata("getSubPackages.kt") | ||
@Test | ||
fun testGetSubPackages() { | ||
runTest("../kotlin-analysis-api/testData/getSubPackages.kt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace with: runTest("../test-utils/testData/api/getSubPackages.kt")
. I.e., don't use the forked version.
@@ -0,0 +1,105 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests under kotlin-analysis-api
are forked version of those in test-utils
. They are (ideally) only used when there is a behavior difference between KSP1 and KSP2. It doesn't seem to be necessary in this case.
// main.test | ||
// main.test.main | ||
// main.test.main.test | ||
// main.test.main.test.nested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace main.test.main[\..]*
with main.test.nested
class KK {} | ||
|
||
|
||
// FILE: main/test/main/test/L.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with // FILE: main/test/L.java
} | ||
|
||
|
||
// FILE: main/test/main/test/nested/M.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with // FILE: main/test/nested/M.java
We no longer need this API for our use case and I'm limited on bandwidth, so I'll need to relinquish this feature to whoever wants to implement it in the future :) |
Resolves #1795
Tests coming, just getting this up for early feedback