Don't lift try-catch statements that are already in local functions

[KacperFKorban]

closes #13941

The problem in #13941 is that before tail recursion can be expanded the try is lifted to a local function, which in turn moves direct recursive calls.

[odersky]

The change looks correct to me. Can you find out more what happened for the CB timeout?

[KacperFKorban]

I'm pretty sure that the problem is with the implementation of intercept in scalatest, which somehow depends on the try-catch being lifted here. Though I'm not sure how to fix it yet.

[odersky]

I'm pretty sure that the problem is with the implementation of intercept in scalatest, which somehow depends on the try-catch being lifted here. Though I'm not sure how to fix it yet.

That looks likely, yes. Who worked on that macro? Maybe we can ask them for help? And how big is the damage? Can we disable the tests that fail individually?

In any case I think the change in this PR is important and should get in.

[KacperFKorban]

Looks like the macro implementation is the same as for Scala 2 and just uses ClassTags and Positions. I see some commits from @nicolasstucki in the same file but I'm pretty sure they are not related.

[odersky]

Does Scala 2 lift to class level as well, or do they stop at the enclosing method?

Also, it would be good to find out what method fails in ScalaTest. Maybe we overlooked something and certain kinds of DefDefs should not supporess try lifting.

[KacperFKorban]

Scala 2 seems to behave the same way here i.e. it doesn't lift in this case. I have an idea about how to fix it. It might be the case that entering a DefDef changes the needLift value for the entire scope since it is never set back to the previous value. If so, then it's just an implementation error on my side.

EDIT: it works as intended, I'll try minimizing the scalatest issue then.

[KacperFKorban]

The problematic test case is RefSpecSpec / "should generate NotAllowedException wrapping a non-fatal RuntimeException is thrown inside scope". The problem seems to be that the position is reported as if it was thrown in one scope higher than it's supposed to (lineNo: 2485 != 2488). This is most likely because one less try is lifted somewhere in the call sequence.

Decreasing the stack depth for NotAllowedException in RefSpecLike seems to fix the tests, but I'm not 100% sure about this fix. Although this would make the stack depth the same as for 2.13, which sounds correct.

Should I disable this test for now?

[odersky]

Decreasing the stack depth for NotAllowedException in RefSpecLike seems to fix the tests, but I'm not 100% sure about this fix. Although this would make the stack depth the same as for 2.13, which sounds correct.

Then it looks like this was a fix to port scalatest to 3 that should be reverted?

[KacperFKorban]

Not really, this default behavior was left since mid-2016 and was intended for Scala 2.11. I think that it just happened to work for dotty.

This means that adding a special case for Scala 3 in RefSpecLike#ensureScopesAndTestsRegistered would be required, but that's still a 2 line change.

[odersky]

Yes, let's patch it as you suggest

[KacperFKorban]

Fixed scalatest's tests. CB should work properly now.