Skip to content

[SwiftParser] Implement nonisolated(nonsending) specifier #3047

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
merged 7 commits into from
Apr 12, 2025

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Apr 7, 2025

This is part of SE-0461 proposal where nonisolated has to be
marked as nonsending in type context to indicate that the
function type it's attached to is caller isolated.

@xedin xedin requested review from ahoppen and bnbarham as code owners April 7, 2025 23:17
@xedin xedin force-pushed the nonisolated-nonsending-attr branch 2 times, most recently from 9edef08 to 2cd441e Compare April 8, 2025 22:05
xedin added 4 commits April 8, 2025 19:04
…ng` argument

This is part of SE-0461 proposal where `nonisolated` has to be
marked as `nonsending` in type context to indicate that the
function type it's attached to is caller isolated.
…suming '('

It's possible that `(nonsending)` was omitted and instead we are
at the parameter list of a function type. Checking ahead allows
for a better diagnostics and recovery.
@xedin
Copy link
Contributor Author

xedin commented Apr 9, 2025

swiftlang/swift#80668
@swift-ci please test

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.

Looks good to me, just a few comments inline.

diagnostics: [
DiagnosticSpec(
message: "expected identifier in modifier",
fixIts: ["replace 'sending' with identifier"]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this error message isn’t great. I would prefer if we just accept any identifier here in the parser and let ASTGen diagnose unknown identifiers as the detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something to consider here before we make a final decision: if we did allow any and all identifiers here, it would mean that we'd always parse nonisolated as a declaration modifier, which is a break from what C++ parser is doing and is not always correct.

Consider this situation:

func nonisolated() {
}

let v: Int = 42

nonisolated(v)
func test() -> Int { 42 }

This is currently a call + a decl but it would be interpreted as a a decl with a modifier with this change.

Copy link
Member

Choose a reason for hiding this comment

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

OK, let’s just stick with the current solution. It’s not the best but maybe good enough for now. And I don’t want to block this PR on improving this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

@xedin xedin force-pushed the nonisolated-nonsending-attr branch 2 times, most recently from edc83c8 to 1df326d Compare April 9, 2025 19:12
@xedin xedin force-pushed the nonisolated-nonsending-attr branch from 1df326d to 2b13304 Compare April 9, 2025 20:38
@xedin
Copy link
Contributor Author

xedin commented Apr 10, 2025

swiftlang/swift#80668
@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Apr 10, 2025

swiftlang/swift#80668
@swift-ci please test Windows platform

xedin added 2 commits April 11, 2025 14:27
…odifiers while skipping

Otherwise in situations like `nonisolated()` `canParseType` would
return `true` because `()` is valid type.
@xedin xedin force-pushed the nonisolated-nonsending-attr branch from ccc7696 to 2ae464e Compare April 11, 2025 21:28
@xedin
Copy link
Contributor Author

xedin commented Apr 11, 2025

swiftlang/swift#80668
@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Apr 11, 2025

swiftlang/swift#80668
@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Apr 11, 2025

swiftlang/swift#80668
@swift-ci please test Windows platform

@xedin xedin merged commit fb65fcb into swiftlang:main Apr 12, 2025
26 checks passed
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