False-positive diagnostics with sending parameters and use of RBI

khlopko commented 2 months ago

Description

I've run into a several cases with compiler that in some of them to my understanding shouldn't produce an error while it does, and one of them should, while it doesn't.

Reproduction

Given that initially:

// snippet 1
protocol Command<In, Out> {
    associatedtype In
    associatedtype Out

    func execute(_ input: In) async throws -> Out
}

protocol CommandsFactory {
    associatedtype LoggerCommandType: Command<String, Void>
    func logger() -> sending LoggerCommandType
}

@MainActor
final class UseCase<Factory> where Factory: CommandsFactory {
    private let factory: Factory

    init(factory: Factory) {
        self.factory = factory
    }
}

If I add doSmth function to the UseCase in the following manner, there is an error, while I think it should be safe:

// snippet 2
extension UseCase {
    func doSmth() async throws {
        let command = factory.logger() // should be in a disconnected region here 
        try await command.execute("did it!") // error: sending 'command' risks causing data races
    }
}

It is gone if CommandsFactory gets changed to be a concrete type instead of a protocol & generic constraint, so seems like compiler doesn't reason well for protocol case here.

When I wrap this is in a Task in a bit odd manner, the error is gone however:

// snippet 3
extension UseCase {
    func doSmthWithTask() async throws {
        let factory = factory
        try await Task {
            let command = factory.logger()
            try await command.execute("did it!") // ok
        }.value
    }
}

But it is gone too good, since it is missing when I think it should be:

// snippet 4
@MainActor
final class Wrapper<CommandType> where CommandType: Command<String, Void> {
    private let command: CommandType

    init(command: CommandType) {
        self.command = command
    }

    func execute() async throws {
        let command = command
        try await Task {
            try await command.execute("I'm wrapped!") // no complaints here
        }.value
    }
}

command isn't guaranteed to be a struct, so it might not get copied, causing potential data race.

Expected behavior

In snippet 2 I'd expect it to compile without the error, as it does currently when using a concrete type.

In snippet 4 I think there should be an error calling command.execute in that way, as command isn't in a disconnected region (at least, not that compiler can be certain about I guess).

Environment

swift-driver version: 1.115 Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2) Target: arm64-apple-macosx15.0

Additional information

No response

khlopko commented 2 months ago

Reproduced with Xcode 16 RC

cc @hborla

gottesmm commented 1 week ago

Just looked at this with ToT, we get no errors. The change that caused this to not emit errors was in between swift-DEVELOPMENT-SNAPSHOT-2024-08-21-a and swift-DEVELOPMENT-SNAPSHOT-2024-08-26-a.

gottesmm commented 1 week ago

So the fourth example should be safe since the isolation is:

@MainActor
final class Wrapper<CommandType> where CommandType: Command<String, Void> {
    private let command: CommandType

    init(command: CommandType) {
        self.command = command
    }

    func execute() async throws {
        let command = command // command is already in the main actor region
        try await Task {
            // Task's closure is inferred to be main actor isolated, so we are putting main actor region state into a main actor
            // isolated closure which is safe.
            try await command.execute("I'm wrapped!") // no complaints here
        }.value
    }
}

gottesmm commented 1 week ago

I looked at the fourth snippet again. I see what you are saying. The problem is that command is due to a known bug being considered to be sending instead of main actor isolated. I am not exactly sure where the bug is, but I am going to leave this open and will close it when I come around to fixing it.

khlopko commented 1 week ago

@gottesmm thank you for taking a look! Just checked and I also don't have error for the second snipped with the latest toolchain, that is pretty great news (at least for me for sure!) as it was a bit painful to deal with.

khlopko commented 1 week ago

@gottesmm maybe it may help to diagnose the issue (seems like related): I've modified fourth snipped to have Task.detached after reading your comments that command might be inferred to be isolated, and with the latest toolchain snapshot (from 30th of October) got the crash on assertion:

Assertion failed: (!type->hasTypeParameter() && "must take a contextual type. if you really are ok with an " "indefinite answer (and usually YOU ARE NOT), then consider whether " "you really, definitely are ok with an indefinite answer, and " "use `checkConformanceWithoutContext` instead"), function checkConformance, file ConformanceLookup.cpp, line 757.

With stacktrace:

1.  Apple Swift version 6.1-dev (LLVM 9d655fdc6103926, Swift 0d16cbf5d5ef6f9)
2.  Compiling with the current language version
3.  While evaluating request ExecuteSILPipelineRequest(Run pipelines { Mandatory Diagnostic Passes + Enabling Optimization Passes } on SIL for swift_sending_check)
4.  While running pass #193 SILFunctionTransform "TransferNonSendable" on SILFunction "@$s19swift_sending_check7WrapperC7executeyyYaKF".
 for 'execute()' (at /Users/kyrylokhlopko/Projects/swift-sending-check/Sources/swift-sending-check/swift_sending_check.swift:52:5)
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  swift-frontend           0x0000000106808e0c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  swift-frontend           0x00000001068074f0 llvm::sys::RunSignalHandlers() + 112
2  swift-frontend           0x0000000106809468 SignalHandler(int) + 304
3  libsystem_platform.dylib 0x000000018b24c184 _sigtramp + 56
4  libsystem_pthread.dylib  0x000000018b216f70 pthread_kill + 288
5  libsystem_c.dylib        0x000000018b123908 abort + 128
6  libsystem_c.dylib        0x000000018b122c1c err + 0
7  swift-frontend           0x0000000106db258c swift::TypeBase::isSendableType() (.cold.1) + 0
8  swift-frontend           0x000000010240f194 swift::checkConformance(swift::Type, swift::ProtocolDecl*, bool) + 176
9  swift-frontend           0x000000010240f3c4 swift::TypeBase::isSendableType() + 372
10 swift-frontend           0x00000001019fc958 swift::SILIsolationInfo::isNonSendableType(swift::SILType, swift::SILFunction*) + 512
11 swift-frontend           0x00000001017626cc (anonymous namespace)::TransferNonSendableImpl::emitVerbatimErrors() + 8856
12 swift-frontend           0x000000010175d838 (anonymous namespace)::TransferNonSendable::run() + 6756
13 swift-frontend           0x0000000101783cb0 swift::SILPassManager::runPassOnFunction(unsigned int, swift::SILFunction*) + 1440
14 swift-frontend           0x0000000101784b1c swift::SILPassManager::runFunctionPasses(unsigned int, unsigned int) + 1060
15 swift-frontend           0x0000000101787300 swift::SILPassManager::execute() + 596
16 swift-frontend           0x0000000101781bd4 swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 72
17 swift-frontend           0x0000000101781b54 swift::ExecuteSILPipelineRequest::evaluate(swift::Evaluator&, swift::SILPipelineExecutionDescriptor) const + 68
18 swift-frontend           0x00000001017d2040 swift::SimpleRequest<swift::ExecuteSILPipelineRequest, std::__1::tuple<> (swift::SILPipelineExecutionDescriptor), (swift::RequestFlags)1>::evaluateRequest(swift::ExecuteSILPipelineRequest const&, swift::Evaluator&) + 28
19 swift-frontend           0x000000010179e724 swift::ExecuteSILPipelineRequest::OutputType swift::Evaluator::getResultUncached<swift::ExecuteSILPipelineRequest, swift::ExecuteSILPipelineRequest::OutputType swift::evaluateOrFatal<swift::ExecuteSILPipelineRequest>(swift::Evaluator&, swift::ExecuteSILPipelineRequest)::'lambda'()>(swift::ExecuteSILPipelineRequest const&, swift::ExecuteSILPipelineRequest::OutputType swift::evaluateOrFatal<swift::ExecuteSILPipelineRequest>(swift::Evaluator&, swift::ExecuteSILPipelineRequest)::'lambda'()) + 204
20 swift-frontend           0x0000000101781db0 swift::executePassPipelinePlan(swift::SILModule*, swift::SILPassPipelinePlan const&, bool, swift::irgen::IRGenModule*) + 64
21 swift-frontend           0x00000001017b40c0 swift::runSILDiagnosticPasses(swift::SILModule&) + 192
22 swift-frontend           0x0000000100f6ce5c swift::CompilerInstance::performSILProcessing(swift::SILModule*) + 80
23 swift-frontend           0x0000000100d1cc34 performCompileStepsPostSILGen(swift::CompilerInstance&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule>>, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*) + 820
24 swift-frontend           0x0000000100d1c2b4 swift::performCompileStepsPostSema(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 656
25 swift-frontend           0x0000000100d28788 withSemanticAnalysis(swift::CompilerInstance&, swift::FrontendObserver*, llvm::function_ref<bool (swift::CompilerInstance&)>, bool) + 160
26 swift-frontend           0x0000000100d1e180 performCompile(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 716
27 swift-frontend           0x0000000100d1d84c swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 2328
28 swift-frontend           0x0000000100af925c swift::mainEntry(int, char const**) + 3100
29 dyld                     0x000000018ae94274 start + 2840

swiftlang / swift