Make sure captured types are listed before terms #17144
Conversation
nicolasstucki
force-pushed
the
splice-targ-order
branch
3 times, most recently
from
9aec519
to
ac0c77c
Compare
nicolasstucki
force-pushed
the
splice-targ-order
branch
2 times, most recently
from
f42366d
to
738c3aa
Compare
| val (refs, bindings) = refBindingMap.values.toList.unzip | ||
| val (typeRefs, typeBindings) = typeBindingMap.values.toList.unzip | ||
| val (termRefs, termBindings) = termBindingMap.values.toList.unzip |
There was a problem hiding this comment.
Not new in this PR, but Maps by default don't have a deterministic iteration order. Shouldn't a LinkedHashMap be used for determinism?
There was a problem hiding this comment.
It would be better. I will use LinkedHashMap.
| * a single list. For backwards compatibility we read holes from tasty as | ||
| * if they had no targs and have only args. Therefore the args may contain | ||
| * type trees. |
There was a problem hiding this comment.
Would it be possible to instead have the Unpickler separate the type args from the term args when unpickling an older tasty file?
There was a problem hiding this comment.
That would be ideal. Unfortunately, we cannot do that because we must pass all these arguments in the order they were listed. This is a consequence of the old format's interleaved type and term arguments. Specifically, the functions of ExprHole.V1 and ExprHole.V2 take these arguments in the order they are listed in the hole.
| * It is only used when encoding pickled quotes. These will be encoded | ||
| * as TastyQuoteHole when pickled. |
There was a problem hiding this comment.
I have read the documentation of Hole and TastyQuoteHole twice but I'm still confused on which one is used when, could this be described in more details, or could the documentation link to some other part of the codebase where this is explained?
There was a problem hiding this comment.
Updated the documentation
smarter
added
the
needs-minor-release
nicolasstucki
force-pushed
the
splice-targ-order
branch
2 times, most recently
from
f0efb27
to
7285496
Compare
nicolasstucki
removed
the
needs-minor-release
|
This PR does not need a minor release. The minor release will be needed for the last step in #17225 where we change the representation of the hole in the TASTy format. |
| * (U$2: Type[U], x1$: Expr[T], x2$: Expr[U]) => // body of this lambda does not contain references to x1 or x2 | ||
| * (q: Quotes) ?=> f('{ @SplicedType type U$3 = $[ `0`: U | U$1 ]; g[U$3](${x1$}, ${x2$}) }) |
There was a problem hiding this comment.
The parameter is called U$2 but I see U$1 used here, is that intentional? I also still think it'd be helpful to have some explanation of the $[ ... ] syntax somewhere
There was a problem hiding this comment.
I think we need to figure out how to handle types that depend on terms (#17072 (comment)) to decide if we should merge this.
nicolasstucki
force-pushed
the
splice-targ-order
branch
from
7285496
to
0427731
Compare
Noticed in #17144 (comment), but that PR was not merged.
Common change from #17225, #17140 and #17120. One of them will be the fix for #17137. Probably #17225 will be the chosen solution.