lint trivially self-recursive extension methods

[SethTisue]

broadens Martin's fix for #13542, as suggested by Guillaume in a comment on that PR

fixes #9880

[SethTisue]

sorry to take an issue labeled "spree", but Dale and I actually worked through this explicitly as if we were spree-ing, with me using VS Code + Metals (instead of my usual Emacs or IntelliJ) as practice for doing the same thing with a newcomer at the next spree

[SethTisue]

CI is failing because&& and || are conditionals, but aren't among the tree shapes that Martin excluded. I'll work on it.

[griggt]

I was going to push an even broader version of this I have that also covers lazy vals, based on the work we did on our spree ticket. It would fix issues 9880, 10947, 12943, and 13011.

Seems like a lot of tickets converged here.

[SethTisue]

@griggt heh. did you already handle the || and && thing?

[griggt]

We were focused on the lazy vals stuff, and I hadn't tried running a bootstrapped test since broadening to cover 9880, so had not seen the && and || thing pop up.

It would have come up tonight when opening a PR though, so thanks for catching it first! :-)

[SethTisue]

@griggt happy to let your more ambitious effort go forward instead of this one, but let us know if you get stuck or need reviewers/collaborators

Dale noticed when he looked at #13011 that isInfiniteRecCall in TailRec already exists; have you looked at that and considered where it could be DRYed with CheckLoopingImplicits?

[SethTisue]

Dale and I suspect that Martin's version of the check could issue a false-positive warning in a case such as:

class Parent:
  val p: Parent = new Child
  def foo: Unit = p.foo
class Child extends Parent:
  override def foo = {}

here Parent#foo appears to be trivially self-recursive but there is a loophole through subclassing where it isn't always self-recursive

this is similar to the well-known thing where @tailrec requires that this loophole be closed via final method

but is the false positive in this case actually a concern? maybe not

but perhaps the trickiness of reasoning about this argues for the DRYing I suggested in my previous comment. (but we haven't really considered how feasible/hard that DRYing might be)

[smarter]

but is the false positive in this case actually a concern? maybe not

I would say yes, any warning on by default should have a vanishingly small false positive rate.

[dwijnand]

Martin's original was for given/implicits, where redefining in a subclass is plausible but I couldn't think of a reasonable real-world example of that. Extension methods can't be redefined, so they're ok. For Tom's lazy vals (IIRC!) can't be redefined either, so still good. But this is relevant if we want to go for the broader approach (which Tom mentioned).

[smarter]

Extension methods and lazy vals can be overridden.

[dwijnand]

Derp

scala> class Bar extends Foo { override lazy val foo = 2 + 2 }
// defined class Bar
...
scala> class BarOps extends FooOps:
     |   extension (c: Circle)
     |     override def circumference: Double = 0.0
     |     override def foo = 2
     |
// defined class BarOps

[SethTisue]

@griggt any update? (Dale and I were thinking of finishing this more modest PR up anyway.)

[griggt]

Ah, sorry, I've been swamped lately. But please continue your work and don't let me hold up progress, I likely wouldn't have time to look at this until the weekend at the earliest.

[SethTisue]

As an aside that might be interesting to some...

CheckLoopingImplicits already has code exempting by-name arguments, so the need to special-case && and || wouldn't have arisen if those operators were declared to take by-name parameters.

In the Scala 2 spec, they are actually spec'ed as taking a by-name parameter; https://scala-lang.org/files/archive/spec/2.13/12-the-scala-standard-library.html has:

/** Compares two Boolean expressions and returns `true` if both of them evaluate to true.
    *
    * `a && b` returns `true` if and only if
    *  - `a` and `b` are `true`.
    *
    * @note This method uses 'short-circuit' evaluation and
    *       behaves as if it was declared as `def &&(x: => Boolean): Boolean`.
    *       If `a` evaluates to `false`, `false` is returned without evaluating `b`.
    */
  def &&(x: Boolean): Boolean

but Dale found that there's also this comment from Paul:

  // Compiler won't build with these seemingly more accurate signatures
  // def ||(x: => Boolean): Boolean
  // def &&(x: => Boolean): Boolean

so something blocked Paul from making that change in Scala 2 at some past point in time, but perhaps the same blocker wouldn't exist in Scala 3. But we choose not to try that experiment at the moment.

[SethTisue]

I see that #13749 has appeared — it looks orthogonal to me

[SethTisue]

rebased. remains ready for review.

[smarter]

To make sure review requests don't get lost, please both ask for review and assign the reviewer to your PR.

[smarter]

Is the issue with false positives mentioned in #13709 (comment) now fixed?

[dwijnand]

Is the issue with false positives mentioned in #13709 (comment) now fixed?

Good question. I only just became aware that you can override extension methods, so I'm not entirely sure... The problem is when you invoke a method on a class and it dispatches onto a subclass, thus making it non-self-recursive at runtime even though it might look like it is at compile time (naively). But I don't think you can do that with extension methods. Here's my attempt:

abstract class Foo:
  def parent: Foo

class FooOps:
  extension (x: Foo)
    def doFoo: Int =
      val ops = new Foo2Ops
      import ops._
      x.parent.doFoo

class Foo2Ops extends FooOps:
  extension (x: Foo)
    override def doFoo: Int = 2

This isn't going to be self-recursive, because it's referencing the subtypes doFoo, not the self-reference doFoo (if I haven't messed up). Can you come up with a reasonable counter-example we should consider?

[smarter]

Do you mean the issue depends on the same symbol being used in the body? Then I could imagine:

      val ops: FooOps = new Foo2Ops
      import ops._
      x.parent.doFoo

[dwijnand]

That false positive counter-example seems too farfetched to me. It also doesn't seem like a problem-at-a-distance anymore, with FooOps using Foo2Ops and Foo2Ops extending FooOps. This is, in comparison to the original case where Symbol and its owner is used in an extension method and throwing a false positive.

Did you have an easy solution in mind, because otherwise I would wait for a real world case to appear before making this more complex. WDYT?

[SethTisue]

@smarter I don't think your concern about false positives should be considered a blocker for merging this PR, because:

1. the code pattern in question seems like something highly artificial that we dreamed up rather than something that would plausibly appear in real code
2. anyway it's a merely a false-positive warning which can be suppressed, it's not actually failing compilation, so if we misjudge the stakes aren't very high

[smarter]

How do you suppress that warning?

[SethTisue]

How do you suppress that warning?

slap : @annotation.nowarn on it, optionally with a filter

[odersky]

I think the danger of false positives is very low in this case, and the possibility of accidental recursion is higher. So LGTM.