SE-0362 "upcoming features" are treated as "unsafe flags"

[OneSadCookie]

Description

When using SE-0362's upcomingFeatures, a package becomes ineligible for use as a dependency.

This isn't explicitly called out in the evolution proposal, but I believe it runs contrary to the intent. After all, what use is the ability to enable (eg) bare slash regular expressions for my own convenience, if nobody can use my library because of it?

Expected behavior

upcomingFeatures should not be treated as "unsafe", and other packages should be able to depend on my package if I use these (experimental features is much more of a gray area, and might require deeper consideration!)

Steps to reproduce

Here's a failing test you can add to PackageGraphTests.swift:

    func testDependencyOnUpcomingFeatures() throws {
        let fs = InMemoryFileSystem(emptyFiles:
            "/Foo/Sources/Foo/foo.swift",
            "/Foo/Sources/Foo2/foo.swift",
            "/Bar/Sources/Bar/bar.swift",
            "/Bar/Sources/Bar2/bar.swift",
            "/Bar/Sources/Bar3/bar.swift",
            "/Bar/Sources/TransitiveBar/bar.swift",
            "<end>"
        )

        let observability = ObservabilitySystem.makeForTesting()
        _ = try loadPackageGraph(
            fileSystem: fs,
            manifests: [
                Manifest.createRootManifest(
                    name: "Foo",
                    path: .init(path: "/Foo"),
                    dependencies: [
                        .localSourceControl(path: .init(path: "/Bar"), requirement: .upToNextMajor(from: "1.0.0")),
                    ],
                    targets: [
                        TargetDescription(name: "Foo", dependencies: ["Bar"]),
                        TargetDescription(name: "Foo2", dependencies: ["TransitiveBar"]),
                    ]),
                Manifest.createFileSystemManifest(
                    name: "Bar",
                    path: .init(path: "/Bar"),
                    products: [
                        ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar", "Bar2", "Bar3"]),
                        ProductDescription(name: "TransitiveBar", type: .library(.automatic), targets: ["TransitiveBar"]),
                    ],
                    targets: [
                        TargetDescription(
                            name: "Bar",
                            settings: [
                                .init(tool: .swift, kind: .upcomingFeatures(["ConciseMagicFile"])),
                            ]
                        ),
                        TargetDescription(
                            name: "Bar2",
                            settings: [
                                .init(tool: .swift, kind: .upcomingFeatures(["UnknownToTheseTools"])),
                            ]
                        ),
                        TargetDescription(
                            name: "Bar3",
                            settings: [
                                .init(tool: .swift, kind: .upcomingFeatures(["ExistentialAny", "UnknownToTheseTools"])),
                            ]
                        ),
                        TargetDescription(
                            name: "TransitiveBar",
                            dependencies: ["Bar2"]
                        ),
                    ]),
            ],
            observabilityScope: observability.topScope
        )

        XCTAssertEqual(observability.diagnostics.count, 0)
    }

Swift Package Manager version/commit hash

dd7e9cc

Swift & OS version (output of swift --version && uname -a)

Swift version 5.7 (swift-5.7-RELEASE)
Target: x86_64-unknown-linux-gnu
Linux GGPC 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

[OneSadCookie]

It's not at all clear to me how to fix this. The flags are added to OTHER_SWIFT_FLAGS, which seems appropriate on the surface, but then any flag in OTHER_SWIFT_FLAGS is treated as unsafe. So the obvious way to avoid this is to introduce another BuildSettings.Declaration that is safe, but these appear to be chosen for their specific meaning within Xcode?

[OneSadCookie]

@DougGregor what do you think?

[neonichu]

Yah, I don't think that is intentional, as you say, the entire point would be to not have to resort to unsafe flags for these.

[DougGregor]

The intent is that "upcoming" and "experimental" features are not treated as unsafe flags. I hadn't realized the link to OTHER_SWIFT_FLAGS when I did the initial implementation, and we should fix this.

[neonichu]

BTW, only SwiftPM's XCBuild support should be using OTHER_SWIFT_FLAGS, is that issue only occurring specifically with that or is it more general?

[KeithBauerANZ]

It's completely general; my original report is from Linux. The OTHER_SWIFT_FLAGS is a general concept of how SwiftPM gathers compiler flags, not specific to Xcode.

[neonichu]

Oh yah, you're right.

I think the way diagnoseInvalidUseOfUnsafeFlags works seems a bit backwards since we know whether or not the actual unsafeFlags API has been used without having to later infer that knowledge. The target itself should carry a bit on whether that API has been used instead.

[KeithBauerANZ]

From my very limited poking around in the source, that seems a more reasonable approach that will also prevent issues like this one from recurring as more safe SwiftSettings get added