Fix Closure span assignment in makeClosure

[Florian3k]

Fixes #15098

Wrong line numbers were coming from Closure. Previously it's span was inherited from block end position, now it's assigned explicitly.

This fix changes behaviour of test tests/neg/i9299.scala:

type F <: F = 1 match { // error
  case _ => foo.foo // error // error
}
def foo(a: Int): Unit = ???

Previously there were 3 errors generated:

Compiler output before fix (3 errors)

-- Error: tests/neg/i9299.scala:1:10 -------------------------------------------
1 |type F <: F = 1 match { // error
  |          ^
  |Recursion limit exceeded.
  |Maybe there is an illegal cyclic reference?
  |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
  |A recurring operation is (inner to outer):
  |
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  ...
  |
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
-- Error: tests/neg/i9299.scala:2:12 -------------------------------------------
2 |  case _ => foo.foo // error // error
  |            ^
  |Recursion limit exceeded.
  |Maybe there is an illegal cyclic reference?
  |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
  |A recurring operation is (inner to outer):
  |
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  ...
  |
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of  <: F
-- [E046] Cyclic Error: tests/neg/i9299.scala:2:15 -----------------------------
2 |  case _ => foo.foo // error // error
  |               ^
  |               Cyclic reference involving method $anonfun
  |-----------------------------------------------------------------------------
  | Explanation (enabled by `-explain`)
  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
  | method $anonfun is declared as part of a cycle which makes it impossible for the
  | compiler to decide upon $anonfun's type.
  | To avoid this error, try giving $anonfun an explicit type.
   -----------------------------------------------------------------------------
3 errors found

Now there are 2 errors:

Compiler output after fix (2 errors)

-- Error: tests/neg/i9299.scala:1:10 -------------------------------------------
1 |type F <: F = 1 match { // error
  |          ^
  |Recursion limit exceeded.
  |Maybe there is an illegal cyclic reference?
  |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
  |A recurring operation is (inner to outer):
  |
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  ...
  |
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
  |  type parameters of  <: F
  |  type parameters of F
-- Error: tests/neg/i9299.scala:2:12 -------------------------------------------
2 |  case _ => foo.foo // error // error
  |            ^
  |Recursion limit exceeded.
  |Maybe there is an illegal cyclic reference?
  |If that's not the case, you could also try to increase the stacksize using the -Xss JVM option.
  |A recurring operation is (inner to outer):
  |
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  ...
  |
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of F
  |  base classes of  <: F
2 errors found

This is because now in function UniqueMessagePositions.isHidden:

trait UniqueMessagePositions extends Reporter {
  // ...
  /** Logs a position and returns true if it was already logged.
   *  @note  Two positions are considered identical for logging if they have the same point.
   */
  override def isHidden(dia: Diagnostic)(using Context): Boolean =
    super.isHidden(dia)
    ||
      dia.pos.exists
      && !ctx.settings.YshowSuppressedErrors.value
      && (dia.pos.start to dia.pos.end).exists(pos =>
            positions.get((ctx.source, pos)).exists(_.hides(dia)))
  // ...
}

this fragment:

(dia.pos.start to dia.pos.end).exists(pos =>
    positions.get((ctx.source, pos)).exists(_.hides(dia)))

evaluates to true with third error. New position is considered as overlapping with position of previous error and it's not printed.

I think this may be desired behaviour, but I am not entirely sure.

[ckipp01]

I see a bunch of performance requests here, @mbovel was there performance concerns here, or what should we do about this pr?

[mbovel]

@ckipp01 no, not at all, I was just testing the benchmarks bot. Sorry for the spam, should have done it on an empty PR 😅

[ckipp01]

@ckipp01 no, not at all, I was just testing the benchmarks bot. Sorry for the spam, should have done it on an empty PR 😅

No worries! I'll re-trigger the CI here and see if we can move it along.

EDIT: actually I don't see the option to even do that since it's so old. @Florian3k do you mind rebasing this, and we can try to get it across the finish line?

[Florian3k]

@ckipp01 The story behind this PR is that my fix did not pass some tests in completions. I tried to fix it, but without success.

[OndrejSpanel]

What will be the fate of this issue? Unreliable debugger variables on last lambda line are quite unfortunate.

See also https://youtrack.jetbrains.com/issue/SCL-22145 and https://youtrack.jetbrains.com/issue/SCL-22144

This is something even users using no advanced language features will hit, as lambdas are everywhere. (For-comprehensions anyone?)

[Florian3k]

We recently got back to this, and I think we're close to fixing this (thanks to @rochala for helping with completions).
I don't want to make any promises, but hopefully next week 🤞

[OndrejSpanel]

Could this be backported to 3.3.4? The issue makes debugging any code with lambdas, esp. single line lambdas, very hard.