[SILGen]: fix a bug in isolated default argument handling #84664
Conversation
Changes in swiftlang#81370 addressed a number of issues with isolated default argument handling. However, there seemed to still be a problem when dealing with isolated default arguments of instance/static methods under the following conditions: - The method contained an isolated default parameter - The method contained a defaulted parameter that expanded to 0 or >1 values - Said parameter was followed by another defaulted parameter The attempted fix here was to adjust the isolated default argument iteration bookkeeping.
|
@swift-ci please smoke test |
|
@rjmccall if you get a chance to take a look at this proposed fix, i'd appreciate your insights on:
|
| // - delayed arguments that expanded to zero or greater than one value | ||
| // - a subsequent delayed argument |
There was a problem hiding this comment.
should probably say 'default' rather than 'delayed'
| @@ -3203,6 +3202,7 @@ static void emitDelayedArguments(SILGenFunction &SGF, | |||
| // Otherwise, reset the current index adjustment. | |||
| } else { | |||
| indexAdjustment = 0; | |||
| indexAdjustmentSite = site; | |||
There was a problem hiding this comment.
Based on a cursory look at this bit of code, it seems suspicious to me that we'd only update indexAdjustmentSite in this else branch. We were updating the indexAdjustment below in this loop unconditionally, but seemingly didn't update the indexAdjustmentSite at all, prior to this PR. Why is that?
There was a problem hiding this comment.
i assume that was an error/oversight as not updating it causes the added tests to hit assertion failures when they overwrite prior default arguments. IIUC the 'site args' stuff is perhaps a vestige from curried function call syntax. this is what Slava informed me of here:
SILGen has some vestigial support for the old curried function call syntax that was removed in Swift 3. We still use it to represent the
selfparameter, so a method has type(Self) -> (Args...) -> Resultin the AST, but in SIL it's(Args..., Self) -> Result. There are two "call sites" for a single "call", basically. But now it only ever comes up in this case.
so now i think there are only ever at most 2 'levels' of 'site args'. most of the existing test cases i saw for this isolated default args stuff appeared to use free functions and not instance methods (static or otherwise), so i'm not sure they exercised this case.
|
@swift-ci Please test |
3 participants
Changes in #81370 addressed a number of issues with isolated default argument handling. However, there seemed to still be a problem when dealing with isolated default arguments of instance/static methods under the following conditions:
The attempted fix here was to adjust the isolated default argument iteration bookkeeping.
resolves: #84647