Skip to content

Commit a227ff0

Browse files
neonichuneonichu
authored
Refactor how we are diagnosing unsafe flags (#6059)
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
1 parent cc30527 commit a227ff0

File tree

9 files changed

+118
-31
lines changed

9 files changed

+118
-31
lines changed

‎Sources/Build/BuildPlan.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -998,7 +998,8 @@ private extension PackageModel.SwiftTarget {
998998
path: .root,
999999
sources: sources,
10001000
dependencies: dependencies,
1001-
swiftVersion: .v5
1001+
swiftVersion: .v5,
1002+
usesUnsafeFlags: false
10021003
)
10031004
}
10041005
}

‎Sources/PackageGraph/PackageGraph+Loading.swift

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -797,12 +797,8 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
797797
func diagnoseInvalidUseOfUnsafeFlags(_ product: ResolvedProduct) throws {
798798
// Diagnose if any target in this product uses an unsafe flag.
799799
for target in try product.recursiveTargetDependencies() {
800-
for (decl, assignments) in target.underlyingTarget.buildSettings.assignments {
801-
let flags = assignments.flatMap(\.values)
802-
if BuildSettings.Declaration.unsafeSettings.contains(decl) && !flags.isEmpty {
803-
self.diagnosticsEmitter.emit(.productUsesUnsafeFlags(product: product.name, target: target.name))
804-
break
805-
}
800+
if target.underlyingTarget.usesUnsafeFlags {
801+
self.diagnosticsEmitter.emit(.productUsesUnsafeFlags(product: product.name, target: target.name))
806802
}
807803
}
808804
}

‎Sources/PackageLoading/PackageBuilder.swift

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,8 @@ public final class PackageBuilder {
867867
others: others,
868868
dependencies: dependencies,
869869
swiftVersion: try swiftVersion(),
870-
buildSettings: buildSettings
870+
buildSettings: buildSettings,
871+
usesUnsafeFlags: manifestTarget.usesUnsafeFlags
871872
)
872873
} else {
873874
// It's not a Swift target, so it's a Clang target (those are the only two types of source target currently supported).
@@ -899,7 +900,8 @@ public final class PackageBuilder {
899900
resources: resources,
900901
ignored: ignored,
901902
dependencies: dependencies,
902-
buildSettings: buildSettings
903+
buildSettings: buildSettings,
904+
usesUnsafeFlags: manifestTarget.usesUnsafeFlags
903905
)
904906
}
905907
}
@@ -1474,7 +1476,8 @@ extension PackageBuilder {
14741476
sources: sources,
14751477
dependencies: dependencies,
14761478
swiftVersion: try swiftVersion(),
1477-
buildSettings: buildSettings
1479+
buildSettings: buildSettings,
1480+
usesUnsafeFlags: false
14781481
)
14791482
}
14801483
}
@@ -1499,3 +1502,9 @@ fileprivate extension Sequence {
14991502
return results
15001503
}
15011504
}
1505+
1506+
fileprivate extension TargetDescription {
1507+
var usesUnsafeFlags: Bool {
1508+
return settings.filter { $0.kind.isUnsafeFlags }.isEmpty == false
1509+
}
1510+
}

‎Sources/PackageModel/BuildSettings.swift

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,6 @@ public enum BuildSettings {
3838
private init(_ name: String) {
3939
self.name = name
4040
}
41-
42-
/// The list of settings that are considered as unsafe build settings.
43-
public static let unsafeSettings: Set<Declaration> = [
44-
OTHER_CFLAGS, OTHER_CPLUSPLUSFLAGS, OTHER_SWIFT_FLAGS, OTHER_LDFLAGS,
45-
]
4641
}
4742

4843
/// An individual build setting assignment.

‎Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,16 @@ public enum TargetBuildSettingDescription {
3131
case unsafeFlags([String])
3232
case upcomingFeatures([String])
3333
case experimentalFeatures([String])
34+
35+
public var isUnsafeFlags: Bool {
36+
switch self {
37+
case .unsafeFlags(let flags):
38+
// If `.unsafeFlags` is used, but doesn't specify any flags, we treat it the same way as not specifying it.
39+
return !flags.isEmpty
40+
case .headerSearchPath, .define, .linkedLibrary, .linkedFramework, .upcomingFeatures, .experimentalFeatures:
41+
return false
42+
}
43+
}
3444
}
3545

3646
/// An individual build setting.

‎Sources/PackageModel/Target.swift

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,9 @@ public class Target: PolymorphicCodableProtocol {
228228
/// The usages of package plugins by this target.
229229
public let pluginUsages: [PluginUsage]
230230

231+
/// Whether or not this target uses any custom unsafe flags.
232+
public let usesUnsafeFlags: Bool
233+
231234
fileprivate init(
232235
name: String,
233236
potentialBundleName: String? = nil,
@@ -239,7 +242,8 @@ public class Target: PolymorphicCodableProtocol {
239242
others: [AbsolutePath] = [],
240243
dependencies: [Target.Dependency],
241244
buildSettings: BuildSettings.AssignmentTable,
242-
pluginUsages: [PluginUsage]
245+
pluginUsages: [PluginUsage],
246+
usesUnsafeFlags: Bool
243247
) {
244248
self.name = name
245249
self.potentialBundleName = potentialBundleName
@@ -253,10 +257,11 @@ public class Target: PolymorphicCodableProtocol {
253257
self.c99name = self.name.spm_mangledToC99ExtendedIdentifier()
254258
self.buildSettings = buildSettings
255259
self.pluginUsages = pluginUsages
260+
self.usesUnsafeFlags = usesUnsafeFlags
256261
}
257262

258263
private enum CodingKeys: String, CodingKey {
259-
case name, potentialBundleName, defaultLocalization, platforms, type, path, sources, resources, ignored, others, buildSettings, pluginUsages
264+
case name, potentialBundleName, defaultLocalization, platforms, type, path, sources, resources, ignored, others, buildSettings, pluginUsages, usesUnsafeFlags
260265
}
261266

262267
public func encode(to encoder: Encoder) throws {
@@ -275,6 +280,7 @@ public class Target: PolymorphicCodableProtocol {
275280
try container.encode(buildSettings, forKey: .buildSettings)
276281
// FIXME: pluginUsages property is skipped on purpose as it points to
277282
// the actual target dependency object.
283+
try container.encode(usesUnsafeFlags, forKey: .usesUnsafeFlags)
278284
}
279285

280286
required public init(from decoder: Decoder) throws {
@@ -295,6 +301,7 @@ public class Target: PolymorphicCodableProtocol {
295301
// FIXME: pluginUsages property is skipped on purpose as it points to
296302
// the actual target dependency object.
297303
self.pluginUsages = []
304+
self.usesUnsafeFlags = try container.decode(Bool.self, forKey: .usesUnsafeFlags)
298305
}
299306
}
300307

@@ -334,7 +341,8 @@ public final class SwiftTarget: Target {
334341
sources: testDiscoverySrc,
335342
dependencies: dependencies,
336343
buildSettings: .init(),
337-
pluginUsages: []
344+
pluginUsages: [],
345+
usesUnsafeFlags: false
338346
)
339347
}
340348

@@ -353,7 +361,8 @@ public final class SwiftTarget: Target {
353361
dependencies: [Target.Dependency] = [],
354362
swiftVersion: SwiftLanguageVersion,
355363
buildSettings: BuildSettings.AssignmentTable = .init(),
356-
pluginUsages: [PluginUsage] = []
364+
pluginUsages: [PluginUsage] = [],
365+
usesUnsafeFlags: Bool
357366
) {
358367
self.swiftVersion = swiftVersion
359368
super.init(
@@ -367,7 +376,8 @@ public final class SwiftTarget: Target {
367376
others: others,
368377
dependencies: dependencies,
369378
buildSettings: buildSettings,
370-
pluginUsages: pluginUsages
379+
pluginUsages: pluginUsages,
380+
usesUnsafeFlags: usesUnsafeFlags
371381
)
372382
}
373383

@@ -395,7 +405,8 @@ public final class SwiftTarget: Target {
395405
sources: sources,
396406
dependencies: dependencies,
397407
buildSettings: .init(),
398-
pluginUsages: []
408+
pluginUsages: [],
409+
usesUnsafeFlags: false
399410
)
400411
}
401412

@@ -446,7 +457,8 @@ public final class SystemLibraryTarget: Target {
446457
sources: sources,
447458
dependencies: [],
448459
buildSettings: .init(),
449-
pluginUsages: []
460+
pluginUsages: [],
461+
usesUnsafeFlags: false
450462
)
451463
}
452464

@@ -511,7 +523,8 @@ public final class ClangTarget: Target {
511523
ignored: [AbsolutePath] = [],
512524
others: [AbsolutePath] = [],
513525
dependencies: [Target.Dependency] = [],
514-
buildSettings: BuildSettings.AssignmentTable = .init()
526+
buildSettings: BuildSettings.AssignmentTable = .init(),
527+
usesUnsafeFlags: Bool
515528
) throws {
516529
guard includeDir.isDescendantOfOrEqual(to: sources.root) else {
517530
throw StringError("\(includeDir) should be contained in the source root \(sources.root)")
@@ -533,7 +546,8 @@ public final class ClangTarget: Target {
533546
others: others,
534547
dependencies: dependencies,
535548
buildSettings: buildSettings,
536-
pluginUsages: []
549+
pluginUsages: [],
550+
usesUnsafeFlags: usesUnsafeFlags
537551
)
538552
}
539553

@@ -592,7 +606,8 @@ public final class BinaryTarget: Target {
592606
sources: sources,
593607
dependencies: [],
594608
buildSettings: .init(),
595-
pluginUsages: []
609+
pluginUsages: [],
610+
usesUnsafeFlags: false
596611
)
597612
}
598613

@@ -711,7 +726,8 @@ public final class PluginTarget: Target {
711726
sources: sources,
712727
dependencies: dependencies,
713728
buildSettings: .init(),
714-
pluginUsages: []
729+
pluginUsages: [],
730+
usesUnsafeFlags: false
715731
)
716732
}
717733

‎Sources/XCBuildSupport/PIFBuilder.swift

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,10 +1421,6 @@ extension Target {
14211421
var isCxx: Bool {
14221422
(self as? ClangTarget)?.isCXX ?? false
14231423
}
1424-
1425-
var usesUnsafeFlags: Bool {
1426-
Set(buildSettings.assignments.keys).contains(where: BuildSettings.Declaration.unsafeSettings.contains)
1427-
}
14281424
}
14291425

14301426
extension ProductType {

‎Tests/PackageGraphTests/PackageGraphTests.swift

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2282,6 +2282,69 @@ class PackageGraphTests: XCTestCase {
22822282
}
22832283
}
22842284
}
2285+
2286+
func testDependencyOnUpcomingFeatures() throws {
2287+
let fs = InMemoryFileSystem(emptyFiles:
2288+
"/Foo/Sources/Foo/foo.swift",
2289+
"/Foo/Sources/Foo2/foo.swift",
2290+
"/Bar/Sources/Bar/bar.swift",
2291+
"/Bar/Sources/Bar2/bar.swift",
2292+
"/Bar/Sources/Bar3/bar.swift",
2293+
"/Bar/Sources/TransitiveBar/bar.swift",
2294+
"<end>"
2295+
)
2296+
2297+
let observability = ObservabilitySystem.makeForTesting()
2298+
_ = try loadPackageGraph(
2299+
fileSystem: fs,
2300+
manifests: [
2301+
Manifest.createRootManifest(
2302+
name: "Foo",
2303+
path: .init(path: "/Foo"),
2304+
dependencies: [
2305+
.localSourceControl(path: .init(path: "/Bar"), requirement: .upToNextMajor(from: "1.0.0")),
2306+
],
2307+
targets: [
2308+
TargetDescription(name: "Foo", dependencies: ["Bar"]),
2309+
TargetDescription(name: "Foo2", dependencies: ["TransitiveBar"]),
2310+
]),
2311+
Manifest.createFileSystemManifest(
2312+
name: "Bar",
2313+
path: .init(path: "/Bar"),
2314+
products: [
2315+
ProductDescription(name: "Bar", type: .library(.automatic), targets: ["Bar", "Bar2", "Bar3"]),
2316+
ProductDescription(name: "TransitiveBar", type: .library(.automatic), targets: ["TransitiveBar"]),
2317+
],
2318+
targets: [
2319+
TargetDescription(
2320+
name: "Bar",
2321+
settings: [
2322+
.init(tool: .swift, kind: .upcomingFeatures(["ConciseMagicFile"])),
2323+
]
2324+
),
2325+
TargetDescription(
2326+
name: "Bar2",
2327+
settings: [
2328+
.init(tool: .swift, kind: .upcomingFeatures(["UnknownToTheseTools"])),
2329+
]
2330+
),
2331+
TargetDescription(
2332+
name: "Bar3",
2333+
settings: [
2334+
.init(tool: .swift, kind: .upcomingFeatures(["ExistentialAny", "UnknownToTheseTools"])),
2335+
]
2336+
),
2337+
TargetDescription(
2338+
name: "TransitiveBar",
2339+
dependencies: ["Bar2"]
2340+
),
2341+
]),
2342+
],
2343+
observabilityScope: observability.topScope
2344+
)
2345+
2346+
XCTAssertEqual(observability.diagnostics.count, 0, "unexpected diagnostics: \(observability.diagnostics.map { $0.description })")
2347+
}
22852348
}
22862349

22872350

‎Tests/PackageGraphTests/TargetTests.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ private extension ResolvedTarget {
2525
path: .root,
2626
sources: Sources(paths: [], root: AbsolutePath(path: "/")),
2727
dependencies: [],
28-
swiftVersion: .v4
28+
swiftVersion: .v4,
29+
usesUnsafeFlags: false
2930
),
3031
dependencies: deps.map { .target($0, conditions: []) },
3132
defaultLocalization: nil,

0 commit comments

Comments
 (0)