FullParameterization: fix rewiring of Returns #383
Conversation
|
@smarter can you please include details about bug that you are fixing in commit message? |
| case Ident(_) => rewireCall(thisRef) | ||
| case Ident(_) => | ||
| val rewired = rewiredTarget(tree, derived) | ||
| if (rewired.exists) // This only happens when `tree` is the `from` part of a Return |
There was a problem hiding this comment.
If Idents inside Returns need special handling, I'd prefer to handle them on Return node level. IE add case t: Return => which has special handling of return.
|
@DarkDimius: My hope was that the commit message + the test case was enough to understand what the commit was doing, but I'll see if I can add something to the description |
smarter
force-pushed
the
fix/fullyParameterizedDef
branch
from
9e3a9aa to
a92f936
Compare
| if (rewired.exists) | ||
| Return(expr, Ident(rewired.termRef)) | ||
| else | ||
| EmptyTree |
There was a problem hiding this comment.
- That's a silent failure. Should at least issue
ctx.error - From could be an EmptyTree(which means return from enclsoing method), in this case calling
rewiredTarget(NoSymbol, derived)doesn't make sense.
Based on this I would better check iffrom.isEmptyand only if nonempty rewirefrom
There was a problem hiding this comment.
Ahh, EmptyTree here is a sentinel value. Disregard my comment about ctx.error
The `from` field of a Return tree should either be EmptyTree or an Ident corresponding to the method we're returning from.
smarter
force-pushed
the
fix/fullyParameterizedDef
branch
from
a92f936 to
a273f38
Compare
|
Issue addressed, I also replaced my use of |
|
|
FullParameterization: fix rewiring of Returns
Backport "Process Export for unused check" to 3.3 LTS
This assumes that an
Identcorresponding to a method with extensions only occurs as thefromfield of aReturn, can someone confirm that this will always be the case?EDIT: I updated the code to do a
case Returnso the assumption above is no longer needed, but I'm still curious ifIdentcan appear anywhere else, otherwise we could remove thecase IdentReview by @odersky or @DarkDimius .