Add disallow_default_parameter opt-in rule#6506
Open
William-Laverty wants to merge 3 commits intorealm:mainfrom
Open
Add disallow_default_parameter opt-in rule#6506William-Laverty wants to merge 3 commits intorealm:mainfrom
disallow_default_parameter opt-in rule#6506William-Laverty wants to merge 3 commits intorealm:mainfrom
Conversation
Generated by 🚫 Danger |
Adds a new opt-in rule that flags default parameter values on functions with certain access levels. By default, it disallows defaults on private and internal functions, encouraging explicit parameter passing at non-public API boundaries. Configurable via `disallowed_access_levels` to target any combination of `private`, `fileprivate`, `internal`, and `package`. Closes realm#6488
William-Laverty
force-pushed
the
disallow-default-parameter
branch
from
26e530a to
0081fe6
Compare
SimplyDanny
requested changes
Feb 22, 2026
Collaborator
SimplyDanny
left a comment
SimplyDanny
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
There are a few remarks I'd like to have addressed before merging.
| name: "Disallow Default Parameter", | ||
| description: "Default parameter values should not be used in functions with certain access levels", | ||
| kind: .idiomatic, | ||
| nonTriggeringExamples: DisallowDefaultParameterRuleExamples.nonTriggeringExamples, |
Collaborator
There was a problem hiding this comment.
If there aren't too many examples, it's okay to inline them here.
| static let description = RuleDescription( | ||
| identifier: "disallow_default_parameter", | ||
| name: "Disallow Default Parameter", | ||
| description: "Default parameter values should not be used in functions with certain access levels", |
Collaborator
There was a problem hiding this comment.
Suggested change
| description: "Default parameter values should not be used in functions with certain access levels", | |
| description: "Default parameter values should not be used in functions with certain access levels.", |
and use a shorter reason with every violation mentioning the access level like
"Default parameter values should not be used in '<visibility>' functions"| identifier: "disallow_default_parameter", | ||
| name: "Disallow Default Parameter", | ||
| description: "Default parameter values should not be used in functions with certain access levels", | ||
| kind: .idiomatic, |
Collaborator
There was a problem hiding this comment.
Fits better:
Suggested change
| kind: .idiomatic, | |
| kind: .lint, |
|
|
||
| private func collectViolations( | ||
| modifiers: DeclModifierListSyntax, | ||
| signature: FunctionSignatureSyntax |
Collaborator
There was a problem hiding this comment.
If you use ParameterClauseSyntax here, you can implement the case for SubscriptDeclSyntax exactly like the other two.
| ) { | ||
| guard matchesDisallowedAccessLevel(modifiers) else { return } | ||
| for param in signature.parameterClause.parameters where param.defaultValue != nil { | ||
| violations.append(param.defaultValue!.positionAfterSkippingLeadingTrivia) |
Collaborator
There was a problem hiding this comment.
I'd prefer to have the default parameter unwrapped in an optional binding instead of relying on the where implicitly.
| key: "disallowed_access_levels", | ||
| postprocessor: { $0 = Set($0.sorted()) } | ||
| ) | ||
| private(set) var disallowedAccessLevels: Set<AccessLevel> = [.private, .internal] |
Collaborator
There was a problem hiding this comment.
Why do you suggest these two as defaults?
| private(set) var severityConfiguration = SeverityConfiguration<Parent>(.warning) | ||
| @ConfigurationElement( | ||
| key: "disallowed_access_levels", | ||
| postprocessor: { $0 = Set($0.sorted()) } |
Collaborator
There was a problem hiding this comment.
Postprocessor can be removed. Neither order nor uniqueness are important.
Incidentally, with Set($0.sorted()) sorting would have no effect. What you want is Set($0).sorted(), but then disallowedAccessLevels would need to be a list.
- Change kind from .idiomatic to .lint - Use per-violation reason with access level name - Unify subscript handling via ParameterClauseSyntax - Use optional binding instead of where + force unwrap - Remove postprocessor from configuration - Default to [.internal] only (most common implicit level) - Inline examples into rule description - Add CHANGELOG entry
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
3 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.
New Rule
Adds a new opt-in rule
disallow_default_parameterthat flags default parameter values on functions with certain access levels.Motivation
As described in #6488, default parameter values on non-public functions can mask bugs when parameters aren't properly forwarded. This rule enforces explicit parameterization at internal API boundaries while allowing defaults on public APIs where consumers benefit from convenience.
Configuration
disallowed_access_levels: Array of access levels to flag. Default:["private", "internal"]private,fileprivate,internal,packageExamples
Triggering (default config):
Non-triggering:
Closes #6488