[SILGen]: ban forward captures of let temporary allocations

[jamieQ]

This addresses the same problem as #37935
but handles address-only lets rather than vars.

fixes: #85028

[jamieQ]

@swift-ci please smoke test macos

[jamieQ]

hmm... got this when building some stuff in Runtimes/Core:

07:01:44  419 |   if _openExistential(type(of: value as Any), do: _isOptional) {
07:01:44  420 |     let debugPrintable = value as! CustomDebugStringConvertible
07:01:44      |         `- note: constant defined here
07:01:44  421 |     debugPrintable.debugDescription.write(to: &target)
07:01:44      |                    `- error: constant 'debugPrintable' used before being initialized
07:01:44  422 |     return
07:01:44  423 |   }

haven't been able to repro locally, but maybe there's something more complex going on here...

---

current changes cause a spurious DI error on this pattern:

protocol P {}
func fail(_ a: Any) {
  let p =  a as! P
}

bb0(%0 : $*Any):
  debug_value %0, let, name "a", argno 1, expr op_deref // id: %1
  %2 = alloc_stack [lexical] [var_decl] $any P, let, name "p" // users: %9, %3
  %3 = mark_uninitialized [var] %2                // users: %8, %6
  %4 = alloc_stack $Any                           // users: %7, %6, %5
  copy_addr %0 to [init] %4                       // id: %5
  unconditional_checked_cast_addr Any in %4 to any P in %3 // id: %6
  dealloc_stack %4                                // id: %7
  destroy_addr %3                                 // id: %8
  dealloc_stack %2                                // id: %9
  %10 = tuple ()                                  // user: %11
  return %10                                      // id: %11
} // end sil function '$s3bug4failyyypF'

i think the issue now is that DI is treating the unconditional_checked_cast_addr as an escape

[slavapestov]

That regression ought to be fixed of course, but independently of that we might want to ask them to move away from _openExistential(). These days with -swift-version 6 it ought to be possible to use implicit opening for everything, and _openExistential() is kind of broken in that incorrect usage of it will crash the compiler.

[jamieQ]

@slavapestov do you think updating SILGen & DI is the best (or least effort...) way to go about addressing this? i was wondering if alternatively there's some way it could be detected/banned in Sema...

[kavon]

i think the issue now is that DI is treating the unconditional_checked_cast_addr as an escape

I would expect that DI needs to treat this instruction as initializing the to address %3 here, so something like DIUseKind::Initialization.

[slavapestov]

@jamieQ I think catching this in DI is your best bet, since it already does the right thing for loadable types AFAIK.

[jamieQ]

@swift-ci please smoke test macos

[jamieQ]

okay i've looked into some of the relevant code here and had a go at solving this in a few ways. some thoughts:

1. update SILGenDecl & DI

changes:

• unconditionally add a mark_uninitialized instruction into SILGenDecl logic for let declarations that require temporary buffers
• adjust DIMemoryUseCollector to treat some address cast instructions as inits to account for regressions this introduced into let bindings containing certain dynamic casts

this strategy seems to fix the patterns discovered, but i ran into the following issues:

1. it perturbs numerous tests that expect particular SSA registers registers to contain specific things, and inserting a mark_uninitialized instruction throws things off. haven't looked into all the test failures that deeply, but i assume most would be straightforward (if somewhat toilsome) to fix.
2. the logic feels a bit 'scattered' throughout various places. this is probably inescapable given how DI works, but makes it much more difficult (for me) to try and think through the consequences analytically.
3. something about how it's currently implemented causes failures with parameter pack codegen patterns (have not yet looked into this further)

2. update SILGenFunction

most existing detection of this problem seems like it doesn't actually come from DI, but from SILGenFunction – there is logic Slava added a while ago to ban 'forward captures'. said logic is what currently catches things like this where the type is not address-only:

let fwdInt: Int = { () -> Int in fwdInt }()

these cases don't actually make it to DI b/c SILGen errors out when attempting to emit the closure captures. i'm not yet certain why this handling doesn't work the same way for the address-only cases though, and haven't determined if there's some minor adjustment that could be made to the existing logic here to cover those additional cases.

3. update TypeCheckCaptures

DI already handles 'two-phase' initialization of this pattern, but 'direct' initialization seems like a simpler case to detect & analyze, and i think could be identified during typechecking. i have another rough draft PR here to test out this concept. the current implementation approach adds a check into TypeCheckCaptures that looks for a decl ref being considered as a potential capture that's within it's decl's initializer. it just uses a simple source range containment check at the moment, but the check could presumably be done via an AST walker, and potentially even moved out as its own pass in MiscDiagnostics or something.

the perks of this approach IMO is that it could be pretty self-contained and is less 'invasive' – it won't (i think) mess with any existing tests, nor change existing codegen.

---

currently i'm leaning toward approach 3., but 2. would also be a decent candidate if it can be made to work in this case. 1. seems potentially doable, but seems like it has more unknowns and a larger 'blast radius'. would appreciate any thoughts you may have on this @kavon @slavapestov – thanks for your time!

---

edits:

• Slava also added this relevant-looking fix for a similar-looking issue a while back – SILGen: Don't allow referencing the 'var' box before it's fully initialized #37935. adopting something like this looks promising, as i think the address-only let case is quite similar.

[jamieQ]

@swift-ci please smoke test macos

[jamieQ]

@swift-ci please smoke test macos

[slavapestov]

Looks like the tests passed, good job! Are you more confident in this approach now?

[jamieQ]

Looks like the tests passed, good job! Are you more confident in this approach now?

@slavapestov i changed tack somewhat – i found this old PR of yours that i think basically handled this problem for vars, and updated the implementation to try and do the analogous thing for lets, so we will still avoid having to change DI. it seems to 'work' in that it produces the desired diagnostics in SILGen. unfortunately some (all?) of the bad patterns crash further along in the pipeline in something to do with the 'closure lifetime fixup' pass. i don't yet understand enough about the cleanup scopes, but something is tripping an assertion before the diagnostics can be emitted. so... short answer 'yes' i'm more confident in this approach, since it seems like it's basically what's done in most other analogous cases, but still not certain the implementation is sufficient yet. though i suppose maybe a crash is better than a successful UB compile?

edit: seems the crashes in the optimizer have mysteriously disappeared for me locally, so maybe i was in a weird state or had left some code enabled that i should have removed. anyway, will see how CI likes this approach soon.

[jamieQ]

@swift-ci please smoke test

[jamieQ]

@swift-ci please smoke test linux

[jamieQ]

@kavon @slavapestov – this approach seems to work and made it through the CI gauntlet (so far). lmk what you think

[hamishknight]

FYI for this particular case I'm hoping we can just ban it in Sema (de3dc77)

[jamieQ]

@hamishknight thanks for the heads-up! i can't say i totally follow the implications of that change yet 😅, but to try and clarify – are the cases you'd expect to be possible to catch in Sema something like: variable bindings with (immediate) initializers containing closure expressions that reference the binding itself? would you expect it to cover all the test cases i've added here, or just certain ones?

---

also, more tangentially, do you have a sense if your proposed changes could potentially alleviate the need for what is currently done in the var case in SILGen too (i see you moved some existing tests out of this file)? i.e. with the approach you're considering do you think the SILGen approach where the var is 'hidden' until the initializer emission is completed could be removed in favor of:

1. your proposed detection in Sema (to catch immediate inits with forward references)
2. existing logic in DI (to catch flow-sensitive violations)

[hamishknight]

are the cases you'd expect to be possible to catch in Sema something like: variable bindings with (immediate) initializers containing closure expressions that reference the binding itself? would you expect it to cover all the test cases i've added here, or just certain ones?

So my change takes the existing rule we have in Sema that prevents you from doing this:

  let a = a // error: use of local variable 'a' before its declaration
  _ = b // error: use of local variable 'b' before its declaration
  let b: Int

And extends it such that it also considers any use nested in a closure, which is already illegal today in most cases, and is necessary for correctness since we can't support forward references in closure bodies in the constraint system as the bodies are solved in order. Unfortunately it breaks one project in the source compat suite (since you can do such forward references today with lazy), so I think there will need to be more discussion on that change, so it doesn't need to block this PR.

I don't expect that change to cover all of the cases you're handling here since it purposefully doesn't handle nested func declarations, which are currently allowed to forward reference things. Although I think we probably ought to revisit that since the current behavior can be pretty confusing e.g:

let x = 0
do {
  func fn() {
    print(x) // prints 1
  }
  defer {
    fn()
    print(x) // prints 0
  }
  let x = 1
}

[jamieQ]

gotcha, thanks for the explanation. and... wow is that last example confusing 🙃

[jamieQ]

@kavon @jckarter – any thoughts on this? i think it should be a pretty safe change since it essentially just copies the existing logic for vars over for the address-only let case