Fix #10951: update typedDynamicSelect #10952
Conversation
|
could someone restart the CLA check please? |
There was a problem hiding this comment.
That change would re-typecheck the function arguments. This is usually a big no-no, since arguments can be arbitrarily large and arbitrarily nested. So there's a risk of an exponential performance blowup for certain cases.
Can we find a way to do this without throwing away the types of the arguments?
|
I added |
| @@ -1076,7 +1076,7 @@ trait Applications extends Compatibility { | |||
| case _ => | |||
| } | |||
| def tryDynamicTypeApply(): Tree = typedFn match { | |||
| case typedFn: Select if !pt.isInstanceOf[FunProto] => typedDynamicSelect(typedFn, typedArgs, pt) | |||
| case typedFn: Select if !pt.isInstanceOf[FunProto] => typedDynamicSelect(typedFn, typedArgs.map(untpd.MemoizedTypeTree(_)), pt) | |||
There was a problem hiding this comment.
Let's see whether we find a way to avoid MemoizedTypeTree. Why did it not work with typedArgs in the first place? What issue gets fixed by wrapping the arguments in MemoizedTypeTree?
There was a problem hiding this comment.
the issue with typedArgs was that after
tree.args: List(Tuple(List(Ident(Int), Ident(Int))))
gets typed to
typedArgs: List(AppliedTypeTree(TypeTree[TypeRef(...,class Tuple2)],List(Ident(Int), Ident(Int))))
it fails when it tries to type check the AppliedTypeTree again because typedAppliedTypeTree sets pt to AnyTypeConstructorProto
https://github.com/lampepfl/dotty/blob/04ea25cfe9c77f612964ba306f070ba058ee695c/compiler/src/dotty/tools/dotc/typer/Typer.scala#L1700-L1703
So I wrapped typedArgs with MemoizedTypeTree so that typedTypeTree could know that the args were already type checked and return the cached results instead of an error type. doing a hasType check + cast worked as well, but having an explicit memoize type tree seemed like a safer solution.
There was a problem hiding this comment.
Thanks for explaining! We actually already have an analogous memoization mechanism in place. It's untpd.TypedSplice. Can you try to simply replace MemoizedTypeTree with untpd.TypedSplice?
There was a problem hiding this comment.
thanks for the suggestion! i've updated the PR
fixes #10951
pass
List[untpd.Tree]totypedDynamicSelectinstead ofList[Tree]. This resolves the issue, but I have no idea if this is the correct fix. just seemed like the solution based on howtypedDynamicApplyis implemented.