Avoid blowup of compute times for ill-formed retains #24564
Conversation
This reverts commit 52db678.
- Completely drop all retains-like annotations if cc is not enabled somewhere. This is the same as in the reverted commit but now done in Annotation.mapWith - Strip nonsensical parts of retains-like annotations to avoid blowup. - Revise Annotation#refersToParamOf to account for type arguments (this is a correctness fix; we could have gotten wrog behavior before).
| if arg.isType then | ||
| arg.tpe.existsPart(isLambdaParam, stopAt = StopAt.Static) | ||
| else | ||
| arg.existsSubTree: |
There was a problem hiding this comment.
Great, that partially (or completely?) fixes #22008 (and is similar to what I suggested in #22001 (comment)).
There was a problem hiding this comment.
Ah, good. I had not seen that issue before! I discovered this after a lengthy debug session where I wondered why my changes to mapWith would cause lots of errors with orphan parameters in pickling. So I assume we can close #22008? I see that not: Term trees generated by macros might still cause problems.
There was a problem hiding this comment.
I discovered this after a lengthy debug session
Sorry for the loss of time; we should probably have added a TODO when we discussed the bug first.
I see that not: Term trees generated by macros might still cause problems.
Yes, that's a concern.
And outside of macros, are we sure that TermParamRef really can't appear in types of trees other than Ident, This or TypeTree? I don't know how to validate that assumption. Morally, I still feel the safe and correct implementation should be to visit each type part of each tree as in #22001 (comment). But that might be a performance hit.
|
|
||
| def sanitize(tp: Type): Type = tp match | ||
| case SkolemType(_) => | ||
| SkolemType(defn.AnyType) |
There was a problem hiding this comment.
I trust the author about the capture checking part 😄
Co-authored-by: Matt Bovel <matthieu@bovel.net>
|
Unrelated intermittent network failure in scala-library-sjs, I relaunched it. |
2 participants
This reverts one commit in #24556 but keeps its logic. It just moves the code elsewhere.