diff --git a/Sources/Indexer/SwiftIndexer.swift b/Sources/Indexer/SwiftIndexer.swift index cab614ccd..453c62740 100644 --- a/Sources/Indexer/SwiftIndexer.swift +++ b/Sources/Indexer/SwiftIndexer.swift @@ -414,6 +414,7 @@ final class SwiftIndexer: Indexer { decl.modifiers = Set(result.modifiers) decl.commentCommands = Set(result.commentCommands) decl.declaredType = result.variableType + decl.isLetBinding = result.isLetBinding decl.hasGenericFunctionReturnedMetatypeParameters = result.hasGenericFunctionReturnedMetatypeParameters diff --git a/Sources/SourceGraph/Elements/Declaration.swift b/Sources/SourceGraph/Elements/Declaration.swift index 382c58403..c683f8d47 100644 --- a/Sources/SourceGraph/Elements/Declaration.swift +++ b/Sources/SourceGraph/Elements/Declaration.swift @@ -231,6 +231,7 @@ public final class Declaration { public var related: Set = [] public var isImplicit: Bool = false public var isObjcAccessible: Bool = false + public var isLetBinding: Bool = false private let hashValueCache: Int diff --git a/Sources/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift b/Sources/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift index 4f0c78895..c44bbafbb 100644 --- a/Sources/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift +++ b/Sources/SourceGraph/Mutators/AssignOnlyPropertyReferenceEliminator.swift @@ -48,13 +48,50 @@ final class AssignOnlyPropertyReferenceEliminator: SourceGraphMutator { guard !configuration.retainAssignOnlyProperties else { return } for property in graph.declarations(ofKinds: Declaration.Kind.variableKinds) { - if AssignOnlyPropertyAnalyzer.isAssignOnlyProperty(property, graph: graph, configuration: configuration) { - if graph.isRetained(property) { - graph.markSuppressedAssignOnlyProperty(property) - } else { - graph.markAssignOnlyProperty(property) - } + guard AssignOnlyPropertyAnalyzer.isAssignOnlyProperty(property, graph: graph, configuration: configuration) else { + continue } + + if isInitializedConstant(property) { + // The property is a `let` constant whose value is supplied by an explicit + // initializer that is itself used. Removing the declaration would orphan the + // `self.x = x` assignment in the initializer body and drop the matching + // initializer parameter, breaking every call site that supplies it. The property + // is therefore retained rather than reported, as it is not safely removable. + graph.markRetained(property) + } else if graph.isRetained(property) { + graph.markSuppressedAssignOnlyProperty(property) + } else { + graph.markAssignOnlyProperty(property) + } + } + } + + // MARK: - Private + + /// Returns true when the property is a `let` constant initialized by an explicit initializer + /// that is referenced elsewhere. + /// + /// A `let` property can only be assigned once, in an initializer body. When that initializer is + /// written explicitly and is used, its `self.x = x` assignment and corresponding parameter are + /// load-bearing: removing the property would leave the assignment without a target and the call + /// sites passing the argument without a parameter. Such a property is a genuine stored value + /// that simply isn't read, not an assign-only property, so it must not be reported. + /// + /// A `let` initialized only by a compiler-synthesized memberwise initializer is excluded, as is + /// one whose initializer is never called; in both cases the declaration can be removed safely. + private func isInitializedConstant(_ property: Declaration) -> Bool { + guard property.isLetBinding, + let setter = property.declarations.first(where: { $0.kind == .functionAccessorSetter }) + else { return false } + + let initializers = graph.references(to: setter) + .filter { $0.kind != .retained } + .compactMap(\.parent) + .filter { $0.kind == .functionConstructor && !$0.isImplicit } + + return initializers.contains { initializer in + graph.references(to: initializer).contains { $0.kind != .retained } } } } diff --git a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift index cd0432643..431d57bb6 100644 --- a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift +++ b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift @@ -22,7 +22,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { functionCallMetatypeArgumentLocations: Set, typeInitializerLocations: Set, variableInitExprLocations: Set, - hasGenericFunctionReturnedMetatypeParameters: Bool + hasGenericFunctionReturnedMetatypeParameters: Bool, + isLetBinding: Bool ) private let sourceLocationBuilder: SourceLocationBuilder @@ -191,6 +192,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { } public func visitPost(_ node: VariableDeclSyntax) { + let isLetBinding = node.bindingSpecifier.tokenKind == .keyword(.let) + for binding in node.bindings { if binding.pattern.is(IdentifierPatternSyntax.self) { let closureSignature = binding.initializer?.value.as(ClosureExprSyntax.self)?.signature @@ -205,6 +208,7 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { returnClause: closureSignature?.returnClause, variableInitFunctionCallExpr: functionCallExpr, variableInitExpr: binding.initializer?.value, + isLetBinding: isLetBinding, at: binding.positionAfterSkippingLeadingTrivia ) } else if let tuplePatternSyntax = binding.pattern.as(TuplePatternSyntax.self) { @@ -212,20 +216,22 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { node: node, pattern: tuplePatternSyntax, typeTuple: binding.typeAnnotation?.type.as(TupleTypeSyntax.self)?.elements, - initializerTuple: binding.initializer?.value.as(TupleExprSyntax.self)?.elements + initializerTuple: binding.initializer?.value.as(TupleExprSyntax.self)?.elements, + isLetBinding: isLetBinding ) } else { parse( modifiers: node.modifiers, attributes: node.attributes, trivia: node.leadingTrivia.merging(node.trailingTrivia), + isLetBinding: isLetBinding, at: binding.positionAfterSkippingLeadingTrivia ) } } } - func visitVariableTupleBinding(node: VariableDeclSyntax, pattern: TuplePatternSyntax, typeTuple: TupleTypeElementListSyntax?, initializerTuple: LabeledExprListSyntax?) { + func visitVariableTupleBinding(node: VariableDeclSyntax, pattern: TuplePatternSyntax, typeTuple: TupleTypeElementListSyntax?, initializerTuple: LabeledExprListSyntax?, isLetBinding: Bool) { let elements = Array(pattern.elements) let types: [TupleTypeElementSyntax?] = typeTuple?.map(\.self) ?? Array(repeating: nil, count: elements.count) let initializers: [LabeledExprSyntax?] = initializerTuple?.map(\.self) ?? Array(repeating: nil, count: elements.count) @@ -239,7 +245,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { node: node, pattern: elementTuplePattern, typeTuple: typeTuple, - initializerTuple: initializerTuple + initializerTuple: initializerTuple, + isLetBinding: isLetBinding ) } else { parse( @@ -248,6 +255,7 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { trivia: node.commentCommandTrivia, variableType: type?.type, variableInitFunctionCallExpr: initializer?.expression.as(FunctionCallExprSyntax.self), + isLetBinding: isLetBinding, at: element.positionAfterSkippingLeadingTrivia ) } @@ -314,6 +322,7 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { variableInitFunctionCallExpr: FunctionCallExprSyntax? = nil, variableInitExpr: ExprSyntax? = nil, typeInitializerClause: TypeInitializerClauseSyntax? = nil, + isLetBinding: Bool = false, at position: AbsolutePosition ) { let modifierNames = modifiers?.map(\.name.text) ?? [] @@ -367,7 +376,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { functionCallMetatypeArgumentLocations: functionCallMetatypeArgumentLocations(for: variableInitFunctionCallExpr), typeInitializerLocations: typeLocations(for: typeInitializerClause?.value), variableInitExprLocations: memberBaseLocations(for: variableInitExpr), - hasGenericFunctionReturnedMetatypeParameters: hasGenericFunctionReturnedMetatypeParameters + hasGenericFunctionReturnedMetatypeParameters: hasGenericFunctionReturnedMetatypeParameters, + isLetBinding: isLetBinding )) } diff --git a/Tests/Fixtures/Sources/RetentionFixtures/testRetainsInitializedConstantProperties.swift b/Tests/Fixtures/Sources/RetentionFixtures/testRetainsInitializedConstantProperties.swift new file mode 100644 index 000000000..1dfba3aaf --- /dev/null +++ b/Tests/Fixtures/Sources/RetentionFixtures/testRetainsInitializedConstantProperties.swift @@ -0,0 +1,29 @@ +import Foundation + +// A `let` property supplied by an explicit, used initializer is a stored value that simply isn't +// read, not an assign-only property. Removing it would orphan `self.identifier = identifier` and +// drop the initializer parameter that the call site below relies on. +public struct FixtureStruct230: Identifiable { + public let id = UUID() + public let identifier: String + + public init(identifier: String) { + self.identifier = identifier + } +} + +// The initializer of this type is never called, so the `let` property remains safely removable and +// is still reported as assign-only. +public struct FixtureStruct231 { + let identifier: String + + init(identifier: String) { + self.identifier = identifier + } +} + +public struct FixtureStruct230Retainer { + public func retain() { + _ = FixtureStruct230(identifier: "") + } +} diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift index 599bb83ec..611130df5 100644 --- a/Tests/PeripheryTests/RetentionTest.swift +++ b/Tests/PeripheryTests/RetentionTest.swift @@ -1060,6 +1060,25 @@ final class RetentionTest: FixtureSourceGraphTestCase { } } + func testRetainsInitializedConstantProperties() { + analyze(retainPublic: true, retainAssignOnlyProperties: false) { + // The initializer is used, so its assignment and parameter are load-bearing and the + // `let` property must be retained rather than reported as assign-only. + assertReferenced(.struct("FixtureStruct230")) { + self.assertReferenced(.functionConstructor("init(identifier:)")) + self.assertReferenced(.varInstance("identifier")) + self.assertNotAssignOnlyProperty(.varInstance("identifier")) + } + + // The initializer is never called, so the `let` property remains safely removable and + // is still reported as assign-only. + assertReferenced(.struct("FixtureStruct231")) { + self.assertNotReferenced(.functionConstructor("init(identifier:)")) + self.assertAssignOnlyProperty(.varInstance("identifier")) + } + } + } + func testRetainsFilesOption() { analyze(retainFiles: [testFixturePath.string]) { assertReferenced(.class("FixtureClass100"))