Don't flag typed factory constants in no_magic_numbers - #6842
Closed
lechuckcaptain wants to merge 2 commits into
Closed
Don't flag typed factory constants in no_magic_numbers#6842lechuckcaptain wants to merge 2 commits into
no_magic_numbers#6842lechuckcaptain wants to merge 2 commits into
Conversation
Generated by 🚫 Danger |
`no_magic_numbers` already exempts the component arguments of a `UIColor` initializer, but the check hardcoded that single type name, so SwiftUI's `Color` and AppKit's `NSColor` got no exemption at all. Recognise all three, and accept the `opacity` label alongside `alpha`, since SwiftUI spells it differently. AppKit encodes the color space in the label of the first component, so cover the `calibratedRed`, `deviceRed`, `srgbRed`, `calibratedWhite`, `deviceWhite`, `genericGamma22White`, `calibratedHue` and `deviceHue` variants as well as the `deviceCyan:magenta:yellow:black:alpha:` components. Recognising the type name alone would have left the first component of such a call reported while the rest were not. The exemption also required the literal to be the *immediate* argument, so `UIColor(red: 0.6, ...)` was exempt while `UIColor(red: 0x19 / 255, ...)` was not: there the literal's parent is the division, not the labelled argument. Walk up through arithmetic, parentheses and numeric conversions to find the enclosing argument instead, and bail out on anything else so an exemption granted for one argument cannot leak past it. That boundary is what keeps the existing manual bit-packing example `Int(r * 255) << 16` flagged, and `Color(...).opacity(0.42)` too, since in neither case is the literal inside a color initializer's argument list. Every literal remains judged by its own label, so an unrecognised label is exempted for nothing but itself: `UIColor(rgb: 0x33373A, alpha: 0.16)` keeps reporting the `rgb:` value while exempting `alpha:`, which is a color component whatever its sibling happens to be.
The rule exempts a literal bound directly to a name, so `let x: Int = 5` is clean while `let x: Duration = .seconds(5)` — the same constant, better typed — is flagged: the factory call breaks the grandparent chain `isMagicNumber` looks for. The rule therefore penalises the stronger API, and the finding tends to get silenced with a disable comment rather than fixed. Exempt a literal that is the sole argument of a factory call forming the whole initializer of a declaration whose declared type is listed in the new `definitional_types` option, defaulting to `Duration` and `Angle`. The type is read from the syntax — a member-access base, or the declaration's type annotation — because the rule has no type information; that is the same trade-off `test_parent_classes` already makes when it matches written superclass names. Gating on the declared type rather than on the call shape is what keeps arbitrary single-argument factories flagged: in `let x: Int = .factorial(20)` and `let d: Data = .randomBytes(64)` the name labels the result of a computation, not a unit. Requiring the call to *be* the initializer value keeps use sites such as `clock.sleep(for: .seconds(30))` flagged.
lechuckcaptain
force-pushed
the
no-magic-numbers-typed-factory-constants
branch
from
August 4, 2026 16:05
5e6b547 to
352bbb1
Compare
Author
|
Closing for now. This was stacked on #6841, which I have closed in favour of a smaller series, so the diff here was misleading. The change itself is still worth doing. I will raise it as a proposal issue first to agree the option name and default before reopening a PR, rather than leaving a draft sitting here. |
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
2 participants
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.
Note
Stacked on #6841. This branch is based on that one, so the diff below includes its commit too —
the second commit is the change described here. Happy to rebase onto
mainif you would ratherreview them independently, or if #6841 is not wanted.
Summary
no_magic_numbersexempts a literal bound directly to a name, solet x: Int = 5is clean whilelet x: Duration = .seconds(5)— the same constant, better typed — is flagged: the factory callbreaks the grandparent chain
isMagicNumberlooks for. The rule effectively penalises thestronger API.
initializer of a declaration whose declared type is listed in the new
definitional_typesoption, defaulting to
DurationandAngle.Duration.seconds(5)) or thedeclaration's type annotation (
let x: Duration = .seconds(5)) — since the rule has no typeinformation. That is the trade-off
test_parent_classesalready makes for superclass names; atypealiasis not followed.Gating on the declared type rather than on the call shape is what keeps arbitrary single-argument
factories flagged: in
let x: Int = .factorial(20)andlet d: Data = .randomBytes(64)the namelabels the result of a computation, not a unit. Requiring the call to be the initializer value
keeps use sites such as
clock.sleep(for: .seconds(30))flagged.Measurementis deliberately absent from the default. Every construction is either.init(value:unit:)orMeasurement(value:unit:), excluded by theinitcheck and thesole-argument requirement respectively, so listing it would be a no-op.
Validation
swift test --filter NoMagicNumbersRuleGeneratedTestspasses. No existing example was edited,reordered or removed; the new option is covered by
configuration:examples.swift run swiftlint-dev rules registerrun, sodefault_rule_configurations.ymlis up to date.swift run swiftlintclean on the changed files.4 removed, 0 introduced. The four are three
Durationconstants and oneAngle.degrees(90).Note for adopters: because this pattern is usually silenced with a disable comment today, projects
on
--strictmay seesuperfluous_disable_commandfire after upgrading where a block existed onlyfor this. That is the intended payoff, but the upgrade is not silent.