Skip to content

Split DeclEffectSpecifiers into AccessorEffectSpecifiers and FunctionEffectSpecifiers #1454

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

Merged

Conversation

StevenWong12
Copy link
Contributor

Part of #1373

@StevenWong12 StevenWong12 changed the title Add diagnostics for rethrows in accessor declaration [WIP] Add diagnostics for rethrows in accessor declaration Mar 27, 2023
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Conceptually this boils down to a similar problem that I think we discussed in one of your other PRs: As long as we can’t represent that the effect specifier of an accessor cannot be reasync or rethrows, then no client (and “client” here includes later stages of the compiler) can assume that asyncSpecifier of an AccessorDecl cannot be reasync and thus needs to diagnose it themselves.

What needs to be done here is that we need to split DeclEffectSpecifiers into two different nodes: AccessorEffectSpecifiers and FunctionEffectSpecifiers where FunctionEffectSpecifiers does not allow rethrows and reasync. For this you need to modify the declaration of DeclEffectSpecifiers in CodeGeneration and run generate-swiftsyntax to regenerate the code. Do you think you could try that?

@StevenWong12
Copy link
Contributor Author

Thanks for your comments! I think I only considered SwiftParserDiagnostics as the client of the parser, which makes the changes in my PRs not friendly to other clients. I think your suggestions make sense and I will make a try to modify my PRs.

@StevenWong12
Copy link
Contributor Author

AccessorEffectSpecifiers and FunctionEffectSpecifiers where FunctionEffectSpecifiers does not allow rethrows and reasync.

Just want to make sure if there is a typo and it should be

where AccessorEffectSpecifiers does not allow rethrows and reasync

@ahoppen
Copy link
Member

ahoppen commented Mar 31, 2023

Yes, that was a typo indeed. Good catch

@StevenWong12 StevenWong12 force-pushed the effectful_properties_diagnostics branch from ea592ba to 057fd9c Compare April 10, 2023 08:45
@StevenWong12 StevenWong12 changed the title [WIP] Add diagnostics for rethrows in accessor declaration Split DeclEffectSpecifiers into AccessorEffectSpecifiers and FunctionEffectSpecifiers Apr 10, 2023
@StevenWong12 StevenWong12 marked this pull request as ready for review April 10, 2023 08:47
@StevenWong12
Copy link
Contributor Author

StevenWong12 commented Apr 10, 2023

I have took AccessorEffectSpecifiers apart from DeclEffectSpecifiers and renamed DeclEffectSpecifiers to FunctionEffectSpecifiers. It seems that this split also solves the TODOs in EffectfulPropertiesTests.swift without many add-ons.😀

@StevenWong12 StevenWong12 requested a review from ahoppen April 10, 2023 08:55
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good. I just think you have a lingering TODO comment in your codebase.

…ctionEffectSpecifiers`.

Solve TODOs in `EffectfulPropertiesTests.swift`
@StevenWong12 StevenWong12 force-pushed the effectful_properties_diagnostics branch from 057fd9c to 47617a8 Compare April 12, 2023 12:55
@StevenWong12
Copy link
Contributor Author

Oh, I forgot to delete that. I've removed it and squashed my changes into a single commit.

@kimdv
Copy link
Contributor

kimdv commented Apr 12, 2023

@swift-ci please test

@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge April 12, 2023 19:55
@ahoppen ahoppen merged commit 39dcb5b into swiftlang:main Apr 12, 2023
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
…ies_diagnostics

Split DeclEffectSpecifiers into AccessorEffectSpecifiers and FunctionEffectSpecifiers
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
…ies_diagnostics

Split DeclEffectSpecifiers into AccessorEffectSpecifiers and FunctionEffectSpecifiers
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.

3 participants