Fix a minor threading issue and TODO by switching to structured concurrency

[benpious]

In #2233, it was suggested that a better long term direction was to adopt Structured Concurrency. This PR does exactly that.

The issue this pr is fixing is that the errors var had no synchronization and could cause crashes if it was written to.

[bnbarham]

@swift-ci please test

[bnbarham]

Thanks @benpious! Out of interest, did you see any increase in generation time with this change?

[benpious]

Out of interest, did you see any increase in generation time with this change?

I didn't notice any when I originally wrote it, and in my tests there doesn't seem to be any difference. I ran time swift run --package-path CodeGeneration generate-swift-syntax Sources and subtracted the reported build time from the total; it seems to be consistently between 118 and 135 seconds on my computer regardless of whether I'm on main or this branch.

[ahoppen]

Very nice. 🤩 I only have one minor comment to make the code slightly more readable, otherwise looks very good to me.

[ahoppen]

I’m happy with making this a top-level function but just FYI, you could keep this a member of GenerateSwiftSyntax if you mark the function as nonisolated.

[bnbarham]

It doesn't actually use any members, so IMO a top level fileprivate function makes sense anyway.

[benpious]

It turns out you don't have to even mark it as nonisolated in the current setup of run, it still works.

I originally made this change because when I tried to mark run as async without changing the conformance to the async version of ParseableCommand, the compiler error kind of implied this was the problem. That was not the case.

So I don't have a strong opinion on this, but I'm partial to the top level free function.

[bnbarham]

it seems to be consistently between 118 and 135 seconds on my computer regardless of whether I'm on main or this branch

Thanks for checking!

[bnbarham]

Thanks for the updates! LGTM, could you squash your commits and add a commit message, see https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#authoring-commits? I'll start testing after that so we don't have to run it twice.

[ahoppen]

Thank you for the PR. Looks very good to me and great to see Swift Concurrency making its way into swift-syntax 👍🏽

[benpious]

Thanks for the updates! LGTM, could you squash your commits and add a commit message, see https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#authoring-commits? I'll start testing after that so we don't have to run it twice.

Done!

[bnbarham]

@swift-ci please test

[bnbarham]

Done!

FWIW it's generally good practice to have a max 72 length title (to avoid truncation with the rest added to description). No need to fix for this commit, I've started testing. Thanks again!

[ahoppen]

@swift-ci Please test macOS