From 6aa51bd26711ac2e7d2d82588c3dd83114e78fc6 Mon Sep 17 00:00:00 2001 From: Ian Leitch Date: Tue, 2 Jan 2024 12:02:34 +0000 Subject: [PATCH] Detect assign-only properties on structs with synthesized initializers --- CHANGELOG.md | 2 + .../Formatters/OutputDeclarationFilter.swift | 2 +- ...ssignOnlyPropertyReferenceEliminator.swift | 66 ++++--------------- .../CodingKeyEnumReferenceBuilder.swift | 11 +--- .../DefaultConstructorReferenceBuilder.swift | 4 +- ...tImplicitInitializerReferenceBuilder.swift | 65 ++++++++++++++++++ .../SourceGraph/SourceGraph.swift | 26 +++++--- .../SourceGraphMutatorRunner.swift | 3 +- .../RetentionFixtures/testCodingKeyEnum.swift | 20 ------ .../testRetainsEncodableProperties.swift | 40 ----------- .../testStructImplicitInitializer.swift | 18 +++++ Tests/PeripheryTests/RetentionTest.swift | 47 ++++++++----- 12 files changed, 152 insertions(+), 152 deletions(-) create mode 100644 Sources/PeripheryKit/SourceGraph/Mutators/StructImplicitInitializerReferenceBuilder.swift delete mode 100644 Tests/Fixtures/RetentionFixtures/testRetainsEncodableProperties.swift create mode 100644 Tests/Fixtures/RetentionFixtures/testStructImplicitInitializer.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index e81e6ba47..4c7961463 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Add experimental unused import analysis. - The option `--external-encodable-protocols` is deprecated, use `--external-codable-protocols` instead. +- Assign-only properties on structs with synthesized initializers are now detected. ##### Bug Fixes @@ -23,6 +24,7 @@ - Fix public accessibility analysis false-positive for retained/ignored declarations. - Fix public accessibility analysis false-positive for enum case parameter types. - Fix public accessibility analysis false-positive for properties initialized with generic specialized types. +- Types associated with assign-only properties are no longer identified as unused until the property is removed. ## 2.17.1 (2023-12-04) diff --git a/Sources/PeripheryKit/Formatters/OutputDeclarationFilter.swift b/Sources/PeripheryKit/Formatters/OutputDeclarationFilter.swift index ff64b96f7..d15fa5bd2 100644 --- a/Sources/PeripheryKit/Formatters/OutputDeclarationFilter.swift +++ b/Sources/PeripheryKit/Formatters/OutputDeclarationFilter.swift @@ -14,7 +14,7 @@ public final class OutputDeclarationFilter { public func filter(_ declarations: [ScanResult]) -> [ScanResult] { if configuration.reportInclude.isEmpty && configuration.reportExclude.isEmpty { - return declarations + return declarations.sorted { $0.declaration < $1.declaration } } return declarations diff --git a/Sources/PeripheryKit/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift b/Sources/PeripheryKit/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift index 789854e1f..f09be7ecb 100644 --- a/Sources/PeripheryKit/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift +++ b/Sources/PeripheryKit/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift @@ -16,59 +16,19 @@ final class AssignOnlyPropertyReferenceEliminator: SourceGraphMutator { func mutate() throws { guard !configuration.retainAssignOnlyProperties else { return } - let setters = graph.declarations(ofKind: .functionAccessorSetter) - var assignOnlyProperties: [Declaration: [Reference]] = [:] - - for setter in setters { - let references = graph.references(to: setter) - - for reference in references { - // Ensure the property being assigned is simple. - guard - let caller = reference.parent, - let property = setter.parent, - !property.isComplexProperty - else { continue } - - // Ensure the property hasn't been been explicitly retained, e.g by a comment command. - guard !graph.isRetained(property) else { continue } - - // A protocol property can technically be assigned and never used when the protocol is used as an existential - // type, however communicating that succinctly would be very tricky, and most likely just lead to confusion. - // Here we filter out protocol properties and thus restrict this analysis only to concrete properties. - guard property.parent?.kind != .protocol else { continue } - - // Find all references to the property at the same call site as the setter reference. - let propertyReferences = caller.references.filter { - property.usrs.contains($0.usr) && $0.location == reference.location - } - - // Determine if the containing method contains references to the property's getter accessor. - let propertyGetterUSRs = property.declarations - .filter { $0.kind == .functionAccessorGetter } - .flatMap { $0.usrs } - let hasGetterReference = caller.references - .contains { $0.kind == .functionAccessorGetter && propertyGetterUSRs.contains($0.usr) } - - if !hasGetterReference { - propertyReferences.forEach { - assignOnlyProperties[property, default: []].append($0) - } - } - } - } - - for (property, references) in assignOnlyProperties { - if let declaredType = property.declaredType, - retainAssignOnlyPropertyTypes.contains(declaredType) { - continue - } - - graph.markPotentialAssignOnlyProperty(property) - - for reference in references { - graph.remove(reference) - } + for property in graph.declarations(ofKinds: Declaration.Kind.variableKinds) { + guard let declaredType = property.declaredType, + !retainAssignOnlyPropertyTypes.contains(declaredType), + !graph.isRetained(property), + !property.isComplexProperty, + !graph.references(to: property).contains(where: { $0.parent?.parent?.kind == .protocol }), + let setter = property.declarations.first(where: { $0.kind == .functionAccessorSetter }), + let getter = property.declarations.first(where: { $0.kind == .functionAccessorGetter }), + graph.hasReferences(to: setter), + !graph.hasReferences(to: getter) + else { continue } + + graph.markAssignOnlyProperty(property) } } } diff --git a/Sources/PeripheryKit/SourceGraph/Mutators/CodingKeyEnumReferenceBuilder.swift b/Sources/PeripheryKit/SourceGraph/Mutators/CodingKeyEnumReferenceBuilder.swift index 36ad2b075..b38c75f5e 100644 --- a/Sources/PeripheryKit/SourceGraph/Mutators/CodingKeyEnumReferenceBuilder.swift +++ b/Sources/PeripheryKit/SourceGraph/Mutators/CodingKeyEnumReferenceBuilder.swift @@ -4,11 +4,9 @@ import Shared /// Builds a reference from a `Codable` conforming type to any child enum that conforms to `CodingKey`. final class CodingKeyEnumReferenceBuilder: SourceGraphMutator { private let graph: SourceGraph - private let configuration: Configuration required init(graph: SourceGraph, configuration: Configuration) { self.graph = graph - self.configuration = configuration } func mutate() { @@ -19,13 +17,6 @@ final class CodingKeyEnumReferenceBuilder: SourceGraphMutator { $0.kind == .protocol && $0.name == "CodingKey" } - let codableTypes = ["Codable", "Decodable", "Encodable"] + configuration.externalEncodableProtocols + configuration.externalCodableProtocols - - let isParentCodable = graph.inheritedTypeReferences(of: parent).contains { - guard let name = $0.name else { return false } - return [.protocol, .typealias].contains($0.kind) && codableTypes.contains(name) - } - guard isCodingKey else { continue } // Retain each enum element. @@ -34,7 +25,7 @@ final class CodingKeyEnumReferenceBuilder: SourceGraphMutator { graph.markRetained(elem) } - if isParentCodable { + if graph.isCodable(parent) { // Build a reference from the Codable type to the CodingKey enum. for usr in enumDeclaration.usrs { let newReference = Reference(kind: .enum, usr: usr, location: enumDeclaration.location) diff --git a/Sources/PeripheryKit/SourceGraph/Mutators/DefaultConstructorReferenceBuilder.swift b/Sources/PeripheryKit/SourceGraph/Mutators/DefaultConstructorReferenceBuilder.swift index 7d283b7c1..3ca9313ef 100644 --- a/Sources/PeripheryKit/SourceGraph/Mutators/DefaultConstructorReferenceBuilder.swift +++ b/Sources/PeripheryKit/SourceGraph/Mutators/DefaultConstructorReferenceBuilder.swift @@ -17,7 +17,9 @@ final class DefaultConstructorReferenceBuilder: SourceGraphMutator { private func referenceDefaultConstructors() { let defaultConstructors = graph.declarations(ofKind: .functionConstructor).filter { - $0.name == "init()" + // Some initializers are referenced internally, e.g by JSONEncoder/Decoder so we need + // to assume they are referenced. + $0.name == "init()" || $0.isImplicit } defaultConstructors.forEach { constructor in diff --git a/Sources/PeripheryKit/SourceGraph/Mutators/StructImplicitInitializerReferenceBuilder.swift b/Sources/PeripheryKit/SourceGraph/Mutators/StructImplicitInitializerReferenceBuilder.swift new file mode 100644 index 000000000..975cf4449 --- /dev/null +++ b/Sources/PeripheryKit/SourceGraph/Mutators/StructImplicitInitializerReferenceBuilder.swift @@ -0,0 +1,65 @@ +import Foundation +import Shared + +/// Builds references from struct implicit initializers to the properties it assigns. +final class StructImplicitInitializerReferenceBuilder: SourceGraphMutator { + private let graph: SourceGraph + + init(graph: SourceGraph, configuration: Configuration) { + self.graph = graph + } + + func mutate() throws { + for structDecl in graph.declarations(ofKind: .struct) { + let implicitInitDecl = structDecl.declarations.first { $0.kind == .functionConstructor && $0.isImplicit } + + guard let implicitInitDecl, let name = implicitInitDecl.name else { continue } + + let propertyNames = name + .dropFirst("init(".count) + .dropLast(")".count) + .split(separator: ":") + .map(String.init) + + let initPropertyDecls = structDecl.declarations.filter { + guard $0.kind == .varInstance, let name = $0.name, propertyNames.contains(name) + else { return false } + return true + } + + for propertyDecl in initPropertyDecls { + guard let setterDecl = propertyDecl.declarations.first(where: { $0.kind == .functionAccessorSetter }) + else { continue } + + for decl in [propertyDecl, setterDecl] { + for usr in decl.usrs { + let ref = Reference(kind: decl.kind, usr: usr, location: implicitInitDecl.location) + ref.name = decl.name + ref.parent = implicitInitDecl + graph.add(ref, from: implicitInitDecl) + } + } + } + + // The index contains references to properties at the call site of the implicit + // initializer. This pattern is contrary to explicit initializers, where only the + // initializer is referenced. Now that we've built the property references stemming from + // the implicit initializer, we can remove the additional property references at the + // call site. This enables assign-only property detection for structs using implicit + // initializers. + for initRef in graph.references(to: implicitInitDecl) { + guard let caller = initRef.parent, caller != structDecl else { continue } + + let sortedReferences = caller.references.sorted() + + for initPropertyDecl in initPropertyDecls { + let firstRef = sortedReferences.first { initPropertyDecl.usrs.contains($0.usr) && $0.location > initRef.location } + + if let firstRef { + graph.remove(firstRef) + } + } + } + } + } +} diff --git a/Sources/PeripheryKit/SourceGraph/SourceGraph.swift b/Sources/PeripheryKit/SourceGraph/SourceGraph.swift index 53d8f9a9e..40dd21fac 100644 --- a/Sources/PeripheryKit/SourceGraph/SourceGraph.swift +++ b/Sources/PeripheryKit/SourceGraph/SourceGraph.swift @@ -19,13 +19,18 @@ public final class SourceGraph { private(set) var allReferencesByUsr: [String: Set] = [:] private(set) var indexedModules: Set = [] private(set) var unusedModuleImports: Set = [] + private(set) var assignOnlyProperties: Set = [] - private var potentialAssignOnlyProperties: Set = [] private var allDeclarationsByKind: [Declaration.Kind: Set] = [:] private var allExplicitDeclarationsByUsr: [String: Declaration] = [:] private var moduleToExportingModules: [String: Set] = [:] private let lock = UnfairLock() + private let configuration: Configuration + + init(configuration: Configuration = .shared) { + self.configuration = configuration + } public func indexingComplete() { rootDeclarations = allDeclarations.filter { $0.parent == nil } @@ -36,10 +41,6 @@ public final class SourceGraph { allDeclarations.subtracting(usedDeclarations) } - var assignOnlyProperties: Set { - return potentialAssignOnlyProperties.intersection(unusedDeclarations) - } - func declarations(ofKind kind: Declaration.Kind) -> Set { allDeclarationsByKind[kind] ?? [] } @@ -98,9 +99,9 @@ public final class SourceGraph { _ = retainedDeclarations.insert(declaration) } - func markPotentialAssignOnlyProperty(_ declaration: Declaration) { + func markAssignOnlyProperty(_ declaration: Declaration) { withLock { - _ = potentialAssignOnlyProperties.insert(declaration) + _ = assignOnlyProperties.insert(declaration) } } @@ -149,7 +150,7 @@ public final class SourceGraph { allDeclarationsByKind[declaration.kind]?.remove(declaration) rootDeclarations.remove(declaration) usedDeclarations.remove(declaration) - potentialAssignOnlyProperties.remove(declaration) + assignOnlyProperties.remove(declaration) declaration.usrs.forEach { allExplicitDeclarationsByUsr.removeValue(forKey: $0) } } @@ -341,4 +342,13 @@ public final class SourceGraph { result.formUnion(allOverrideDeclarations(fromBase: decl)) } } + + func isCodable(_ decl: Declaration) -> Bool { + let codableTypes = ["Codable", "Decodable", "Encodable"] + configuration.externalEncodableProtocols + configuration.externalCodableProtocols + + return inheritedTypeReferences(of: decl).contains { + guard let name = $0.name else { return false } + return [.protocol, .typealias].contains($0.kind) && codableTypes.contains(name) + } + } } diff --git a/Sources/PeripheryKit/SourceGraph/SourceGraphMutatorRunner.swift b/Sources/PeripheryKit/SourceGraph/SourceGraphMutatorRunner.swift index 28e9b3014..e2d7887f8 100644 --- a/Sources/PeripheryKit/SourceGraph/SourceGraphMutatorRunner.swift +++ b/Sources/PeripheryKit/SourceGraph/SourceGraphMutatorRunner.swift @@ -27,10 +27,11 @@ public final class SourceGraphMutatorRunner { ProtocolExtensionReferenceBuilder.self, ProtocolConformanceReferenceBuilder.self, ExternalTypeProtocolConformanceReferenceRemover.self, - DefaultConstructorReferenceBuilder.self, ComplexPropertyAccessorReferenceBuilder.self, EnumCaseReferenceBuilder.self, DynamicMemberLookupReferenceBuilder.self, + DefaultConstructorReferenceBuilder.self, + StructImplicitInitializerReferenceBuilder.self, UnusedParameterRetainer.self, AssetReferenceRetainer.self, diff --git a/Tests/Fixtures/RetentionFixtures/testCodingKeyEnum.swift b/Tests/Fixtures/RetentionFixtures/testCodingKeyEnum.swift index ab370d92a..f52c3e352 100644 --- a/Tests/Fixtures/RetentionFixtures/testCodingKeyEnum.swift +++ b/Tests/Fixtures/RetentionFixtures/testCodingKeyEnum.swift @@ -62,23 +62,3 @@ public struct FixtureClass218: CustomStringConvertible { case someVar } } - -public class FixtureClass220: Encodable { - public var someUsedVar: String? - var someUnusedVar: String? - - enum CodingKeys: CodingKey { - case someUsedVar - case someUnusedVar - } -} - -public struct FixtureStruct220: Encodable { - public var someUsedVar: String? - var someUnusedVar: String? - - enum CodingKeys: String, CodingKey { - case someUsedVar - case someUnusedVar - } -} diff --git a/Tests/Fixtures/RetentionFixtures/testRetainsEncodableProperties.swift b/Tests/Fixtures/RetentionFixtures/testRetainsEncodableProperties.swift deleted file mode 100644 index a27aae7d9..000000000 --- a/Tests/Fixtures/RetentionFixtures/testRetainsEncodableProperties.swift +++ /dev/null @@ -1,40 +0,0 @@ -import Foundation - -public class FixtureClass204: Codable { - var someVar: String -} - -public class FixtureClass205: Encodable { - var someVar: String = "" -} - -public protocol FixtureProtocol206: Codable {} - -public class FixtureClass206: FixtureProtocol206 { - var someVar: String -} - -public protocol FixtureProtocol207: Encodable {} - -public class FixtureClass207: FixtureProtocol207 { - var someVar: String = "" -} - -public protocol FixtureProtocol208Base: Encodable {} -public protocol FixtureProtocol208: FixtureProtocol208Base {} - -public class FixtureClass208: FixtureProtocol208 { - var someVar: String = "" -} - -// CustomStringConvertible doesn't actually inherit Encodable, we're just using it because we don't have an external -// module in which to declare our own type. -public class FixtureClass209: CustomStringConvertible { - var someVar: String = "" - public var description: String { "" } -} - -public class FixtureClass210 { - var someVar: String = "" -} -extension FixtureClass210: Encodable {} diff --git a/Tests/Fixtures/RetentionFixtures/testStructImplicitInitializer.swift b/Tests/Fixtures/RetentionFixtures/testStructImplicitInitializer.swift new file mode 100644 index 000000000..ece85708a --- /dev/null +++ b/Tests/Fixtures/RetentionFixtures/testStructImplicitInitializer.swift @@ -0,0 +1,18 @@ +import Foundation + +public struct FixtureStruct13_Codable: Codable { + let assignOnly: Int +} + +public struct FixtureStruct13_NotCodable { + let assignOnly: Int + let used: Int +} + +public struct FixtureStruct13Retainer { + public func retain() throws { + let data = "".data(using: .utf8)! + _ = try JSONDecoder().decode(FixtureStruct13_Codable.self, from: data) + _ = FixtureStruct13_NotCodable(assignOnly: 0, used: 0).used + } +} diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift index deab71fb6..1751e628a 100644 --- a/Tests/PeripheryTests/RetentionTest.swift +++ b/Tests/PeripheryTests/RetentionTest.swift @@ -574,24 +574,6 @@ final class RetentionTest: FixtureSourceGraphTestCase { assertReferenced(.struct("FixtureClass218")) { self.assertReferenced(.enum("CodingKeys")) } - assertReferenced(.class("FixtureClass220")) { - self.assertReferenced(.varInstance("someUsedVar")) - self.assertNotReferenced(.varInstance("someUnusedVar")) - - self.assertReferenced(.enum("CodingKeys")) { - self.assertReferenced(.enumelement("someUsedVar")) - self.assertReferenced(.enumelement("someUnusedVar")) - } - } - assertReferenced(.struct("FixtureStruct220")) { - self.assertReferenced(.varInstance("someUsedVar")) - self.assertNotReferenced(.varInstance("someUnusedVar")) - - self.assertReferenced(.enum("CodingKeys")) { - self.assertReferenced(.enumelement("someUsedVar")) - self.assertReferenced(.enumelement("someUnusedVar")) - } - } } } @@ -1019,6 +1001,35 @@ final class RetentionTest: FixtureSourceGraphTestCase { // MARK: - Assign-only properties + func testStructImplicitInitializer() { + configuration.retainAssignOnlyProperties = false + + analyze(retainPublic: true) { + assertReferenced(.struct("FixtureStruct13_Codable")) { + self.assertAssignOnlyProperty(.varInstance("assignOnly")) + } + assertReferenced(.struct("FixtureStruct13_NotCodable")) { + self.assertAssignOnlyProperty(.varInstance("assignOnly")) + self.assertNotAssignOnlyProperty(.varInstance("used")) + } + } + + configuration.retainAssignOnlyProperties = true + + analyze(retainPublic: true) { + assertReferenced(.struct("FixtureStruct13_Codable")) { + self.assertReferenced(.varInstance("assignOnly")) + self.assertNotAssignOnlyProperty(.varInstance("assignOnly")) + } + assertReferenced(.struct("FixtureStruct13_NotCodable")) { + self.assertReferenced(.varInstance("assignOnly")) + self.assertNotAssignOnlyProperty(.varInstance("assignOnly")) + self.assertReferenced(.varInstance("used")) + self.assertNotAssignOnlyProperty(.varInstance("used")) + } + } + } + func testSimplePropertyAssignedButNeverRead() { configuration.retainAssignOnlyProperties = false