Improve UnnecessaryLet

[BraisGabin]

This PR will make UnnecessaryLet a bit more restrictive. let is overused (as other socped functions) in Kotlin. This PR makes UnnecessaryLet a bit smarter. The main different between this new implementation and the old one is to treat different this two cases:

fun foo() {
  val a: Any? = getA()
  a?.let { print(a) } // can be replaced by `if (a != null) println(a)`
  a?.let { print(it) } // can be replaced by `if (a != null) println(a)`
}

class A {
  var a: String? = null

  fun foo() {
    a?.let { print(it) } // needed for thread safety
  }
}

and don't allow let when with is a better fit:

binding.textView.let {
  it.text = "hi"
  it.visibility = VISIBLE
  it.color = 0xffff0000
}

Should be replaced by:

with(binding.textView) {
  text = "hi"
  visibility = VISIBLE
  color = 0xffff0000
}

The reason for this second change is this one:

Grouping function calls on an object: with

From https://kotlinlang.org/docs/scope-functions.html#function-selection

---

IMPORTANT: These changes could be controversia,l for that reason I didn't implement the changes in the rule yet. I just changed the documentation and the tests. I want to know if you agree with this changes to start implementing the changes.

---

Closes #4248

[marschwar]

I really like those changes. As you mentioned let is easily over used and this would help guide (new) kotlin developers.

[BraisGabin]

I'm marking this PR as "help wanted". I don't know how to implement this rule correctly. So plese, feel free to take this branch and finish it.

The reason I'm not continuing developing this is because I don't know how to get the information I need from the PSI and the BindingContext to finish it. To be more precise I need to know this: #4291.

In the part of the algorithm I think that I know what should I check to make this work. All the usages of let follow one of this two cases:

X{?}.let { Y -> code } (where Y could be omitted and use it)

if (the usage of let could be changed by just an usage of `.` or `?.`) {
  Unnecessary usage.
  return;
}
if (nullSafeCall used && the output of let is consumed) {
  Legit usage
  return;
}
if (Y is not used) {
  Unnecessary usage.
  return;
}
if (the referene of X can not be changed) {
  Unnecessary usage.
} else {
  if (nullSafeCall used) {
    Legit usage
  } else {
    if (Y is only used once) {
      Unnecesary usage
    } else {
      Legit usage
    }
  }
}

I simplified the case where desctructuring declaration is used. But that just make a bit more complex the checks over Y. But the main algorithm should be the same.

X{?}.let(function)

if (nullSafeCall used && the output of let is consumed) {
  legit usage
} else {
  Unnecesary usage
}

I can be wrong here but I think that this should cover all the cases.

[BraisGabin]

I'm closing this. I tried to go back to this and I can't decide what should be flagged and what should not. There are a lot of blurry lines. I think that we first need to list which are the proper uses of let and then forbid the other ones. We could even "tag" they type of usages and then add flags to allow or disallow those type of usages.