Skip to content

[SILGen]: fix a bug in isolated default argument handling #84664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 30, 2025
Merged

[SILGen]: fix a bug in isolated default argument handling #84664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/SILGen/SILGenApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3202,6 +3202,7 @@ static void emitDelayedArguments(SILGenFunction &SGF,
// Otherwise, reset the current index adjustment.
} else {
indexAdjustment = 0;
indexAdjustmentSite = site;
Copy link
Member

@kavon kavon Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jamieQ jamieQ Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 self parameter, so a method has type (Self) -> (Args...) -> Result in 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.

}

assert(!siteArgs[argIndex] &&
Expand Down
63 changes: 63 additions & 0 deletions test/SILGen/isolated_default_arguments.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,66 @@ func useCallerIsolatedDefaultArg() async {
// CHECK: // function_ref default argument 0 of
// CHECK-NEXT: [[GEN:%.*]] = function_ref @$s4test24callerIsolatedDefaultArg1xySi_tYaFfA_ :
// CHECK-NEXT: [[ARG:%.*]] = apply [[GEN]]()

// The following tests failed due to a book-keeping error when processing delayed
// isolated arguments in methods with:
//
// - a `self` parameter
// - delayed arguments that expanded to zero or greater than one value
// - a subsequent delayed argument
Comment on lines +169 to +170
Copy link
Contributor Author

@jamieQ jamieQ Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably say 'default' rather than 'delayed'

//
// https://github.com/swiftlang/swift/issues/84647

@MainActor
enum E {
static func tupleIsolatedDefaultArgStatic(
_: Int,
x: Int = main_actor_int_x(),
// need not be isolated defaults
tup: (Int, Int) = (1, 2),
i: Int = 0
) {}

static func voidIsolatedDefaultArgStatic(
_: Int,
v: Void = main_actor_void(),
// need not be isolated defaults
tup: (Int, Int) = (1, 2),
i: Int = 0
) {}
}

func testTupleIsolatedDefaultArgStatic() async {
await E.tupleIsolatedDefaultArgStatic(0)
}

func testVoidIsolatedDefaultArgStatic() async {
await E.voidIsolatedDefaultArgStatic(0)
}

@MainActor
final class Klazz {
func tupleIsolatedDefaultArgInstanceMethod(
_: Int,
x: Int = main_actor_int_x(),
// need not be isolated defaults
tup: (Int, Int) = (1, 2),
i: Int = 0
) {}

func voidIsolatedDefaultArgInstanceMethod(
_: Int,
v: Void = main_actor_void(),
// need not be isolated defaults
tup: (Int, Int) = (1, 2),
i: Int = 0
) {}
}

func testTupleIsolatedDefaultArgInstanceMethod(_ klazz: Klazz) async {
await klazz.tupleIsolatedDefaultArgInstanceMethod(0)
}

func testVoidIsolatedDefaultArgInstanceMethod(_ klazz: Klazz) async {
await klazz.voidIsolatedDefaultArgInstanceMethod(0)
}