Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/Indexer/SwiftIndexer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions Sources/SourceGraph/Elements/Declaration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ public final class Declaration {
public var related: Set<Reference> = []
public var isImplicit: Bool = false
public var isObjcAccessible: Bool = false
public var isLetBinding: Bool = false

private let hashValueCache: Int

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
}
}
}
20 changes: 15 additions & 5 deletions Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor {
functionCallMetatypeArgumentLocations: Set<Location>,
typeInitializerLocations: Set<Location>,
variableInitExprLocations: Set<Location>,
hasGenericFunctionReturnedMetatypeParameters: Bool
hasGenericFunctionReturnedMetatypeParameters: Bool,
isLetBinding: Bool
)

private let sourceLocationBuilder: SourceLocationBuilder
Expand Down Expand Up @@ -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
Expand All @@ -205,27 +208,30 @@ 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) {
visitVariableTupleBinding(
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)
Expand All @@ -239,7 +245,8 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor {
node: node,
pattern: elementTuplePattern,
typeTuple: typeTuple,
initializerTuple: initializerTuple
initializerTuple: initializerTuple,
isLetBinding: isLetBinding
)
} else {
parse(
Expand All @@ -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
)
}
Expand Down Expand Up @@ -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) ?? []
Expand Down Expand Up @@ -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
))
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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: "")
}
}
19 changes: 19 additions & 0 deletions Tests/PeripheryTests/RetentionTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
Loading