Skip to content

[6.1][rbi] Change Region Based Isolation for closures to not use the AST and instead just use SIL. #78990

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.

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

Closed

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jan 28, 2025

This changes how region based isolation emits errors on closures. Specifically, previously we would attempt to combine AST information with SIL level information by attempting to create a correspondence in between the captures of a partial_apply and the captures of the ClosureExpr attached as a SILLocation to the partial_apply. This turns out to not be sound due to cases like the following where a partial_apply has captures from multiple different closures:

class NS {}

func test(_ x: NS, _ y: NS) {
  func local() {
    print(x)
  }
  {
    _ = y
    local()
  }()
}

which codegens to:

// closure #1 in test(_:_:)
// Isolation: concurrent
sil private [ossa] @$s4testAAyyAA2NSC_ACtFyyXEfU_ : $@convention(thin) (@guaranteed NS, @guaranteed NS) -> () {
// %0 "y"                                         // users: %4, %2
// %1 "x"                                         // users: %6, %3
bb0(%0 : @closureCapture @guaranteed $NS, %1 : @closureCapture @guaranteed $NS):

the partial_apply for closure 1 will have a SILLocation with a ClosureExpr that only contains y. If one attempts to process the closure parameters backwards using that information, one will get the wrong information. Instead of doing this, I introduced a new utility function called findClosureUses that recursively searches through closures looking for a use of the capture to emit a note upon.

If one looks at the PR it looks a little large, but most of the changes are due to my needing to add a new instruction called ignored_use. This is because the AST information that we used to use allowed us to emit diagnostics on code like:

Task {
   _ = x
}

sadly at the SIL level, we did not actually emit any code for _ = x so the findClosureUse function I added did not have a SILLocation that could be used. So I introduced a new SILInstruction called ignored_use that models _ = x appropriately namely as an instruction that suppresses unused variable warnings (that is a fake use).

Scope: Added a utility function that uses SIL instead of AST information to find closure uses to emit a diagnostic upon. This works around correctness issues exposed by the old implementation. This only comes up if we were going to already emit a diagnostic but needed to find the use that caused the diagnostic to need to be emitted. By taking this, we will eliminate crashes along this code path when we attempt to diagnose. So this cannot break any code that already works and will just provide a better experience for users since the compiler will emit the correct diagnostic instead of crashing or emitting the wrong diagnostic.
Risk: Low
Testing: Added compiler tests.
Issue: rdar://142661388
Reviewer: @nate-chandler
Original PRs:
#79028
#78837

This is used for synthetic uses like _ = x that do not act as a true use but
instead only suppress unused variable warnings. This patch just adds the
instruction.

Eventually, we can use it to move the unused variable warning from Sema to SIL
slimmming the type checker down a little bit... but for now I am using it so
that other diagnostic passes can have a SIL instruction (with SIL location) so
that we can emit diagnostics on code like _ = x. Today we just do not emit
anything at all for that case so a diagnostic SIL pass would not see any
instruction that it could emit a diagnostic upon. In the next patch of this
series, I am going to add SILGen support to do that.

(cherry picked from commit 7ae56aa)
…lack hole initialization.

(cherry picked from commit 6058b1d)

Conflicts:
	test/SILGen/sendable_to_any_for_generic_arguments.swift
…nd instead just use SIL.

The reason why I am doing this is that in certain cases the AST captures indices
will never actually line up with partial apply capture indices since we seem to
"smush" together closures and locally defined functions.

NOTE: The reason for the really small amount of test changes is that this change
does not change the actual output by design. The only cases I had to change were
a case where we began to emit a better diagnostic and also where I added code
coverage around _ and let _ since those require ignored_use to be implemented so
that they would be diagnosed (previously we just did not emit anything so we
couldn't emit the diagnostic at the SIL level).

rdar://142661388
(cherry picked from commit 082b824)
@gottesmm gottesmm requested a review from a team as a code owner January 28, 2025 20:30
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm changed the title [6[rbi] Change Region Based Isolation for closures to not use the AST and instead just use SIL. [6.1][rbi] Change Region Based Isolation for closures to not use the AST and instead just use SIL. Jan 29, 2025
…d after a try_apply or begin_apply.

(cherry picked from commit 39e63b4)
…stinguish in between captures and parameters.

Otherwise, when one diagnoses code like the following:

```
Task {
  {
    use($0)
  }(x)
}
```

One gets that $0 was captured instead of x. Unfortunately, since function
parameters do not have locations associated with them, we do not mark x
itself... instead, we mark the closure... which is unfortunate.

(cherry picked from commit 8c96a8d)
@gottesmm
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@nate-chandler nate-chandler left a comment

Choose a reason for hiding this comment

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

Reviewed for CCC

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

We decided this is took big of a change for 6.1. I am going to do a smaller change that doesn't change approaches and fix a few of the issues people have been hitting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants