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

Detekt 1.22.0-RC1 -> 1.22.0-RC2 breaks ignoreAnnotated #5427

Closed
G00fY2 opened this issue Oct 17, 2022 · 10 comments · Fixed by #5508
Closed

Detekt 1.22.0-RC1 -> 1.22.0-RC2 breaks ignoreAnnotated #5427

G00fY2 opened this issue Oct 17, 2022 · 10 comments · Fixed by #5508

Comments

@G00fY2
Copy link
Contributor

G00fY2 commented Oct 17, 2022

After updating from Detekt 1.22.0-RC1 to 1.22.0-RC2 the ignoreAnnotated configurations are not used.

Expected Behavior

Specific rules for code annotated with an annotation defined in ignoreAnnotated should be ignored.

Observed Behavior

Code gets reported despite the ignoreAnnotated configuration.

Steps to Reproduce

The config file.

Build report with 1.22.0-RC1.

Run ./gradlew assembleDebug detektDebug
Starting a Gradle Daemon (subsequent builds will be faster)

> Configure project :app
WARNING:The option setting 'android.experimental.enableNewResourceShrinker.preciseShrinking=true' is experimental.
The current default is 'false'.

> Task :app:preBuild UP-TO-DATE
> Task :app:preDebugBuild UP-TO-DATE
> Task :app:mergeDebugNativeDebugMetadata NO-SOURCE
> Task :app:generateDebugBuildConfig
> Task :app:checkDebugAarMetadata
> Task :app:mapDebugSourceSetPaths
> Task :app:generateDebugResources UP-TO-DATE
> Task :app:packageDebugResources
> Task :app:mergeDebugResources
> Task :app:createDebugCompatibleScreenManifests
> Task :app:extractDeepLinksDebug
> Task :app:parseDebugLocalResources
> Task :app:processDebugMainManifest
> Task :app:processDebugManifest
> Task :app:processDebugManifestForPackage
> Task :app:javaPreCompileDebug
> Task :app:generateDebugAssets UP-TO-DATE
> Task :app:mergeDebugAssets
> Task :app:compressDebugAssets
> Task :app:processDebugJavaRes NO-SOURCE
> Task :app:processDebugResources
> Task :app:checkDebugDuplicateClasses
> Task :app:desugarDebugFileDependencies
> Task :app:kaptGenerateStubsDebugKotlin
> Task :app:mergeExtDexDebug
> Task :app:mergeLibDexDebug
> Task :app:mergeDebugJniLibFolders

> Task :app:kaptDebugKotlin
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported

> Task :app:mergeDebugNativeLibs NO-SOURCE
> Task :app:stripDebugDebugSymbols NO-SOURCE
> Task :app:validateSigningDebug
> Task :app:writeDebugAppMetadata
> Task :app:writeDebugSigningConfigVersions
> Task :app:compileDebugKotlin
> Task :app:compileDebugJavaWithJavac
> Task :app:hiltAggregateDepsDebug
> Task :app:detektDebug
> Task :app:hiltJavaCompileDebug
> Task :app:transformDebugClassesWithAsm
> Task :app:mergeDebugJavaResource
> Task :app:dexBuilderDebug
> Task :app:mergeProjectDexDebug
> Task :app:packageDebug
> Task :app:createDebugApkListingFileRedirect
> Task :app:assembleDebug

BUILD SUCCESSFUL in 2m 21s
36 actionable tasks: 36 executed

Build report with 1.22.0-RC2.

Run ./gradlew assembleDebug detektDebug
Starting a Gradle Daemon (subsequent builds will be faster)

> Configure project :app
WARNING:The option setting 'android.experimental.enableNewResourceShrinker.preciseShrinking=true' is experimental.
The current default is 'false'.

> Task :app:preBuild UP-TO-DATE
> Task :app:preDebugBuild UP-TO-DATE
> Task :app:mergeDebugNativeDebugMetadata NO-SOURCE
> Task :app:generateDebugBuildConfig
> Task :app:checkDebugAarMetadata
> Task :app:mapDebugSourceSetPaths
> Task :app:generateDebugResources UP-TO-DATE
> Task :app:packageDebugResources
> Task :app:mergeDebugResources
> Task :app:createDebugCompatibleScreenManifests
> Task :app:extractDeepLinksDebug
> Task :app:parseDebugLocalResources
> Task :app:processDebugMainManifest
> Task :app:processDebugManifest
> Task :app:processDebugManifestForPackage
> Task :app:javaPreCompileDebug
> Task :app:generateDebugAssets UP-TO-DATE
> Task :app:mergeDebugAssets
> Task :app:compressDebugAssets
> Task :app:processDebugJavaRes NO-SOURCE
> Task :app:processDebugResources
> Task :app:checkDebugDuplicateClasses
> Task :app:desugarDebugFileDependencies
> Task :app:kaptGenerateStubsDebugKotlin
> Task :app:mergeExtDexDebug
> Task :app:mergeLibDexDebug
> Task :app:mergeDebugJniLibFolders

> Task :app:kaptDebugKotlin
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported
Values of variant API AnnotationProcessorOptions.arguments are queried and may return non final values, this is unsupported

> Task :app:mergeDebugNativeLibs NO-SOURCE
> Task :app:stripDebugDebugSymbols NO-SOURCE
> Task :app:validateSigningDebug
> Task :app:writeDebugAppMetadata
> Task :app:writeDebugSigningConfigVersions
> Task :app:compileDebugKotlin
> Task :app:compileDebugJavaWithJavac
> Task :app:hiltAggregateDepsDebug

> Task :app:detektDebug
The BindingContext was created with 159 issues. Run detekt CLI with --debug or set `detekt { debug = true }` in Gradle to see the error messages.
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/Beers.kt:22:5: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/Beers.kt:34:5: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/BeerCard.kt:25:5: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/BeerCard.kt:56:13: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/BeerCard.kt:[7](https://github.com/G00fY2/Sample-PunkApp/actions/runs/3263770673/jobs/5363350392#step:5:8)3:13: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/main/PunkMain.kt:11:5: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/theme/PunkAppTheme.kt:31:5: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/BeerCard.kt:56:13: Private function `MessageCardPreview` is unused. [UnusedPrivateMember]
/home/runner/work/Sample-PunkApp/Sample-PunkApp/app/src/main/kotlin/com/g00fy2/punkapp/ui/beers/BeerCard.kt:73:13: Private function `MessageCardDarkPreview` is unused. [UnusedPrivateMember]


> Task :app:hiltJavaCompileDebug
> Task :app:transformDebugClassesWithAsm
> Task :app:mergeDebugJavaResource
> Task :app:dexBuilderDebug
> Task :app:mergeProjectDexDebug
> Task :app:packageDebug
> Task :app:createDebugApkListingFileRedirect
> Task :app:assembleDebug

BUILD SUCCESSFUL in 3m 8s
36 actionable tasks: 36 executed

Your Environment

See the sample project. Also occurred in other (closed source) project with stable release versions (AGP 7.3.1, Compose 1.2.1).

@cortinico
Copy link
Member

Thanks for reporting this @G00fY2. It looks like a regression that should be addressed before we call 1.22 stable.

@G00fY2
Copy link
Contributor Author

G00fY2 commented Oct 20, 2022

@3flex @cortinico I was able to pin down the issue to this commit: 505d6d7

Reverting these changes fixes the ignored ignoreAnnotated configurations.

Unfortunately I was not able to write a failing test. Still hope this information helps.

@3flex
Copy link
Member

3flex commented Oct 20, 2022

Thanks for finding the offending commit. We can revert the commit and reintroduce #5150, or find a better fix for that which doesn't introduce a regression.

@G00fY2
Copy link
Contributor Author

G00fY2 commented Oct 20, 2022

Even though configuration cache is an incubating feature I know that many people are already using it (we also have it enabled).
It would be nice if detekt is "fully compatible" once it gets stable. Until then you can e.g. run detekt taks with --no-configuration-cache (we do this on CI). Detekt ignoring it's config file is probably a much more serious issue.

Regardless of whether we decide to simply revert the changes it would be nice to have a test that makes sure this regression will not appear again. Strangely enough when I try to run the following tests on my machine, everything succeeds: G00fY2@9ed069a

@3flex
Copy link
Member

3flex commented Oct 21, 2022

We strive for configuration cache compatibility and on non-Android projects at least I'm not aware of any issues currently.

That said, it would be best to prioritise correctness over performance. We should revert that PR, break config cache and fix this issue.

@yogurtearl
Copy link
Contributor

#5150 is keeping us on detekt 1.20.0, and detekt 1.20.0 is keeping us on an old version of Kotlin. :(

#4762 is the PR that caused #5150

if we revert the fix for #5150, maybe also revert #4762 ? 😬

@3flex
Copy link
Member

3flex commented Oct 21, 2022

We should find a proper fix then instead of reverting - though detekt being on an older Kotlin version shouldn't block you from updating the dependency for your project. Please open a separate issue for that.

@BraisGabin
Copy link
Member

It is really strange that 505d6d7 could break ignoreAnnotated. I don't doubt about it. But I think that its breaking something more "fundamental" on detekt and as a secundary effect it breakis ignoreAnnotated. Maybe this is related with #5435.

@G00fY2
Copy link
Contributor Author

G00fY2 commented Oct 27, 2022

Not sure what you think about this and if this is already in planing. But what about moving away from the already old (already deprecated) BaseVariant API to the new one: https://medium.com/androiddevelopers/new-apis-in-the-android-gradle-plugin-f5325742e614

There are new shiny APIs for accessing the compileClasspath e.g.

This would maybe fix some of the issues about the Android setups. Only drawback is that AGP compatibility will jump to 7.3 and a lot of APIs are marked as Incubating.

Here is a first idea how this could look like: main...G00fY2:detekt:variant-migration-wip

@BraisGabin
Copy link
Member

BraisGabin commented Oct 27, 2022

I would be OK about bumping it to 7.3 if it let us to get more information for the type solving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants