Register compiler plugin services in Analysis API session#9331
Draft
3flex wants to merge 9 commits into
Draft
Conversation
detekt-ci
added
rules
api
core
gradle-plugin
build
dependencies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (43.03%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9331 +/- ##
============================================
- Coverage 84.72% 84.46% -0.27%
Complexity 4486 4486
============================================
Files 571 571
Lines 12423 12501 +78
Branches 2767 2788 +21
============================================
+ Hits 10526 10559 +33
- Misses 696 728 +32
- Partials 1201 1214 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
Is this going to support all the compiler plugins? Should we create more tests with different plugins to ensure that it works (I can implement them)? For example, I think that we should have tests for Compose and Metro. |
Member
Author
|
If you coud check and add tests that would be fantastic. I don't use either. |
Member
|
Do you want me to add the tests on this branch directly or do you prefer that I add them to a new branch with |
Member
Author
|
Happy to add to this branch, thank you! |
Member
|
Checking on my project I realised that Compose and Metro doesn't generate any issue. But, on the other hand, pacelize, viewbinding and BuildConfig does. So I just pushed 3 new tests. One for each of those. I'm pretty sure that viewbinding and BuildConfig are not kotlin compiler plugins so if you prefer I can move them to a separated PR. On my project SQLDelight also generates problems but I'm not able to make SQLDelight work with |
Member
Author
|
There's a new flag for Kotlin compiler in 2.4 which prints compiler plugin config (among other info) so you can confirm which compiler plugins are in use. Add I don't see any value in adding tests for things like BuildConfig that I'm 99% certain doesn't use a compiler plugin, either in this PR or any other. |
Member
Author
|
You can also check source for I think for our purposes only plugins that interact with the front end plugin API would ever be in scope for detekt to analyse, see https://kotlinlang.org/docs/custom-compiler-plugins.html#frontend-plugin-api. On my reading this would include:
And possibly others. So I think the best approach to ensure coverage would be:
As long as there's at least one test that checks that the compiler plugin infrastructure is working correctly I don't think we need to test for a plugin that checks every type of compiler front end usage. That means more maintenance, particularly as the compiler plugin API is not stable. One test with serialization plugin should suffice, since compiler plugin updates are shipped with Kotlin itself, and will continue working with new compiler versions. |
Member
Parcelize is also shipped with kotlin so we can keep it. I'm going to move the other 2 to other PR and I'm going to create three issues: About test with more plugins I guess that we can wait for users to raise issues about incompatible plugins once we support at least some of them. |
Contributor
|
Wanted to try this out on a sample project I've used for testing since I had looked into this earlier, but I'm running into two issues. Not sure if this is only an issue since I add Changing stacktrace |
Member
Author
|
@FletchMcKee I'll update this PR later today. Can you share your sample project so I can look at those failures? |
Collaborator
Member
Author
|
@BraisGabin can I remove the buildconfig and viewbinding tests? This is otherwise ready for review. |
Member
|
Yes I'll move them to other prs. You can remove them |
Contributor
|
@3flex Just created a detekt-7531 branch in my NowInAndroid fork and set it up so that it would find mavenLocal Detekt artifacts. Has the same issue with gradle-public-api, but when switching that to compileOnly, you'll see Detekt runs fine in some modules but fails in others. For example, no issues in NIA's ./gradlew :core:database:detektMainBut ./gradlew :core:ui:detektMainLet me know if you have any issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7531. Check the final commit for the changes.
The implementation is fairly simple, except that I've had to copy a fair bit of code into
KotlinEnvironmentUtils.ktasloadPluginsinCliCompileris protected and can't be called directly. There's probably a better way to do this but it works for now.I used this test (which I've cherry-picked in this PR) #7531 (comment) and also the sample project (updated to newer Gradle & Kotlin versions) here to verify: #7531 (comment)
I haven't tested with any other compiler plugins, though I have confirmed that the compiler options are correctly parsed and passed to detekt's
CompilerConfigurationso hopefully no additional work is required for other plugins.