Ensuring forward references to local variables are consistently rejected in closures

Evolution Discussion
1

Hey folks, I'd like to propose a fix for the checking of forward references to local variables in the type-checker. Today we forbid forward referencing local variables, e.g the following is invalid:

func foo() {
  let y = x // error: use of local variable 'x' before its declaration
  let x: Int = x // error: use of local variable 'x' before its declaration
}

However we do not currently handle these cases in the type-checker when the forward reference is nested in a closure, e.g:

func foo() {
  let y = { x }
  let x = 0
}

In practice most of these cases still end up being rejected, they are instead caught by SILGen when we go to form the closure, e.g the above emits the error closure captures 'x' before it is declared. However there are a couple of cases that SILGen permits, which I'd like to consistently reject in the type-checker.

These are:

  • lazy local variables with initializers that forward reference a local variable from a closure,
    and local variables with attached macros that similarly relocate the initializer into an accessor, e.g:

    func foo() {
  lazy var x = { y }
  let y = 0
}

    func foo() {
  @LazyLikeMacro var x = { y }
  let y = 0
}

  • Forward references to local computed variables from a closure, e.g:

    func foo() {
  var x = { y }
  var y: Int { 0 }
}

Both of these cases are already rejected today when closures aren't involved. I'd like to reject them consistently. In general we cannot fully support forward references to local variables in the type-checker (at least not without a significant overhaul) since it violates the method by which we type-check closure bodies, where each element is solved individually in source order.

In principle we could always allow forward references to local computed variables, but I think it's better to have a consistent rule for all local variables, regardless of whether they are stored or computed. Lifting the restriction for local computed variables would also potentially be source breaking in the other direction for cases where a reference is not within a closure and is currently relying on shadowing behavior, e.g:

struct S {
  var x: Int
  func foo() {
    let y = x // Currently refers to `self.x`, would change to the local variable
    var x: Int { 0 }
  }
}

Enforcing a consistent rule ensures we get consistent shadowing behavior, avoiding unexpected surprises. For example the following case now has consistent behavior both inside and outside the closure:

struct S {
  var x: Int

  func foo(_ xs: [Int]) {
    print(x) // Refers to `self.x`

    // Currently invalid since it refers to the local variable below, will
    // now refer to `self.x` and be valid.
    let xs = xs.filter { $0 > x }

    // ...
    let x = 0
  }
}

I have opened a PR that implements the proposed fix: [Sema] Catch use-before-declarations in nested closures by hamishknight · Pull Request #85535 · swiftlang/swift · GitHub

12 Likes
2

I'm in favor of anything we do to simplify name lookup behavior in the language.

I only skimmed your PR and tests, but I don't see if it also bans the following, which I think should also be banned:

func f() {
  let fn = { g() }

  func g() {
    _ = x
  }

  var x = 123
}

That is, today we allow closures to forward-reference local functions, and for local functions to forward-reference local variables. There is no reason for that to be the case, and ASTScope has some weird code that could probably be removed if that was disallowed. (If possible, we should also ensure that ASTScope doesn't return these kinds of invalid results at all. This would be even better than diagnosing and detecting this in the type checker.)

The only kind of local declaration that we need to be able to forward reference is a local function, because you can write mutually-recursive local functions:

func f() {
  func g() { h() }
  func h() { g() }
}
3 Likes
3

I agree that most uses of local funcs should be properly sorted after the func just to make the shadowing behavior more sensible. I'm a little torn about uses within computed accessors, though, since they might reasonably want to participate in the same kind of mutual recursion that local funcs can.

3 Likes
4

If we modify the last example to declare local x as a computed variable, then we get a silent behavior change:

struct S {
  var x: Int

  func foo(_ xs: [Int]) {
    print(x) // Refers to `self.x`

    // Currently refers to the local variable below,
    // will now refer to `self.x`.
    let xs = xs.filter { $0 > x }

    // ...
    var x: Int { 0 }
  }
}
1 Like
5

Ah sorry I should have mentioned this in the original post. I agree we ought to ban this too, I was planning on leaving it for a follow-up though since I think the closure case is a lot more clear-cut as we already reject most uses in practice already. It's also necessary for type-checker correctness and fixes a class of compiler crasher where we try to type-check bindings independently of their closure context, as such it's not really something we can stage in with a new language mode (although if necessary we could do this for the local computed variable case).

Local functions on the other hand don't pose any type-checker correctness issues AFAIK, and are different enough that I think there may need to be more discussion on what the semantics should be, and could potentially be something that we stage in through a new language mode.

2 Likes
6

Yes this is indeed unfortunate.

The source compatibility testing I've done so far indicates that these closure forward reference cases should be pretty rare in practice though. There was 1 case in the source compatibility suite, although that was from a dependency on an old revision of swift-syntax, and the problematic code had already been removed a while ago.

From Swift adopters within Apple, I've only found 5 cases so far, 2 of which were also memory safety bugs (which were actually fixed recently by [SILGen]: ban forward captures of let temporary allocations by jamieQ · Pull Request #84951 · swiftlang/swift · GitHub independent of the proposed change).

As mentioned above, this isn't something we can fully stage in with a new language mode since it affects type-checker correctness. We could do if necessary for the local computed variable case, but from the 6 total uses found so far, only 2 of which involved such a case.

2 Likes