Skip to content

Incorrect location of 'async' for protocol in actor's fix suggestion #69246

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 6 commits into from
Nov 6, 2023

Conversation

dancamarotto
Copy link
Contributor

Resolves #69244

@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please smoke test

Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

It looks like this code path was not really covered by tests since nothing broke. So wondering if we could add a few more tests for covering "throws" as well.
But PR looks good, someone from Apple team have the final say before merging :)

@LucianoPAlmeida LucianoPAlmeida requested a review from xedin November 3, 2023 12:51
@LucianoPAlmeida
Copy link
Contributor

LucianoPAlmeida commented Nov 3, 2023

What I meant by throws tests were something like this

import Distributed

protocol P {
    func foo() -> Void // here fix-it is also wrong result in func foo() -> async throws Void
}

distributed actor A: P {
    func foo() { }
}

In this example the fix-it covers throws modifier as well which at the moment is also wrong func foo() -> async throws Void. Note that this test since it requires distributed it has to be on a file that has // REQUIRES: distributed you can search across for examples.

It would be nice, but is not required for this PR I guess, but I'll left that up to someone from the team.

@LucianoPAlmeida LucianoPAlmeida requested a review from ktoso November 3, 2023 23:21
@dancamarotto
Copy link
Contributor Author

dancamarotto commented Nov 4, 2023

@LucianoPAlmeida ah ok, thanks, I hadn't thought of distributed actors - done. please let me know if it does not look correct.

I tried to add the test so that the only error would be what I wanted to test, hence the typealias ActorSystem and the distributed func.

@LucianoPAlmeida
Copy link
Contributor

@LucianoPAlmeida ah ok, thanks, I hadn't thought of distributed actors - done. please let me know if it does not look correct.

Distributed is the only case where we exercise this async throws fix-it, and test does look right. But I don't recommend adding distributed test to this file. This test should probably be in a file that has // REQUIRES: distributed otherwise I think it would fail on CI.

Maybe add that test case to a file under test/Distributed? The test/Distributed/actor_protocols.swift file seems like a good candidate, but I am no expert in this so my suggestions should be taken with a grain of salt. But let's add it and hear from the final reviewers :)

@ktoso
Copy link
Contributor

ktoso commented Nov 6, 2023

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Nov 6, 2023

This looks good AFAICS, thank you

@ktoso ktoso added concurrency Feature: umbrella label for concurrency language features distributed Feature → concurrency: distributed actor labels Nov 6, 2023
@ktoso
Copy link
Contributor

ktoso commented Nov 6, 2023

@swift-ci please smoke test

@ktoso ktoso self-assigned this Nov 6, 2023
@LucianoPAlmeida LucianoPAlmeida requested a review from ktoso November 6, 2023 12:38
@LucianoPAlmeida
Copy link
Contributor

@swift-ci Please smoke test

@LucianoPAlmeida LucianoPAlmeida merged commit 78adeb6 into swiftlang:main Nov 6, 2023
@dancamarotto dancamarotto deleted the 69244-protocol-async branch November 6, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect location of 'async' for protocol in actor's fix suggestion
3 participants