Support for isolated and async deinit

[nickolas-pohilets]

See also swiftlang/swift#60057

[ahoppen]

Thanks for the PR @nickolas-pohilets and for thinking so much about the recovery case. The test cases look really good 🌟 . I’m just wondering if we could simplify the implementation a little bit. It feels overly complicated to me at the moment.

[ahoppen]

This looks really good. I’ve got a few minor comments but all of them should be fairly minor.

[ahoppen]

This worries me a little bit because I’m not entirely sure that we really always end up with this being nil but can’t come up with a counter example from the top of my head. I would feel more comfortable if we just combine it with unexpectedBetweenAsyncSpecifierAndThrowsSpecifier.

RawUnexpectedNodesSyntax(combining: unexpectedBetweenAsyncSpecifierAndThrowsSpecifier, unexpectedAfterThrowsSpecifier, arena: arena),

[nickolas-pohilets]

At this point we have only a single case (as noted in the extension comment), where we know that unexpectedAfterThrowsSpecifier cannot appear. Combining two groups of unexpected syntax looses information, which was affecting how exchangeTokens() behaves. Not sure if it still does after loosening the checks. But I would postpone combining or any other handling logic until other cases appear.

[ahoppen]

I’m mostly thinking that the assumption that unexpectedAfterThrowsSpecifier is empty might change in the future. And especially the recovery cases are not hit as frequently in CI but more infrequently while code is being edited, so we might not actually notice that the assertion starts being violated. Thus, I’d rather be safe here.

And I don’t see how RawUnexpectedNodesSyntax(combining:) can change the layout of the node if unexpectedAfterThrowsSpecifier is nil because it just grabs the elements of unexpectedBetweenAsyncSpecifierAndThrowsSpecifier and puts them in a new RawUnexpectedNodesSyntax.

[nickolas-pohilets]

IMO, this entire conformance is hacky, and I would advise against introducing any new cases relying on it. I think even with RawUnexpectedNodesSyntax(combining:) implementation of the initialiser would be not robust enough to work in the general case. It is still not possible to pass RawDeinitEffectSpecifiersSyntax to parseEffectSpecifiers().

Cleaner solution would be create a new protocol just for the parseMisplacedEffectSpecifiers(). I've pushed an update. @ahoppen, WDYT?

[nickolas-pohilets]

Rebased after #1767

[ahoppen]

I really like the newly introduced protocol 😍 That’s a lot cleaner than everything we had before. Thanks for the idea.

I’ve got two really minor comments, otherwise this looks good to me now. Thanks for your great collaboration on this PR.

[nickolas-pohilets]

Should this MR and swiftlang/swift#60057 be merged in any specific order? I was getting failing tests in the main repo without changes in this one, so I guess this MR needs to be merged first. Or neither can be merged until proposal is approved?

[ahoppen]

Nice. Thanks for your effort! One last request: Could you squash your commits? It just makes for a nicer commit history.

[nickolas-pohilets]

@swift-ci, please test

[nickolas-pohilets]

@swift-ci please test

[nickolas-pohilets]

@ahoppen, could you please trigger CI? Looks like I don’t have rights. Or what am I doing wrong?

[ahoppen]

Yes, you need push access to trigger CI for security reasons. Triggering it now.

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[nickolas-pohilets]

CI failed because of this:

FAIL: code-generated files committed to repository do not match generated ones.

When I try to regenerate them, I get the following error:

CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntax/KeywordFile.swift:18:48: error: type 'TokenSyntax' has no member 'leftSquareToken'
let lookupTable = ArrayExprSyntax(leftSquare: .leftSquareToken(trailingTrivia: .newline)) {
                                              ~^~~~~~~~~~~~~~~

Reproduces on fresh main branch. @ahoppen, looks like main is broken.

Looks like issue was introduced in a862613, parent ccc812e works fine for me.

[nickolas-pohilets]

git clean -fdx helped. Could it be that ./build-script.py generate-source-code incorrectly caches code generator binary?

I've pushed an update, let's re-try CI

[ahoppen]

Yes, we’re currently seeing issues with CodeGeneration not updating correctly. We haven’t had the chance to 100% pinpoint the failure point. There was a discussion going on here: #1741 (comment)

[ahoppen]

@swift-ci Please test