Skip to content
New issue

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

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

Already on GitHub? # to your account

fix almost all Sendable warnings #2994

Merged
merged 2 commits into from
Nov 25, 2024
Merged

fix almost all Sendable warnings #2994

merged 2 commits into from
Nov 25, 2024

Conversation

weissi
Copy link
Member

@weissi weissi commented Nov 23, 2024

Motivation:

Opening the swift-nio repository made me warning blind because there were always so many trivially fixable warnings about things that were correct but cannot be understood by the compiler.

Modifications:

Fix all the sendable warnings that popped up, except for one test where NIOLockedValueBox<Thread?> isn't sendable because Foundation.Thread seemingly isn't Sendable which is odd. Guessing that'll be fixed on their end.

Result:

  • Fewer warnings
  • Less warning-blindness
  • More checks

@weissi weissi requested review from glbrntt, FranzBusch and Lukasa and removed request for FranzBusch November 23, 2024 16:16
@weissi weissi added the 🆕 semver/minor Adds new public API. label Nov 23, 2024
Copy link
Member

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

And that without any unsafe/unchecked swift! Nicely done!

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2024

The intention here has been to wait until we could use the isolated views on ELP and ELF to do this work, to minimise the amount of runtime isolation checking. As the principal advocate for avoiding preconditionOnEventLoop, would you prefer to silence these warnings at that cost?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2024

That comment applies to the changes in the production code, naturally: the changes in the tests are going to be just fine either way.

@weissi
Copy link
Member Author

weissi commented Nov 25, 2024

The intention here has been to wait until we could use the isolated views on ELP and ELF to do this work, to minimise the amount of runtime isolation checking. As the principal advocate for avoiding preconditionOnEventLoop, would you prefer to silence these warnings at that cost?

@Lukasa I really would want to see these warnings go away because there are just too many of them, I lose track of others (point in case this one: #2995 (comment)).

I hear you w.r.t. to the preconditionInEventLoop but here's why I think we should still do it: At first, I only wanted to do tests & the upgraders (because they're not super perf critical). But then I noticed that's like 99% of all of them. None of the code I changed is in actually perf-critical code or changes any of the properties of the code.

And once we have the isolated views (which are fab btw), we can still replace all the code, it's super easy to spot by grepping for NIOLoopBound, right?

tl;dr: This improves things and I mandate that it doesn't regress anything. So I don't think there's any point in waiting.

weissi added a commit that referenced this pull request Nov 25, 2024
…2995)

### Motivation:

Warnings are annoying. Companion of #2994 

### Modifications:

Fix all the `syncShutdownGracefully` not being available in `async`
contexts warnings.

### Result:

Everybody happier.
@Lukasa Lukasa enabled auto-merge (squash) November 25, 2024 14:47
@Lukasa Lukasa merged commit 6ba8f4f into apple:main Nov 25, 2024
42 of 43 checks passed
@weissi weissi deleted the jw-sendwarn branch November 25, 2024 17:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants