Change default params representation #8637
Conversation
Store the parameter symbols of a method in a `paramss` field of the method symbol. Parameter symbols are kept up-to-date until erasure.
Add a method `paramSymss` that mirrors the type structure of a method. Rename the old `paramss` to `rawParamss` to caution against naive uses.
Also, add a nestedZipWithConserve method
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
| final def setParamssFromDefs(tparams: List[TypeDef[?]], vparamss: List[List[ValDef[?]]])(using Context): Unit = | ||
| setParamss(tparams.map(_.symbol), vparamss.map(_.map(_.symbol))) | ||
|
|
||
| /** A pair consistsing of type paremeter symbols and value parameter symbol lists |
There was a problem hiding this comment.
@nicolasstucki The paramSymss method should be exported through Tasty reflect.
There was a problem hiding this comment.
I added paramSymss to the TASTy reflect API
| api.ParameterList.of(params.toArray, mt.isImplicitMethod) :: paramLists(restpe, params.length) | ||
| assert(paramss.nonEmpty && paramss.head.hasSameLengthAs(pnames), | ||
| i"mismatch for $sym, ${sym.info}, ${sym.paramSymss}") | ||
| val apiParams = paramss.head.lazyZip(ptypes).map((param, ptype) => |
There was a problem hiding this comment.
@smarter I am not sure whether we need the parameter type ptype
in the method type to produce an api type, or whether the type of the
parameter symbol is enough. In the latter case, paramLists can be
much simpler; a simple nestedMap instead of the recursion suffices.
There was a problem hiding this comment.
We need the fully parameter type: if any of its part change, that's a possible API change and some code might need to be recompiled.
There was a problem hiding this comment.
But ptype and the parameter symbol's type are usually isomorphic, right? It's just that one refers to internal parameter symbols and the other to external paramRefs.
There was a problem hiding this comment.
Ah I see, I haven't checked this PR in detail yet so I can't say but I guess it's fine then
There was a problem hiding this comment.
I don't know about the fine print though. E.g. does internal vs external representation of varargs matter?
There was a problem hiding this comment.
As long as the varargs gets represented by an api type which doesn't match some other non-varargs type it should be fine, both representations should have this propriety but I'll double-check. Are there other situations where the types differ?
There was a problem hiding this comment.
I can't think of anything else.
|
@nicolasstucki The Tasty reflect additions look good to me. Can you do a quick review of the rest? |
Now: Have a flag
HasDefaulton each parameter symbol that has a default argument, instead of the flagDefaultParameterizedon method symbols that we had before. To make this work, we need to retain a way for methods to refer to their parameter symbols.The new scheme makes it easier to find default arguments, and corresponds to what Scala-2 does.
We should update Tasty reflect to match the new functionality.