New behavior of "redundant let" inspections in 1.2.60 IDE plugin


#1

Kotlin 1.2.60 plugin started adding a warning with a “Remove redundant .let call” suggestion, to the following code

users
    .filter { it.hasEmail() }
    .flatMap { it.posts }
    .groupBy { it.localDate.month }
    .let { Ready(it) }

It now wants to change it to

Ready(users
    .filter { it.hasEmail() }
    .flatMap { it.posts }
    .groupBy { it.localDate.month })

which I find uglier. The “wrapper” class is not really so important as to have to be first, and in these “vertical” pieces of code, I would prefer being able to write it at the end as a final “wrap this around the result” step.

Another example is using Realm and loading detached entities - the final copyFromRealm step is more readable when it’s at the end of the query.

with (realm) {
    // ...

    val users = where<User>()
        .`in`(ID, userIds)
        .equalTo(COUNTRY, country)
        .findAll()
        .let { copyFromRealm(it) }
}

Any possibility of reversing this decision in future versions?


#2

I agree that your preferred version looks better. But the warning is quite useful in general, too. It would be the best if they could adjust it to better understand situations like this and don’t trigger a warning for them.


#3

Please submit an issue to http://kotl.in/issue with a self-contained piece of code that shouldn’t trigger the inspection. Thanks!


#4

Probably this inspection should have some sort of “inner block” size limit or line limit (only for single line content).


#5

I think we should limit let receiver call chain length. Probably with length of three or more inspection should not be reported. Or, may be, we should accept an idea about line limit.


#6

Maybe the combination of both (configurable the standard way). In the end of the day it is a heuristic about what will actually work.


#7

Currently, using method reference in let chains will not trigger this warning. Is this intentional?


#8

No, it just wasn’t implemented.


#9

Issue created: https://youtrack.jetbrains.com/issue/KT-26289


#10

Thanks for creating the issue. Another thing I just noticed today is that I also have some code where the let is not at the end of the chain, and it also triggers this warning, e.g.

return pointsInArea
    .sortedWith(distanceComparator)
    .take(fetchLimit)
    .let { copyFromRealm(it) }
    .toMutableList()

#11

Please consider this warning as well:

http?.let {
    it.authorizeRequests()
            .antMatchers("/publicKey").permitAll()
            .anyRequest().authenticated()
            .and()
            .httpBasic()
            .and()
            .csrf().disable()
}

Here also, it is asking to remove let and write it in single line
e.g.

http?.authorizeRequests()?.antMatchers("/publicKey")?.permitAll()?.anyRequest()?.authenticated()?.and()?.httpBasic()?.and()?.csrf()?.disable()

another solution it is giving as

 http?.run {
            authorizeRequests()
                    .antMatchers("/publicKey").permitAll()
                    .anyRequest().authenticated()
                    .and()
                    .httpBasic()
                    .and()
                    .csrf().disable()

        }