Workaround to possible scala 3 infinite loop bug

[olabusayoT]

• Our pattern of val x = parent.x no longer works in Scala 3 because it can't detect that parent.x can be overridden to end the recursion, so it thinks we're stuck in an infinite loop because parent and this are the same type. Even though parent will eventually resolve to Root, which has a different overridden x. To trick it into seeing there will be no infinite loop, we need a new pattern where we do parent match { case r: RootType => r; case _ => parent.x } or if possible/known val x = root.x. This might actually allow scala to convert the recursion to a while loop, so it might be more efficient.

DAFFODIL-2975

[stevedlawrence]

+1

[mbeckerle]

+1

The fact that this no longer works sure feels like a Scala 3 bug to me. Is there any thing where people are defending this behavior change as somehow rationalized? I think it's simply a bug that we need to change this at all.

[stevedlawrence]

I couldn't find anything about this being intentional or a bug. But it feels like a bug to me too.

That said, this is just to fix a warning, so I think it should still work as expected. If we don't want to make this change, another option is to silence the warning, but I generally prefer to avoid that if possible. I also think the new code is actually a bit cleaner, since the recursive logic is now in one function instead of spread out between different classes with overridden members.

[mbeckerle]

I couldn't find anything about this being intentional or a bug. But it feels like a bug to me too.

That said, this is just to fix a warning, so I think it should still work as expected. If we don't want to make this change, another option is to silence the warning, but I generally prefer to avoid that if possible. I also think the new code is actually a bit cleaner, since the recursive logic is now in one function instead of spread out between different classes with overridden members.

I agree. I would add we need to report this bug. If someone had a couple small inner classes say, so that the algorithm wasn't spread out, but near adjacent, then I would have no complaint about the style of this, and I can imagine a situation where there's a bunch of these sort of run-up-the-hierarchies kinds of relationships where doing these algorithms in this style would actually make sense.

The bigger issue for me is this logic: if this doesn't work, then how exactly does inheritance of methods work in Scala 3? Because I don't understand how any implementation can fail here, yet work for all the other cases where it needs to work.

[stevedlawrence]

The more I think about it, the more I think this isn't a bug. It's just a warning saying there is the possibility that there is an infinite loop. Which technically isn't wrong. For example, image this code which is a simplified version of what we have:

class Node(parent: Option[Node]) {
  lazy val foo: String = parent.get.foo
}

class Root() extends Node(None) {
  override lazy val foo: String = "root"
}

If Node.parent eventually reaches a Root for any Node, then Node.foo will never be an infinite loop. But if you do something like this:

val n1 = Node(None)
val n2 = Node(Some(n1))
val n3 = Node(Some(n2))
n3.foo

And there is no Root parent, then that will lead to an infinite loop (or really in this case it will call None.get and throw and exception). And Scala3 can't guarantee that Node has a Root parent or a parent that overrides foo that has a base case to end the recursion, so it chooses to issue a warning that there might be an infinite loop. The warning could probably be a less concrete and say something like "Possibility for an infinite loop" instead of what it does now ("Infinite loop in function body", but it's not really wrong that there could be an infinite loop.

The bigger issue for me is this logic: if this doesn't work, then how exactly does inheritance of methods work in Scala 3?

It think it does work how you might expect, there's just now a warning that Scala thinks there might be an infinite loop. But if you ignore the warning, it work as expected.

I think then the question is what is the right way to fix it where you don't need to know all the possible parent types to end the recursion. And I think the answer is something like this:

class Node(parent: Option[Node]) {
  lazy val foo: String = parent.map(_.foo).getOrElse {
    throw new Exception("Failed to get a parent")
  }
}

There's now a branch that causes the recursion to terminate without needing to know anything about the parent nodes or if they override foo, and the warning goes away. It makes things a bit more complex, but it arguably results in safer code.

So I think the rule of thumb is that a val/def that calls itself must have a base case that ends the recursion. And that's really what this PR does. Our vals are changed to use a match case where one of the cases is recursive and the other case is the base case. That base case doesn't necessarily need know the type of all possible parents, but there needs to be something.

[mbeckerle]

I like this analysis. I agree it's not a bug other than the message being phrased poorly since the warning text makes it sound like it is always an infinite loop.

The code style where we don't use this technique at all, and just write the full recursive method in one place is clearly the right way to avoid this warning and in every concrete case I can think of, that's a clearer style.