swiftlang / swift-package-manager

The Package Manager for the Swift Programming Language
Apache License 2.0
9.75k stars 1.35k forks source link

CoreCommands: fix unused `getRootPackageInformation()` result warning #7956

Closed valeriyvan closed 2 months ago

dschaefer2 commented 2 months ago

@swift-ci please test

MaxDesiatov commented 2 months ago

Do we have any certainty that this call has no side effects, like calling out to git to check out the package, compiling and loading manifests, or anything else similar to that?

dschaefer2 commented 2 months ago

Do we have any certainty that this call has no side effects, like calling out to git to check out the package, compiling and loading manifests, or anything else similar to that?

@valeriyvan can you check this. It's weird this line of code would still be there. We just need to be careful.

valeriyvan commented 2 months ago

Do we have any certainty that this call has no side effects, like calling out to git to check out the package, compiling and loading manifests, or anything else similar to that?

@valeriyvan can you check this. It's weird this line of code would still be there. We just need to be careful.

I don't see any side effects in getRootPackageInformation() itself:

    public func getRootPackageInformation() async throws -> (dependencies: [PackageIdentity: [PackageIdentity]], targets: [PackageIdentity: [String]]) {
        let workspace = try self.getActiveWorkspace()
        let root = try self.getWorkspaceRoot()
        let rootManifests = try await workspace.loadRootManifests(
            packages: root.packages,
            observabilityScope: self.observabilityScope
        )

        var identities = [PackageIdentity: [PackageIdentity]]()
        var targets = [PackageIdentity: [String]]()

        rootManifests.forEach {
            let identity = PackageIdentity(path: $0.key)
            identities[identity] = $0.value.dependencies.map(\.identity)
            targets[identity] = $0.value.targets.map { $0.name.spm_mangledToC99ExtendedIdentifier() }
        }

        return (identities, targets)
    }
MaxDesiatov commented 2 months ago

I don't think that's true. If that function were pure, it wouldn't be async in the first place. await workspace.loadRootManifests(...) in getRootPackageInformation() body is clearly effectful: it's asynchronous and causes manifests loading (potentially compilation of root package's Package.swift before loading too). If the goal is to address the warning, we should ignore the result, but removing this call changes the behavior.

plemarquand commented 2 months ago

I believe ultimately workspace.loadRootManifests calls ManifestLoader.loadAndCacheManifest. Is it possible this call is meant to do the manifest loading work up front?

MaxDesiatov commented 2 months ago

Yes, that's my understanding of it, although I'm not the original author of this code.

valeriyvan commented 2 months ago

If that is true getRootPackageInformation should be renamed.

valeriyvan commented 2 months ago

Changed PR to just discard unused return value.

dschaefer2 commented 2 months ago

If that is true getRootPackageInformation should be renamed.

Can't argue with that. :).

dschaefer2 commented 2 months ago

@swift-ci please test

dschaefer2 commented 2 months ago

@swift-ci please test windows