Skip to content
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

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

Closed
OneSadCookie opened this issue Dec 10, 2022 · 8 comments · Fixed by #6059
Closed

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

OneSadCookie opened this issue Dec 10, 2022 · 8 comments · Fixed by #6059
Assignees
Labels

Comments

@OneSadCookie
Copy link

OneSadCookie commented Dec 10, 2022

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
Copy link
Author

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
Copy link
Author

@DougGregor what do you think?

@neonichu
Copy link
Member

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.

@neonichu neonichu self-assigned this Dec 14, 2022
@DougGregor
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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

neonichu added a commit that referenced this issue Jan 19, 2023
Currently, we are diagnosing unsafe flags in a bit of a backwards way, trying to infer them from build settings that were derived from them. This adds an explicit `usesUnsafeFlags` model property that is driven by whether the `unsafeFlags` API was used in a given target.

fixes #5965
neonichu added a commit that referenced this issue Jan 19, 2023
Currently, we are diagnosing unsafe flags in a bit of a backwards way, trying to infer them from build settings that were derived from them. This adds an explicit `usesUnsafeFlags` model property that is driven by whether the `unsafeFlags` API was used in a given target.

fixes #5965
neonichu added a commit that referenced this issue Jan 19, 2023
Currently, we are diagnosing unsafe flags in a bit of a backwards way, trying to infer them from build settings that were derived from them. This adds an explicit `usesUnsafeFlags` model property that is driven by whether the `unsafeFlags` API was used in a given target.

fixes #5965
neonichu added a commit that referenced this issue Jan 19, 2023
Currently, we are diagnosing unsafe flags in a bit of a backwards way, trying to infer them from build settings that were derived from them. This adds an explicit `usesUnsafeFlags` model property that is driven by whether the `unsafeFlags` API was used in a given target.

fixes #5965

(cherry picked from commit a227ff0)
neonichu added a commit that referenced this issue Jan 23, 2023
Currently, we are diagnosing unsafe flags in a bit of a backwards way, trying to infer them from build settings that were derived from them. This adds an explicit `usesUnsafeFlags` model property that is driven by whether the `unsafeFlags` API was used in a given target.

fixes #5965

(cherry picked from commit a227ff0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants