Skip to content

[rbi] Small tweaks to the closure patch #78837 #79028

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

Jump to bottom
Merged
merged 2 commits into from
Jan 30, 2025
Merged

[rbi] Small tweaks to the closure patch #78837 #79028

merged 2 commits into from
Jan 30, 2025

Conversation

gottesmm
Copy link
Contributor

This is a tweak of #78837. Specifically:

  1. I added some code that made sure that we handle ignored_use when diagnosing unreachable code caused by noreturn begin_apply/try_apply.
  2. I tweaked findClosureUse so that when we diagnose a capture that is in an immediately used closure, we distinguish in between closure uses and capture uses. That is given the following two cases:
Task {
  {
    print($0)
  }(x)
}

Task {
   {
      x
   }()
}

we would in the first case diagnose $0 as being captured like x is in the second one. The one unfortunate bit is that since function parameters do not have locations in SIL today we put the diagnostic on the inner closure in the first case instead of x... but we at least emit correctly that it was x that was captured and one can easily find the parameter on the closure.

@gottesmm
[sil] Make sure that we do not flag ignored_use as dead code when use…
39e63b4 
…d after a try_apply or begin_apply.
@gottesmm
[rbi] When finding closure uses in an immediately invoked closure, di…
8c96a8d 
…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.
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

nate-chandler
nate-chandler approved these changes Jan 29, 2025
View reviewed changes
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.

lgtm

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test macOS platform

@gottesmm gottesmm enabled auto-merge January 30, 2025 02:43
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test macOS platform

1 similar comment
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test macOS platform

@gottesmm
Copy link
Contributor Author

@swift-ci test macOS platform

@gottesmm gottesmm mentioned this pull request Jan 30, 2025
Closed
@gottesmm gottesmm merged commit 8c19cd7 into swiftlang:main Jan 30, 2025
3 of 4 checks passed
@gottesmm gottesmm deleted the more-closure-fixes branch January 30, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nate-chandler nate-chandler nate-chandler approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

@ktoso ktoso Awaiting requested review from ktoso ktoso is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gottesmm @nate-chandler