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

Make DoubleMutabilityForCollection configurable and set a DoubleMutability alias #4541

Merged

Conversation

rocketraman
Copy link
Contributor

Resolves #4501

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Great stuff! 🚀

@@ -489,6 +530,26 @@ class DoubleMutabilityForCollectionSpec : Spek({
assertThat(result).hasSize(1)
assertThat(result).hasSourceLocation(2, 5)
}

it("detects var declaration with MutableState, when configured") {
Copy link
Member

Choose a reason for hiding this comment

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

Could we also add a test where you declare and use those functions:

fun <T : Any?> mutableStateOf(value: T?): MutableState<T>
fun <T : Any?> remember(calculation: (() -> T)?): T

So to make sure everything works correctly in a Compose scenario

@cortinico cortinico added this to the 1.20.0 milestone Jan 29, 2022
@rocketraman
Copy link
Contributor Author

@cortinico ready for re-review.

Note, I also tried to add a negative test to validate that property delegates are ignored:

            describe("ignores declaration with var and property delegation") {
                val rule = DoubleMutabilityForCollection(
                    TestConfig(
                        mapOf(
                            MUTABLE_TYPES to listOf("MutableState")
                        )
                    )
                )

                val code = """
                    data class MutableState<T>(var state: T)
                    fun <T> mutableStateOf(value: T): MutableState<T>
                    fun <T> remember(calculation: () -> T): T
                    inline operator fun <T> MutableState<T>.getValue(thisObj: Any?, property: KProperty<*>): T = value
                    inline operator fun <T> MutableState<T>.setValue(thisObj: Any?, property: KProperty<*>, value: T) {
                        this.value = value
                    }
                    fun main() {
                        var myState by remember { mutableStateOf("foo") }
                    }
                    """
                val result = rule.compileAndLintWithContext(env, code)
                assertThat(result).isEmpty()
            }

but got an error:

    java.lang.AssertionError: 'env' can not be accessed in this context.
        at org.spekframework.spek2.runtime.lifecycle.MemoizedValueAdapter.get(MemoizedValueAdapter.kt:33)
        at org.spekframework.spek2.runtime.lifecycle.MemoizedValueAdapter.getValue(MemoizedValueAdapter.kt:22)
        at io.gitlab.arturbosch.detekt.rules.bugs.DoubleMutabilityForCollectionSpec$1.invoke$lambda-0(DoubleMutabilityForCollectionSpec.kt:17)
...

Any ideas? Seems like the property delegate breaks it.

@cortinico cortinico changed the title Make double mutability configurable Make DoubleMutabilityForCollection configurable and set a DoubleMutability alias Jan 30, 2022
@cortinico
Copy link
Member

cortinico commented Jan 30, 2022

at org.spekframework.spek2.runtime.lifecycle.MemoizedValueAdapter.get(MemoizedValueAdapter.kt:33)
at org.spekframework.spek2.runtime.lifecycle.MemoizedValueAdapter.getValue(MemoizedValueAdapter.kt:22)

Please note that we're in the process of migrating tests from Spek to JUnit.
Specifically this PR is going to clash with:

Potentially adapting those tests would fix your failure

@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #4541 (b56155a) into main (c34f5fa) will increase coverage by 84.11%.
The diff coverage is 100.00%.

❗ Current head b56155a differs from pull request most recent head fb768cd. Consider uploading reports for the commit fb768cd to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #4541       +/-   ##
===========================================
+ Coverage        0   84.11%   +84.11%     
- Complexity      0     3314     +3314     
===========================================
  Files           0      477      +477     
  Lines           0    10924    +10924     
  Branches        0     2029     +2029     
===========================================
+ Hits            0     9189     +9189     
- Misses          0      700      +700     
- Partials        0     1035     +1035     
Impacted Files Coverage Δ
...detekt/rules/bugs/DoubleMutabilityForCollection.kt 87.09% <100.00%> (ø)
...github/detekt/tooling/dsl/ExtensionsSpecBuilder.kt 95.45% <0.00%> (ø)
...rbosch/detekt/rules/complexity/ComplexCondition.kt 89.36% <0.00%> (ø)
.../detekt/metrics/processors/ProjectCLOCProcessor.kt 100.00% <0.00%> (ø)
...n/io/github/detekt/metrics/processors/util/LLOC.kt 80.00% <0.00%> (ø)
...in/io/github/detekt/metrics/CognitiveComplexity.kt 88.23% <0.00%> (ø)
...lab/arturbosch/detekt/rules/style/MaxLineLength.kt 94.59% <0.00%> (ø)
...arturbosch/detekt/core/baseline/BaselineHandler.kt 95.00% <0.00%> (ø)
.../arturbosch/detekt/core/baseline/BaselineFormat.kt 88.57% <0.00%> (ø)
...turbosch/detekt/rules/style/ForbiddenMethodCall.kt 88.57% <0.00%> (ø)
... and 468 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c34f5fa...fb768cd. Read the comment docs.

@rocketraman rocketraman force-pushed the issue-4501-configurable-double-mutability branch from b56155a to 59a907d Compare January 31, 2022 15:04
@rocketraman
Copy link
Contributor Author

Please note that we're in the process of migrating tests from Spek to JUnit.

Thank goodness, running Spek tests from within IntelliJ is a pain. Actually couldn't figure out a way to do it, even with the plugin.

Potentially adapting those tests would fix your failure

It does indeed fix the problem.

I've rebased this PR on the Junit branch and added the additional tests.

@rocketraman
Copy link
Contributor Author

NOTE: GitHub is not letting me set the junit branch as the base of this PR, so the junit commits are here too.

@cortinico
Copy link
Member

You should now be able to rebase 👍

@cortinico cortinico removed the blocked label Feb 1, 2022
@rocketraman rocketraman force-pushed the issue-4501-configurable-double-mutability branch from 59a907d to fb661f5 Compare February 1, 2022 13:49
@rocketraman
Copy link
Contributor Author

You should now be able to rebase +1

Done

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Great contrib 🙏 LGTM

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I really like this one! Thanks :)

@BraisGabin BraisGabin merged commit c854a2e into detekt:main Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule suggestion: In Compose, forbid var with mutableStateOf (whether wrapped in remember or not)
3 participants