Revised for implicit internal: Add redundant internal and fileprivate analysis to go alongside redundant public analysis#1042
Open
danwood wants to merge 37 commits into
Open
Revised for implicit internal: Add redundant internal and fileprivate analysis to go alongside redundant public analysis#1042danwood wants to merge 37 commits into
danwood wants to merge 37 commits into
Conversation
Contributor
Author
|
Lovely. Apparently some tests aren't passing here even though they passed on my machine. Stand by... |
danwood
force-pushed
the
master
branch
2 times, most recently
from
b511fef to
9868cdd
Compare
Contributor
Author
|
@ileitch One thought I had - I'm concerned that this is opening up a bunch of new runtime flags. This one defines 2 new flags ( I don't know if you would be OK with changing an existing flag but what if we were to remove/deprecate |
Contributor
Author
|
Well when I put this to the test against a large code base, I found a lot of corner cases that needed to be addresssed. It feels pretty robust now. Unfortunately I can't figure out why the CI "lint" process is failing! Hopefully @ileitch has some way to work around this. |
Co-authored-by: Ian Leitch <ian@leitch.io>
This reverts commit 882e58d.
Fix the scanning logic for that, and update the tests to reflect that.
…can` results in no warnings
This commit fixes false positive warnings where internal declarations were incorrectly flagged as "not used outside of file" when they were actually being used in test files via @testable import. This only appeared using the --bazel flag on github. Fix a linux-only warning. Also, handle @usableFromInline to avoid false positives. Updated bazel.json and linux-bazel.json to suppress 3 new Bazel-specific false positives. When Bazel builds modules independently, each module's index store doesn't capture cross-module usage, leading to false "unused" warnings. The baseline suppresses these known artifacts while preserving detection of genuine issues. Updated the build script to clean SwiftPM cache before build to prevent occasional CI failure.
…and adding test cases.
…d from other files are NOT flagged as redundant internal
…tion Note: periphery ought to identify enum cases that are unused
…d cross-type The Swift indexer emits references to implicit accessor children (getter/setter USRs) rather than the varInstance/varStatic declaration itself. As a result, isReferencedFromDifferentTypeInSameFile found no same-file references and incorrectly flagged cross-type accesses as redundant fileprivate. Fix: extend the child-reference check (already done for concreteTypeKinds) to also cover varInstance and varStatic declarations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed via @NSApplicationDelegateAdaptor When a class is referenced only as a metatype argument to @NSApplicationDelegateAdaptor (or @UIApplicationDelegateAdaptor), Periphery correctly retains the adaptor property but did not suppress redundant-internal analysis on the delegate class itself. This caused a spurious suggestion to downgrade AppDelegate to fileprivate, which cascades to a compiler error because the property referencing it must match its access level. Fix: after retaining the adaptor property, walk its references to find the delegate class and unmark it from redundant-internal accessibility analysis. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… SwiftUI apps SwiftUI App conformances don't carry a @main attribute in source — the compiler synthesizes the entry point. As a result, mainAttributedDeclarations is empty when the entry point is a SwiftUI App struct, causing retainApplicationDelegateAdaptors to miss the adaptor property and fail to suppress the redundant-internal warning on the delegate class. Fix: when mainAttributedDeclarations is empty, fall back to scanning all class and struct declarations so that @NSApplicationDelegateAdaptor and @UIApplicationDelegateAdaptor properties are found regardless of entry point style. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…SApplicationDelegateAdaptor cascade SwiftUIRetainer: - Retain parent struct/class when @NSApplicationDelegateAdaptor property found; previously only the property was retained, causing the entire App type and everything it references to cascade as false-positive unused - Add App, Commands, Scene to specialProtocolNames so SwiftUI entry-point conformances are always retained AssignOnlyPropertyReferenceEliminator / Declaration / DeclarationSyntaxVisitor / SwiftIndexer: - Track isLetBinding on Declaration, propagated from syntax visitor through indexer - Skip let stored properties in assign-only check: SourceKit emits a setter for the init-time assignment but no getter for synthesized reads (Codable encode(to:), Hashable/Equatable synthesis, SwiftUI body), causing false assignOnlyProperty warnings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed used
The Swift index store does not always emit a reference occurrence when a
nested type is used as the type annotation of a stored property within the
same parent scope. For example:
struct PhraseRange {
private let status: PhraseStatus // no index reference to PhraseStatus
enum PhraseStatus { case unknown }
}
Without an index reference, the existing reference-walking in markUsed never
marks PhraseStatus as used, so it is falsely flagged as .unused.
Fix: when a parent type is marked used, collect its directly nested concrete
type declarations (enum, struct, class, protocol). Then walk the parent's
child variable declarations: if any child's declaredType name matches a
nested type (after stripping optional markers and array brackets), mark that
nested type as used.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original Description from PR #976: It's useful to make your swift types be as private as possible, to encourage encapsulation. This improvement adds new checks for types that are fileprivate or (implicitly/explicitly) internal, and lets you know if they could be made more private then they are currently.
This new PR fixes a big gap in the previous PR where I had missed out on implicit internal declarations! This now works with implicit internal declarations, and fixes a bunch of false positives once that got enabled. A bunch of new test cases to cover all of these corner cases.
Second-most-recent commit, 35357b8, is just a bunch of changes to the periphery code base, cleaning up based on the results of
mise r scanwith the current codebase. So it might be easier to look at the diff of the commit just before that, so you don't have to get distracted by the trivial changes to accessibility.Update: OK that took some effort to clear the CI hurdles — see last commit, 9868cdd. Especially since scanning via bazel on my Mac produced very different warnings from the CI's logs. I left behind a hint as a comment in case somebody else wants to temporarily put in some debug output to see the USR of whatever symbols are causing warnings when scanning via Bazel.