From af07827d2020cc4bf5606f92ca47a91d3789b467 Mon Sep 17 00:00:00 2001 From: Ian Leitch Date: Sun, 11 Jan 2026 10:33:59 +0100 Subject: [PATCH] Add warning for superfluous ignore comments (#427) Detect and report when `// periphery:ignore` comments are unnecessary because the declaration is actually referenced and would not have been reported as unused. This helps identify outdated ignore comments that should be removed, similar to SwiftLint's superfluous_disable_command rule. Superfluous ignore command results are excluded from baselines since they're warnings about unnecessary comments, not unused code. --- .mise/tasks/scripts/helpers.sh | 8 ++ .mise/tasks/write-baseline-bazel | 8 ++ .mise/tasks/write-baseline-linux | 7 +- .mise/tasks/write-baseline-linux-bazel | 7 +- CHANGELOG.md | 1 + MODULE.bazel.lock | 2 +- Sources/Extensions/String+Version.swift | 2 - Sources/Frontend/Commands/ScanCommand.swift | 1 + Sources/Indexer/SwiftIndexer.swift | 34 ++++--- .../Results/OutputFormatter.swift | 4 + Sources/PeripheryKit/ScanResult.swift | 9 ++ Sources/PeripheryKit/ScanResultBuilder.swift | 85 +++++++++++++++-- .../SourceGraph/Elements/CommentCommand.swift | 11 +++ Sources/SourceGraph/SourceGraph.swift | 10 ++ Sources/SourceGraph/SourceGraphDebugger.swift | 2 - Sources/SyntaxAnalysis/CommentCommand.swift | 38 ++++---- .../DeclarationSyntaxVisitor.swift | 6 +- .../testSuperfluousIgnoreCommand.swift | 94 +++++++++++++++++++ Tests/PeripheryTests/RetentionTest.swift | 46 +++++++++ .../Syntax/CommentCommandTest.swift | 76 +++++++++++++++ Tests/Shared/SourceGraphTestCase.swift | 48 ++++++++++ baselines/bazel.json | 21 ++--- baselines/linux-bazel.json | 1 + 23 files changed, 459 insertions(+), 62 deletions(-) create mode 100644 .mise/tasks/scripts/helpers.sh create mode 100755 .mise/tasks/write-baseline-bazel create mode 100644 Tests/Fixtures/Sources/RetentionFixtures/testSuperfluousIgnoreCommand.swift create mode 100644 Tests/PeripheryTests/Syntax/CommentCommandTest.swift diff --git a/.mise/tasks/scripts/helpers.sh b/.mise/tasks/scripts/helpers.sh new file mode 100644 index 000000000..86e6df8fd --- /dev/null +++ b/.mise/tasks/scripts/helpers.sh @@ -0,0 +1,8 @@ +# Shared helper functions for mise tasks + +# Formats a JSON baseline file in place +format_baseline() { + local file="$1" + jq . "$file" > "$file.tmp" + mv "$file.tmp" "$file" +} diff --git a/.mise/tasks/write-baseline-bazel b/.mise/tasks/write-baseline-bazel new file mode 100755 index 000000000..cc1e7384e --- /dev/null +++ b/.mise/tasks/write-baseline-bazel @@ -0,0 +1,8 @@ +#!/bin/bash + +set -euo pipefail + +source "$(dirname "$0")/scripts/helpers.sh" + +bazel run :periphery -- scan --bazel --write-baseline baselines/bazel.json +format_baseline baselines/bazel.json diff --git a/.mise/tasks/write-baseline-linux b/.mise/tasks/write-baseline-linux index 6e373a715..0d9a1e20b 100755 --- a/.mise/tasks/write-baseline-linux +++ b/.mise/tasks/write-baseline-linux @@ -1,11 +1,12 @@ #!/bin/bash -set -e +set -euo pipefail + +source "$(dirname "$0")/scripts/helpers.sh" export DOCKER_CLI_HINTS=false docker build -t periphery -f docker/Dockerfile-Linux . docker run --name periphery_write_baseline -t periphery scan "$@" --write-baseline /baseline.json docker cp periphery_write_baseline:baseline.json ./baselines/linux.json docker rm periphery_write_baseline -jq . ./baselines/linux.json > ./baselines/linux.json.tmp -mv ./baselines/linux.json.tmp ./baselines/linux.json +format_baseline baselines/linux.json diff --git a/.mise/tasks/write-baseline-linux-bazel b/.mise/tasks/write-baseline-linux-bazel index bffbd617c..05413c9f4 100755 --- a/.mise/tasks/write-baseline-linux-bazel +++ b/.mise/tasks/write-baseline-linux-bazel @@ -1,11 +1,12 @@ #!/bin/bash -set -e +set -euo pipefail + +source "$(dirname "$0")/scripts/helpers.sh" export DOCKER_CLI_HINTS=false docker build -t periphery -f docker/Dockerfile-Bazel . docker run --name periphery_write_baseline -t periphery scan "$@" --bazel --write-baseline /baseline.json docker cp periphery_write_baseline:baseline.json ./baselines/linux-bazel.json docker rm periphery_write_baseline -jq . ./baselines/linux-bazel.json > ./baselines/linux-bazel.json.tmp -mv ./baselines/linux-bazel.json.tmp ./baselines/linux-bazel.json +format_baseline baselines/linux-bazel.json diff --git a/CHANGELOG.md b/CHANGELOG.md index c0c7a0abb..1b0c59954 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ##### Enhancements - The `--color` option now accepts one of `auto`, `always` and `never`. In `auto` mode, color is disabled for dumb terminals and non-TTYs. +- Added detection of superfluous `// periphery:ignore` comments. A warning is now reported when an ignore comment is unnecessary because the declaration is actually used. ##### Bug Fixes diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 11fa27a61..8621f3a50 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -193,7 +193,7 @@ "//bazel:generated.bzl%generated": { "general": { "bzlTransitiveDigest": "nMR2FBcoRPImVocN9DNOnm2NQWyTbJPu7SHJgAXsLFw=", - "usagesDigest": "Hdmxc+LdckmZqHgeSNbQlCU0uQaBARRWkeCpjro+Xhk=", + "usagesDigest": "S5zTCGsw3nugb0U3s6bPvFYl04dzqTMrLLUzzabunO8=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, diff --git a/Sources/Extensions/String+Version.swift b/Sources/Extensions/String+Version.swift index 42006b3f3..07ad33916 100644 --- a/Sources/Extensions/String+Version.swift +++ b/Sources/Extensions/String+Version.swift @@ -1,5 +1,3 @@ -// periphery:ignore:all - import Foundation public typealias VersionString = String diff --git a/Sources/Frontend/Commands/ScanCommand.swift b/Sources/Frontend/Commands/ScanCommand.swift index 14d1a743c..373250166 100644 --- a/Sources/Frontend/Commands/ScanCommand.swift +++ b/Sources/Frontend/Commands/ScanCommand.swift @@ -262,6 +262,7 @@ struct ScanCommand: ParsableCommand { if let baselinePath = configuration.writeBaseline { let usrs = filteredResults + .filter(\.includeInBaseline) .flatMapSet { $0.usrs } .union(baseline?.usrs ?? []) let baseline = Baseline.v1(usrs: usrs.sorted()) diff --git a/Sources/Indexer/SwiftIndexer.swift b/Sources/Indexer/SwiftIndexer.swift index 9f4f28447..2a8dfbadf 100644 --- a/Sources/Indexer/SwiftIndexer.swift +++ b/Sources/Indexer/SwiftIndexer.swift @@ -371,10 +371,10 @@ final class SwiftIndexer: Indexer { let fileCommands = syntaxVisitor.parseComments() if fileCommands.contains(.ignoreAll) { - retainHierarchy(declarations) + retainHierarchy(declarations, markExplicitlyIgnored: true) } else { for decl in declarations where decl.commentCommands.contains(.ignore) { - retainHierarchy([decl]) + retainHierarchy([decl], markExplicitlyIgnored: true) } } } @@ -399,6 +399,7 @@ final class SwiftIndexer: Indexer { decl.modifiers = Set(result.modifiers) decl.commentCommands = Set(result.commentCommands) decl.declaredType = result.variableType + decl.hasGenericFunctionReturnedMetatypeParameters = result.hasGenericFunctionReturnedMetatypeParameters for ref in decl.references.union(decl.related) { @@ -433,13 +434,17 @@ final class SwiftIndexer: Indexer { } } - private func retainHierarchy(_ decls: [Declaration]) { + private func retainHierarchy(_ decls: [Declaration], markExplicitlyIgnored: Bool = false) { for decl in decls { graph.withLock { graph in graph.markRetained(decl) decl.unusedParameters.forEach { graph.markRetained($0) } + if markExplicitlyIgnored { + graph.markExplicitlyIgnored(decl) + decl.unusedParameters.forEach { graph.markExplicitlyIgnored($0) } + } } - retainHierarchy(Array(decl.declarations)) + retainHierarchy(Array(decl.declarations), markExplicitlyIgnored: markExplicitlyIgnored) } } @@ -465,6 +470,17 @@ final class SwiftIndexer: Indexer { $0[$1.location] = $1 } + // Build a map of ignored param names per function, and track functions with ignored + // params so ScanResultBuilder can efficiently detect superfluous ignores. + var ignoredParamsByLocation: [Location: [String]] = [:] + for functionDecl in functionDecls { + let ignoredParamNames = functionDecl.commentCommands.ignoredParameterNames + if !ignoredParamNames.isEmpty { + ignoredParamsByLocation[functionDecl.location] = ignoredParamNames + graph.withLock { $0.markHasIgnoredParameters(functionDecl) } + } + } + let analyzer = UnusedParameterAnalyzer() let paramsByFunction = analyzer.analyze( file: syntaxVisitor.sourceFile, @@ -480,14 +496,7 @@ final class SwiftIndexer: Indexer { continue } - let ignoredParamNames = functionDecl.commentCommands.flatMap { command -> [String] in - switch command { - case let .ignoreParameters(params): - return params - default: - return [] - } - } + let ignoredParamNames = ignoredParamsByLocation[functionDecl.location] ?? [] graph.withLock { graph in for param in params { @@ -501,6 +510,7 @@ final class SwiftIndexer: Indexer { if (functionDecl.isObjcAccessible && configuration.retainObjcAccessible) || ignoredParamNames.contains(param.name.text) { graph.markRetained(paramDecl) + graph.markExplicitlyIgnored(paramDecl) } } } diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 42cfac01e..33b861632 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -33,6 +33,8 @@ extension OutputFormatter { "redundantProtocol" case .redundantPublicAccessibility: "redundantPublicAccessibility" + case .superfluousIgnoreCommand: + "superfluousIgnoreCommand" } } @@ -64,6 +66,8 @@ extension OutputFormatter { case let .redundantPublicAccessibility(modules): let modulesJoined = modules.sorted().joined(separator: ", ") description += "Redundant public accessibility for \(kindDisplayName) '\(name)' (not used outside of \(modulesJoined))" + case .superfluousIgnoreCommand: + description += "Superfluous ignore comment for \(kindDisplayName) '\(name)' (declaration is referenced and should not be ignored)" } } else { description += "Unused" diff --git a/Sources/PeripheryKit/ScanResult.swift b/Sources/PeripheryKit/ScanResult.swift index cc346d3c6..e558270db 100644 --- a/Sources/PeripheryKit/ScanResult.swift +++ b/Sources/PeripheryKit/ScanResult.swift @@ -7,6 +7,7 @@ public struct ScanResult { case assignOnlyProperty case redundantProtocol(references: Set, inherited: Set) case redundantPublicAccessibility(modules: Set) + case superfluousIgnoreCommand } let declaration: Declaration @@ -15,4 +16,12 @@ public struct ScanResult { public var usrs: Set { declaration.usrs } + + /// Indicates whether this result should be included in baselines. + /// Superfluous ignore command results are excluded since they're warnings + /// about unnecessary comments, not unused code. + public var includeInBaseline: Bool { + if case .superfluousIgnoreCommand = annotation { return false } + return true + } } diff --git a/Sources/PeripheryKit/ScanResultBuilder.swift b/Sources/PeripheryKit/ScanResultBuilder.swift index fc3d1a053..97d632e71 100644 --- a/Sources/PeripheryKit/ScanResultBuilder.swift +++ b/Sources/PeripheryKit/ScanResultBuilder.swift @@ -40,17 +40,90 @@ public enum ScanResultBuilder { let annotatedRedundantPublicAccessibility: [ScanResult] = redundantPublicAccessibility.map { .init(declaration: $0.0, annotation: .redundantPublicAccessibility(modules: $0.1)) } + + // Detect superfluous ignore commands + // 1. Declarations with ignore comments that have references from non-ignored code + let superfluousDeclarations = graph.explicitlyIgnoredDeclarations + .filter { decl in + hasReferencesFromNonIgnoredCode(decl, graph: graph) + } + + // 2. Parameters with ignore comments that are actually used (not in unusedParameters) + let superfluousParamResults = findSuperfluousParameterIgnores(graph: graph) + + let annotatedSuperfluousIgnoreCommands: [ScanResult] = superfluousDeclarations + .map { .init(declaration: $0, annotation: .superfluousIgnoreCommand) } + + superfluousParamResults + let allAnnotatedDeclarations = annotatedRemovableDeclarations + annotatedAssignOnlyProperties + annotatedRedundantProtocols + - annotatedRedundantPublicAccessibility + annotatedRedundantPublicAccessibility + + annotatedSuperfluousIgnoreCommands return allAnnotatedDeclarations - .filter { - !$0.declaration.isImplicit && - !$0.declaration.kind.isAccessorKind && - !graph.ignoredDeclarations.contains($0.declaration) && - !graph.retainedDeclarations.contains($0.declaration) + .filter { result in + guard !result.declaration.isImplicit, + !result.declaration.kind.isAccessorKind, + !graph.ignoredDeclarations.contains(result.declaration) + else { return false } + + // Superfluous ignore command results must bypass the retained filter below. + // Declarations with ignore comments are always in retainedDeclarations (that's + // how ignore comments work), so filtering them out would prevent us from ever + // reporting superfluous ignore commands. + if case .superfluousIgnoreCommand = result.annotation { + return true + } + + return !graph.retainedDeclarations.contains(result.declaration) + } + } + + /// Checks if a declaration has references from code that is not part of the explicitly ignored set. + /// This indicates that the declaration would have been marked as used even without the ignore command. + private static func hasReferencesFromNonIgnoredCode(_ decl: Declaration, graph: SourceGraph) -> Bool { + let references = graph.references(to: decl) + + for ref in references { + guard let parent = ref.parent else { continue } + + // Check if the parent is not in the explicitly ignored set. + // This covers deeply nested declarations because retainHierarchy marks + // the entire descendant tree as explicitly ignored, not just the root. + if !graph.explicitlyIgnoredDeclarations.contains(parent) { + // Also check that the parent is actually used (not itself unused) + if graph.usedDeclarations.contains(parent) { + return true + } + } + } + + return false + } + + /// Finds parameters that have `// periphery:ignore:parameters` comments but are actually used. + /// If a parameter is ignored but NOT in unusedParameters, it means it's used and the ignore is superfluous. + private static func findSuperfluousParameterIgnores(graph: SourceGraph) -> [ScanResult] { + var results: [ScanResult] = [] + + for decl in graph.functionsWithIgnoredParameters { + let ignoredParamNames = decl.commentCommands.ignoredParameterNames + let unusedParamNames = Set(decl.unusedParameters.compactMap(\.name)) + + for ignoredParamName in ignoredParamNames { + if !unusedParamNames.contains(ignoredParamName) { + // The ignored parameter is actually used - create a result for it + let parentUsrs = decl.usrs.sorted().joined(separator: "-") + let usr = "param-\(ignoredParamName)-\(decl.name ?? "unknown-function")-\(parentUsrs)" + let paramDecl = Declaration(kind: .varParameter, usrs: [usr], location: decl.location) + paramDecl.name = ignoredParamName + paramDecl.parent = decl + results.append(.init(declaration: paramDecl, annotation: .superfluousIgnoreCommand)) + } } + } + + return results } } diff --git a/Sources/SourceGraph/Elements/CommentCommand.swift b/Sources/SourceGraph/Elements/CommentCommand.swift index 068659436..361cdf112 100644 --- a/Sources/SourceGraph/Elements/CommentCommand.swift +++ b/Sources/SourceGraph/Elements/CommentCommand.swift @@ -77,4 +77,15 @@ public extension Sequence { return nil } + + var ignoredParameterNames: [String] { + flatMap { command -> [String] in + switch command { + case let .ignoreParameters(params): + params + default: + [] + } + } + } } diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 6c06e0e12..0d5538e24 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -20,6 +20,8 @@ public final class SourceGraph { public private(set) var unusedModuleImports: Set = [] public private(set) var assignOnlyProperties: Set = [] public private(set) var extensions: [Declaration: Set] = [:] + public private(set) var explicitlyIgnoredDeclarations: Set = [] + public private(set) var functionsWithIgnoredParameters: Set = [] private var indexedModules: Set = [] private var unindexedExportedModules: Set = [] @@ -89,6 +91,14 @@ public final class SourceGraph { _ = ignoredDeclarations.insert(declaration) } + public func markExplicitlyIgnored(_ declaration: Declaration) { + _ = explicitlyIgnoredDeclarations.insert(declaration) + } + + public func markHasIgnoredParameters(_ declaration: Declaration) { + _ = functionsWithIgnoredParameters.insert(declaration) + } + public func markRetained(_ declaration: Declaration) { _ = retainedDeclarations.insert(declaration) } diff --git a/Sources/SourceGraph/SourceGraphDebugger.swift b/Sources/SourceGraph/SourceGraphDebugger.swift index 538417f57..e62f6ddba 100644 --- a/Sources/SourceGraph/SourceGraphDebugger.swift +++ b/Sources/SourceGraph/SourceGraphDebugger.swift @@ -1,5 +1,3 @@ -// periphery:ignore:all - import Foundation final class SourceGraphDebugger { diff --git a/Sources/SyntaxAnalysis/CommentCommand.swift b/Sources/SyntaxAnalysis/CommentCommand.swift index 1c0dec7c1..7379a763d 100644 --- a/Sources/SyntaxAnalysis/CommentCommand.swift +++ b/Sources/SyntaxAnalysis/CommentCommand.swift @@ -4,27 +4,29 @@ import SystemPackage extension CommentCommand { static func parseCommands(in trivia: Trivia?) -> [CommentCommand] { - let comments: [String] = trivia?.compactMap { - switch $0 { - case let .lineComment(comment), - let .blockComment(comment), - let .docLineComment(comment), - let .docBlockComment(comment): - comment - default: - nil + trivia?.compactMap { piece -> CommentCommand? in + // Extract comment text and marker length based on trivia piece kind + let parsed: (comment: String, markerLength: Int)? = switch piece { + case let .lineComment(text): (text, 2) // // + case let .docLineComment(text): (text, 3) // /// + case let .blockComment(text): (text, 2) // /* + case let .docBlockComment(text): (text, 3) // /** + default: nil } - } ?? [] - return comments - .compactMap { comment in - guard let range = comment.range(of: "periphery:") else { return nil } + guard let (comment, markerLength) = parsed, + let range = comment.range(of: "periphery:") else { return nil } - var rawCommand = String(comment[range.upperBound...]).replacingOccurrences(of: "*/", with: "").trimmed - // Anything after '-' in a comment command is ignored. - rawCommand = String(rawCommand.split(separator: "-").first ?? "").trimmed - return CommentCommand.parse(rawCommand) - } + // Only respect commands at the start of a comment (after the marker and whitespace). + let prefixStart = comment.index(comment.startIndex, offsetBy: markerLength) + let prefixBeforeCommand = String(comment[prefixStart ..< range.lowerBound]) + guard prefixBeforeCommand.trimmingCharacters(in: .whitespaces).isEmpty else { return nil } + + var rawCommand = String(comment[range.upperBound...]).replacingOccurrences(of: "*/", with: "").trimmed + // Anything after '-' in a comment command is ignored. + rawCommand = String(rawCommand.split(separator: "-").first ?? "").trimmed + return CommentCommand.parse(rawCommand) + } ?? [] } static func parse(_ rawCommand: String) -> Self? { diff --git a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift index 704cb748f..985dad60a 100644 --- a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift +++ b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift @@ -578,18 +578,18 @@ private extension SyntaxProtocol { /// Matches uses like these: /// /// ``` - /// // periphery:ignore + /// // ignore /// Foo { /// } /// ``` /// /// ``` - /// Foo { // periphery:ignore + /// Foo { // ignore /// } /// ``` /// /// ``` - /// Foo {} // periphery:ignore + /// Foo {} // ignore /// ``` var commentCommandTrivia: Trivia { var commandTrivia = leadingTrivia.merging(trailingTrivia) diff --git a/Tests/Fixtures/Sources/RetentionFixtures/testSuperfluousIgnoreCommand.swift b/Tests/Fixtures/Sources/RetentionFixtures/testSuperfluousIgnoreCommand.swift new file mode 100644 index 000000000..f82cbea0f --- /dev/null +++ b/Tests/Fixtures/Sources/RetentionFixtures/testSuperfluousIgnoreCommand.swift @@ -0,0 +1,94 @@ +import Foundation + +// MARK: - Superfluous ignore command (function is actually used) + +// periphery:ignore +public func superfluouslyIgnoredFunc() { + print("I am actually used!") +} + +// This function calls the ignored one, making the ignore superfluous +public func callerOfSuperfluouslyIgnoredFunc() { + superfluouslyIgnoredFunc() +} + +// MARK: - Superfluous ignore command on class (class is used) + +// periphery:ignore +public class SuperfluouslyIgnoredClass { + public func someMethod() {} +} + +public func useSuperfluouslyIgnoredClass() { + _ = SuperfluouslyIgnoredClass() +} + +// MARK: - Non-superfluous ignore command (function is NOT used) + +// periphery:ignore +public func correctlyIgnoredFunc() { + print("I am NOT used anywhere!") +} + +// MARK: - Non-superfluous ignore command (class is NOT used) + +// periphery:ignore +public class CorrectlyIgnoredClass { + public func someMethod() {} +} + +// MARK: - Ignored declaration within non-ignored parent + +public class NonIgnoredParentClass { + // This method is ignored but actually used - superfluous + // periphery:ignore + public func superfluouslyIgnoredMethod() { + print("I am used!") + } + + // This method is ignored and NOT used - correctly ignored + // periphery:ignore + public func correctlyIgnoredMethod() { + print("I am not used!") + } + + public func callerMethod() { + superfluouslyIgnoredMethod() + } +} + +// MARK: - Deeply nested declarations within ignored hierarchy + +// When an entire class is ignored, internal references between its members +// should NOT make those members appear superfluously ignored. +// periphery:ignore +public class DeeplyNestedIgnoredClass { + public func methodA() { + methodB() // Internal reference - should NOT make methodB superfluous + } + + public func methodB() { + methodC() // Internal reference - should NOT make methodC superfluous + } + + public func methodC() { + print("Deeply nested") + } +} + +// MARK: - Superfluous ignore for parameters + +public class ParameterIgnoreClass { + // This parameter is ignored but actually used - superfluous + // periphery:ignore:parameters usedParam + public func superfluousParamIgnore(usedParam: String) { + print(usedParam) // Parameter IS used, so ignore is superfluous + } + + // This parameter is ignored and NOT used - correctly ignored + // periphery:ignore:parameters unusedParam + public func correctParamIgnore(unusedParam: String) { + print("Not using the param") + } +} + diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift index 98561b46f..294963cc8 100644 --- a/Tests/PeripheryTests/RetentionTest.swift +++ b/Tests/PeripheryTests/RetentionTest.swift @@ -1173,6 +1173,52 @@ final class RetentionTest: FixtureSourceGraphTestCase { } } + func testSuperfluousIgnoreCommand() { + analyze(retainPublic: true) { + // These have ignore commands but are actually used, so the ignore is superfluous + assertSuperfluousIgnoreCommand(.functionFree("superfluouslyIgnoredFunc()")) + assertSuperfluousIgnoreCommand(.class("SuperfluouslyIgnoredClass")) + + // These have ignore commands and are NOT used, so the ignore is needed + assertNotSuperfluousIgnoreCommand(.functionFree("correctlyIgnoredFunc()")) + assertNotSuperfluousIgnoreCommand(.class("CorrectlyIgnoredClass")) + + // The callers should be referenced normally + assertReferenced(.functionFree("callerOfSuperfluouslyIgnoredFunc()")) + assertReferenced(.functionFree("useSuperfluouslyIgnoredClass()")) + + // Test ignored declarations within non-ignored parent + assertReferenced(.class("NonIgnoredParentClass")) { + // This method is ignored but used by callerMethod - superfluous + self.assertSuperfluousIgnoreCommand(.functionMethodInstance("superfluouslyIgnoredMethod()")) + // This method is ignored and not used - correctly ignored + self.assertNotSuperfluousIgnoreCommand(.functionMethodInstance("correctlyIgnoredMethod()")) + // The caller method should be referenced normally + self.assertReferenced(.functionMethodInstance("callerMethod()")) + } + + // Test deeply nested declarations within ignored hierarchy. + // Internal references between members of an ignored class should NOT + // make those members appear superfluously ignored. + assertNotSuperfluousIgnoreCommand(.class("DeeplyNestedIgnoredClass")) + assertNotSuperfluousIgnoreCommand(.functionMethodInstance("methodA()")) + assertNotSuperfluousIgnoreCommand(.functionMethodInstance("methodB()")) + assertNotSuperfluousIgnoreCommand(.functionMethodInstance("methodC()")) + + // Test superfluous ignore for parameters + assertReferenced(.class("ParameterIgnoreClass")) { + self.assertReferenced(.functionMethodInstance("superfluousParamIgnore(usedParam:)")) { + // Parameter is ignored but actually used - superfluous + self.assertSuperfluousIgnoreCommand(.varParameter("usedParam")) + } + self.assertReferenced(.functionMethodInstance("correctParamIgnore(unusedParam:)")) { + // Parameter is ignored and not used - correctly ignored + self.assertNotSuperfluousIgnoreCommand(.varParameter("unusedParam")) + } + } + } + } + // MARK: - Swift Testing #if canImport(Testing) diff --git a/Tests/PeripheryTests/Syntax/CommentCommandTest.swift b/Tests/PeripheryTests/Syntax/CommentCommandTest.swift new file mode 100644 index 000000000..f3136b948 --- /dev/null +++ b/Tests/PeripheryTests/Syntax/CommentCommandTest.swift @@ -0,0 +1,76 @@ +import Foundation +@testable import SourceGraph +import SwiftSyntax +@testable import SyntaxAnalysis +import XCTest + +final class CommentCommandTest: XCTestCase { + func testParseIgnore() { + assertParsesCommand("// periphery:ignore", expected: .ignore) + assertParsesCommand("/// periphery:ignore", expected: .ignore) + assertParsesCommand("/* periphery:ignore */", expected: .ignore) + assertParsesCommand("/** periphery:ignore */", expected: .ignore) + } + + func testParseIgnoreAll() { + assertParsesCommand("// periphery:ignore:all", expected: .ignoreAll) + } + + func testParseIgnoreParameters() { + assertParsesCommand("// periphery:ignore:parameters foo", expected: .ignoreParameters(["foo"])) + assertParsesCommand("// periphery:ignore:parameters foo,bar", expected: .ignoreParameters(["foo", "bar"])) + } + + func testAllowsLeadingWhitespace() { + assertParsesCommand("// periphery:ignore", expected: .ignore) + assertParsesCommand("/// periphery:ignore", expected: .ignore) + assertParsesCommand("/* periphery:ignore */", expected: .ignore) + } + + func testIgnoresCommandsWithPrecedingText() { + assertDoesNotParseCommand("// some text periphery:ignore") + assertDoesNotParseCommand("/// Docs about periphery:ignore") + assertDoesNotParseCommand("// `periphery:ignore` is used to ignore") + assertDoesNotParseCommand("/* text periphery:ignore */") + } + + func testTrailingCommentAfterHyphen() { + // Anything after '-' is treated as a trailing comment and ignored + assertParsesCommand("// periphery:ignore - this is a reason for ignoring", expected: .ignore) + assertParsesCommand("// periphery:ignore:all - ignore entire file", expected: .ignoreAll) + assertParsesCommand("// periphery:ignore:parameters foo - param is unused intentionally", expected: .ignoreParameters(["foo"])) + } + + // MARK: - Helpers + + private func assertParsesCommand(_ comment: String, expected: CommentCommand, file: StaticString = #file, line: UInt = #line) { + let trivia = parseTrivia(comment) + let commands = CommentCommand.parseCommands(in: trivia) + XCTAssertEqual(commands.count, 1, "Expected exactly one command from '\(comment)'", file: file, line: line) + if let command = commands.first { + XCTAssertEqual(command, expected, "Command mismatch for '\(comment)'", file: file, line: line) + } + } + + private func assertDoesNotParseCommand(_ comment: String, file: StaticString = #file, line: UInt = #line) { + let trivia = parseTrivia(comment) + let commands = CommentCommand.parseCommands(in: trivia) + XCTAssertTrue(commands.isEmpty, "Expected no commands from '\(comment)', but got: \(commands)", file: file, line: line) + } + + private func parseTrivia(_ comment: String) -> Trivia { + // Determine the trivia piece type based on comment prefix + let piece: TriviaPiece = if comment.hasPrefix("///") { + .docLineComment(comment) + } else if comment.hasPrefix("//") { + .lineComment(comment) + } else if comment.hasPrefix("/**") { + .docBlockComment(comment) + } else if comment.hasPrefix("/*") { + .blockComment(comment) + } else { + .lineComment(comment) + } + return Trivia(pieces: [piece]) + } +} diff --git a/Tests/Shared/SourceGraphTestCase.swift b/Tests/Shared/SourceGraphTestCase.swift index f3abf396e..09dc21b92 100644 --- a/Tests/Shared/SourceGraphTestCase.swift +++ b/Tests/Shared/SourceGraphTestCase.swift @@ -209,6 +209,44 @@ open class SourceGraphTestCase: XCTestCase { scopeStack.removeLast() } + func assertSuperfluousIgnoreCommand(_ description: DeclarationDescription, file: StaticString = #file, line: UInt = #line) { + // For parameters, we need to check results directly since they're created at result-building time + if description.kind == .varParameter { + let found = Self.results.superfluousIgnoreCommandDeclarations.contains { + $0.kind == description.kind && $0.name == description.name + } + if !found { + XCTFail("Expected superfluous ignore command for parameter: \(description.name)", file: file, line: line) + } + return + } + + guard let declaration = materialize(description, in: Self.allIndexedDeclarations, file: file, line: line) else { return } + + if !Self.results.superfluousIgnoreCommandDeclarations.contains(declaration) { + XCTFail("Expected declaration to have superfluous ignore command: \(declaration)", file: file, line: line) + } + } + + func assertNotSuperfluousIgnoreCommand(_ description: DeclarationDescription, file: StaticString = #file, line: UInt = #line) { + // For parameters, we need to check results directly since they're created at result-building time + if description.kind == .varParameter { + let found = Self.results.superfluousIgnoreCommandDeclarations.contains { + $0.kind == description.kind && $0.name == description.name + } + if found { + XCTFail("Expected no superfluous ignore command for parameter: \(description.name)", file: file, line: line) + } + return + } + + guard let declaration = materialize(description, in: Self.allIndexedDeclarations, file: file, line: line) else { return } + + if Self.results.superfluousIgnoreCommandDeclarations.contains(declaration) { + XCTFail("Expected declaration to not have superfluous ignore command: \(declaration)", file: file, line: line) + } + } + func assertOverrides(_ description: DeclarationDescription, _ overrides: [CommentCommand.Override], file: StaticString = #file, line: UInt = #line) { guard let declaration = materialize(description, file: file, line: line) else { XCTFail("Failed to materialize \(description)", file: file, line: line) @@ -312,4 +350,14 @@ private extension [ScanResult] { return nil } } + + var superfluousIgnoreCommandDeclarations: Set { + compactMapSet { + if case .superfluousIgnoreCommand = $0.annotation { + return $0.declaration + } + + return nil + } + } } diff --git a/baselines/bazel.json b/baselines/bazel.json index 33393450d..835493c5d 100644 --- a/baselines/bazel.json +++ b/baselines/bazel.json @@ -1,13 +1,10 @@ { - "v1": { - "usrs": [ - "s:13Configuration15AbstractSettingP5resetyyF", - "s:13Configuration7SettingC5resetyyF", - "s:13ConfigurationAAC13resetMatchersyyF", - "s:13ConfigurationAAC5resetyyF", - "s:13SystemPackage8FilePathV10ExtensionsE5chdir7closureyyyKXE_tKF", - "s:14SyntaxAnalysis21UnusedParameterParserV5parse4file0F9ProtocolsSayAA8FunctionVG11SourceGraph0J4FileC_SbtKFZ", - "s:14SyntaxAnalysis8FunctionV8fullNameSSvp" - ] - } -} \ No newline at end of file + "v1": { + "usrs": [ + "s:11SourceGraph0aB8DebuggerC", + "s:13SystemPackage8FilePathV10ExtensionsE5chdir7closureyyyKXE_tKF", + "s:14SyntaxAnalysis21UnusedParameterParserV5parse4file0F9ProtocolsSayAA8FunctionVG11SourceGraph0J4FileC_SbtKFZ", + "s:14SyntaxAnalysis8FunctionV8fullNameSSvp" + ] + } +} diff --git a/baselines/linux-bazel.json b/baselines/linux-bazel.json index c54f8ef14..f55df1d7e 100644 --- a/baselines/linux-bazel.json +++ b/baselines/linux-bazel.json @@ -2,6 +2,7 @@ "v1": { "usrs": [ "s:10Extensions4Glob33_78772790CB7745B67917FD65D1BCE611LLC", + "s:11SourceGraph0aB8DebuggerC", "s:11SourceGraph15ProjectFileKindO10extensionsSaySSGvp", "s:13SystemPackage8FilePathV10ExtensionsE4globyShyACGSSFZ", "s:13SystemPackage8FilePathV10ExtensionsE5chdir7closureyyyKXE_tKF",