Only check seen for LazyRef for TypeSizeAccumulator #20459
Conversation
|
I agree it would be good to think of an F-bounded test where the change proposed here would fail. I believe such tests exist, but they might be a bit tricky to find. I also agree that the current state, i.e. simply not counting types twice, is wrong. One thing we could do is have a (*) I say in principle, since these protecting LazyRefs are introduced only after the type is first constructed, in checkNonCyclic. But hopefully the check is done before we do an implicit search on a type. F-bounds are a curse, it would be so nice if we could get rid of them! |
This fixes scala#15692 and does not seem to break an existing compilation tests. The problem with seen when it comes scala#15692 is that seen means that type that has repeated types will get a lower size and this incorrectly triggers the divergence check since some of the steps involved less repeated types. The seen logic was introduced in scala#6329 and the motivation was to deal with F-bounds. Since not tests fail it not clear if this logic is still needed to deal with F-bounds? If it is still needed we can add a test and instead of removing the seen logic we can make it track only types the appear as a bound and could cause infinite recursion instead of tracking all.
Also adds test case that stackoverflows without any seen check.
eejbyfeldt
changed the title
Added such a tests case and modified the code to only stop recursion on seen inside a LazyRef. |
|
@mbovel This MR was assigned to you over a year ago, back then it was not in a mergable state. Now I believe that it is. Are you still a good a reviewer for it? Or know who might be a good candidate? |
There was a problem hiding this comment.
This PR implements @odersky's comment above:
One thing we could do is have a seen check only for
LazyReftypes. In principle(*), every F-bounded recursion should go through aLazyRef. So if we stop traversing after the first time at aLazyRef, we should still be safe.
It looks good to me!
| case _ => | ||
| foldOver(n, tp) | ||
| } | ||
| seen += tp |
There was a problem hiding this comment.
Could this be moved to the LazyRef case?
This fixes #15692 and does not seem to break any existing compilation tests. The problem with seen logic is that give types with repeated types will get a lower size and this incorrectly triggers the divergence check since #15692 some of the steps involved less repeated types.
The seen logic was introduced in #6329 and the motivation was to deal with F-bounds. Since no tests fail it not clear if this logic is still needed to deal with F-bounds? If it is still needed we can add a test that fails and instead of removing the seen logic we can make it track only types the appear as a bound and could cause infinite recursion instead of tracking all.
Edit: This PR now contains a test that fails witout the seen check and now only does the seen check for LazyRef.