diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/DiscouragedDefaultParameterRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/DiscouragedDefaultParameterRule.swift index ed4f15f49..0414205c2 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/DiscouragedDefaultParameterRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/DiscouragedDefaultParameterRule.swift @@ -17,6 +17,9 @@ struct DiscouragedDefaultParameterRule: Rule { nonTriggeringExamples: [ Example("public func foo(bar: Int = 0) {}"), Example("open func foo(bar: Int = 0) {}"), + Example("public extension Foo { func foo(bar: Int = 0) {} }"), + Example("extension E { public func foo(bar: Int = 0) {} }"), + Example("func outer() { func inner(bar: Int = 0) {} }"), Example("func foo(bar: Int) {}"), Example("private func foo(bar: Int = 0) {}"), Example("fileprivate func foo(bar: Int = 0) {}"), @@ -32,6 +35,8 @@ struct DiscouragedDefaultParameterRule: Rule { Example("package func foo(bar: Int ↓= 0) {}"), Example("func foo(bar: Int ↓= 0, baz: String ↓= \"\") {}"), Example("init(value: Int ↓= 42) {}"), + Example("class C { public func foo(bar: Int ↓= 0) {} }"), + Example("struct S { public init(value: Int ↓= 42) {} }"), Example( "private func foo(bar: Int ↓= 0) {}", configuration: ["disallowed_access_levels": ["private"]] @@ -45,47 +50,43 @@ struct DiscouragedDefaultParameterRule: Rule { } private extension DiscouragedDefaultParameterRule { - final class Visitor: ViolationsSyntaxVisitor { - override func visitPost(_ node: FunctionDeclSyntax) { - collectViolations(modifiers: node.modifiers, parameterClause: node.signature.parameterClause) + final class Visitor: EffectiveAccessControlSyntaxVisitor { + init(configuration: ConfigurationType, file: SwiftLintFile) { + super.init(configuration: configuration, file: file) } - override func visitPost(_ node: InitializerDeclSyntax) { + override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind { collectViolations(modifiers: node.modifiers, parameterClause: node.signature.parameterClause) + return .skipChildren } - override func visitPost(_ node: SubscriptDeclSyntax) { + override func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind { + collectViolations(modifiers: node.modifiers, parameterClause: node.signature.parameterClause) + return .skipChildren + } + + override func visit(_ node: SubscriptDeclSyntax) -> SyntaxVisitorContinueKind { collectViolations(modifiers: node.modifiers, parameterClause: node.parameterClause) + return .skipChildren } - private func collectViolations( - modifiers: DeclModifierListSyntax, - parameterClause: FunctionParameterClauseSyntax - ) { - guard let accessLevel = effectiveAccessLevel(modifiers), + private func collectViolations(modifiers: DeclModifierListSyntax, + parameterClause: FunctionParameterClauseSyntax) { + guard !isInLocalAccessControlScope, + case let accessLevel = effectiveAccessControlLevel(for: modifiers), configuration.disallowedAccessLevels.contains(accessLevel) else { return } - let levelName = accessLevel.rawValue for param in parameterClause.parameters { if let defaultValue = param.defaultValue { violations.append( - ReasonedRuleViolation( + .init( position: defaultValue.positionAfterSkippingLeadingTrivia, - reason: "Default parameter values should not be used in '\(levelName)' functions" + reason: "Default parameter values should not be used in '\(accessLevel)' functions" ) ) } } } - - private func effectiveAccessLevel(_ modifiers: DeclModifierListSyntax) - -> DiscouragedDefaultParameterConfiguration.AccessLevel? { - if modifiers.contains(keyword: .private) { return .private } - if modifiers.contains(keyword: .fileprivate) { return .fileprivate } - if modifiers.contains(keyword: .package) { return .package } - if modifiers.contains(keyword: .public) || modifiers.contains(keyword: .open) { return nil } - return .internal - } } } diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/MissingDocsRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/MissingDocsRule.swift index 49ea44bb5..6a5fb6765 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/MissingDocsRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/MissingDocsRule.swift @@ -16,25 +16,22 @@ struct MissingDocsRule: Rule { } private extension MissingDocsRule { - final class Visitor: ViolationsSyntaxVisitor { - private var aclScope = Stack() + final class Visitor: EffectiveAccessControlSyntaxVisitor { + init(configuration: ConfigurationType, file: SwiftLintFile) { + super.init( + configuration: configuration, + file: file, + evaluateEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel + ) + } override func visit(_ node: ActorDeclSyntax) -> SyntaxVisitorContinueKind { - defer { - aclScope.push( - behavior: .actor(node.modifiers.accessibility), - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) - } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } collectViolation(from: node, on: node.actorKeyword) - return .visitChildren - } - - override func visitPost(_: ActorDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visitPost(_ node: AssociatedTypeDeclSyntax) { @@ -42,21 +39,12 @@ private extension MissingDocsRule { } override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind { - defer { - aclScope.push( - behavior: .class(node.modifiers.accessibility), - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) - } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } collectViolation(from: node, on: node.classKeyword) - return .visitChildren - } - - override func visitPost(_: ClassDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visit(_: ClosureExprSyntax) -> SyntaxVisitorContinueKind { @@ -68,15 +56,14 @@ private extension MissingDocsRule { } override func visitPost(_ node: EnumCaseDeclSyntax) { - guard !node.hasDocComment, case let .enum(enumAcl) = aclScope.peek() else { + guard !node.hasDocComment, let enumAccessControlLevel else { return } - let acl = enumAcl ?? .internal - if let parameter = configuration.parameters.first(where: { $0.value == acl }) { + if let parameter = configuration.parameters.first(where: { $0.value == enumAccessControlLevel }) { violations.append( - ReasonedRuleViolation( + .init( position: node.caseKeyword.positionAfterSkippingLeadingTrivia, - reason: "\(acl) declarations should be documented", + reason: "\(enumAccessControlLevel) declarations should be documented", severity: parameter.severity ) ) @@ -84,36 +71,23 @@ private extension MissingDocsRule { } override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind { - defer { - aclScope.push( - behavior: .enum(node.modifiers.accessibility), - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) - } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } collectViolation(from: node, on: node.enumKeyword) - return .visitChildren - } - - override func visitPost(_: EnumDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visit(_ node: ExtensionDeclSyntax) -> SyntaxVisitorContinueKind { - defer { aclScope.push(.extension(node.modifiers.accessibility)) } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } if !configuration.excludesExtensions { collectViolation(from: node, on: node.extensionKeyword) } - return .visitChildren - } - - override func visitPost(_: ExtensionDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visit(_ node: FunctionDeclSyntax) -> SyntaxVisitorContinueKind { @@ -129,39 +103,21 @@ private extension MissingDocsRule { } override func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind { - defer { - aclScope.push( - behavior: .protocol(node.modifiers.accessibility), - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) - } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } collectViolation(from: node, on: node.protocolKeyword) - return .visitChildren - } - - override func visitPost(_: ProtocolDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind { - defer { - aclScope.push( - behavior: .struct(node.modifiers.accessibility), - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) - } if node.inherits, configuration.excludesInheritedTypes { + _ = super.visit(node) return .skipChildren } collectViolation(from: node, on: node.structKeyword) - return .visitChildren - } - - override func visitPost(_: StructDeclSyntax) { - aclScope.pop() + return super.visit(node) } override func visit(_ node: SubscriptDeclSyntax) -> SyntaxVisitorContinueKind { @@ -182,10 +138,7 @@ private extension MissingDocsRule { if node.modifiers.contains(keyword: .override) || node.hasDocComment { return } - let acl = aclScope.computeAcl( - givenExplicitAcl: node.modifiers.accessibility, - evalEffectiveAcl: configuration.evaluateEffectiveAccessControlLevel - ) + let acl = effectiveAccessControlLevel(for: node.modifiers) if let parameter = configuration.parameters.first(where: { $0.value == acl }) { violations.append( ReasonedRuleViolation( @@ -230,94 +183,3 @@ private extension SyntaxProtocol { } } } - -private extension DeclModifierListSyntax { - var accessibility: AccessControlLevel? { - filter { $0.detail == nil }.compactMap { AccessControlLevel(description: $0.name.text) }.first - } -} - -private enum AccessControlBehavior { - case `actor`(AccessControlLevel?) - case local - case `class`(AccessControlLevel?) - case `enum`(AccessControlLevel?) - case `extension`(AccessControlLevel?) - case `protocol`(AccessControlLevel?) - case `struct`(AccessControlLevel?) - - var effectiveAcl: AccessControlLevel { - explicitAcl ?? .internal - } - - var explicitAcl: AccessControlLevel? { - switch self { - case let .actor(acl): acl - case .local: nil - case let .class(acl): acl - case let .enum(acl): acl - case let .extension(acl): acl - case let .protocol(acl): acl - case let .struct(acl): acl - } - } - - func sameWith(acl: AccessControlLevel) -> Self { - switch self { - case .actor: .actor(acl) - case .local: .local - case .class: .class(acl) - case .enum: .enum(acl) - case .extension: .extension(acl) - case .protocol: .protocol(acl) - case .struct: .struct(acl) - } - } -} - -/// Implementation of Swift's effective ACL logic. Should be moved to a specialized syntax visitor for reuse some time. -private extension Stack { - mutating func push(behavior: AccessControlBehavior, evalEffectiveAcl: Bool) { - if let parentBehavior = peek() { - switch parentBehavior { - case .local: - push(.local) - case .actor, .class, .struct, .enum: - if behavior.effectiveAcl <= parentBehavior.effectiveAcl || !evalEffectiveAcl { - push(behavior) - } else { - push(behavior.sameWith(acl: parentBehavior.effectiveAcl)) - } - case .extension, .protocol: - if behavior.explicitAcl != nil { - push(behavior) - } else { - push(behavior.sameWith(acl: parentBehavior.effectiveAcl)) - } - } - } else { - push(behavior) - } - } - - func computeAcl(givenExplicitAcl acl: AccessControlLevel?, evalEffectiveAcl: Bool) -> AccessControlLevel { - if let parentBehavior = peek() { - switch parentBehavior { - case .local: - .private - case .actor, .class, .struct, .enum: - if let acl { - acl < parentBehavior.effectiveAcl || !evalEffectiveAcl ? acl : parentBehavior.effectiveAcl - } else { - parentBehavior.effectiveAcl >= .internal ? .internal : parentBehavior.effectiveAcl - } - case .protocol: - parentBehavior.effectiveAcl - case .extension: - acl ?? parentBehavior.effectiveAcl - } - } else { - acl ?? .internal - } - } -} diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/DiscouragedDefaultParameterConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/DiscouragedDefaultParameterConfiguration.swift index 1df96dc9d..b0837e974 100644 --- a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/DiscouragedDefaultParameterConfiguration.swift +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/DiscouragedDefaultParameterConfiguration.swift @@ -5,7 +5,7 @@ struct DiscouragedDefaultParameterConfiguration: SeverityBasedRuleConfiguration @ConfigurationElement(key: "severity") private(set) var severityConfiguration = SeverityConfiguration(.warning) @ConfigurationElement(key: "disallowed_access_levels") - private(set) var disallowedAccessLevels: Set = [.internal, .package] + private(set) var disallowedAccessLevels: Set = [.internal, .package] @AcceptableByConfigurationElement enum AccessLevel: String, Comparable { diff --git a/Source/SwiftLintCore/Models/AccessControlLevel.swift b/Source/SwiftLintCore/Models/AccessControlLevel.swift index 6570f916d..59bf0a4bf 100644 --- a/Source/SwiftLintCore/Models/AccessControlLevel.swift +++ b/Source/SwiftLintCore/Models/AccessControlLevel.swift @@ -65,3 +65,17 @@ extension AccessControlLevel: Comparable { lhs.priority < rhs.priority } } + +extension AccessControlLevel: AcceptableByConfigurationElement { + public init(fromAny value: Any, context ruleID: String) throws(Issue) { + if let value = value as? String, let newSelf = Self(description: value) { + self = newSelf + } else { + throw .invalidConfiguration(ruleID: ruleID) + } + } + + public func asOption() -> OptionType { + .symbol(description) + } +} diff --git a/Source/SwiftLintCore/Visitors/EffectiveAccessControlSyntaxVisitor.swift b/Source/SwiftLintCore/Visitors/EffectiveAccessControlSyntaxVisitor.swift new file mode 100644 index 000000000..e259926a9 --- /dev/null +++ b/Source/SwiftLintCore/Visitors/EffectiveAccessControlSyntaxVisitor.swift @@ -0,0 +1,239 @@ +import SwiftSyntax + +/// A `ViolationsSyntaxVisitor` with helpers to compute effective access control levels for declarations. +open class EffectiveAccessControlSyntaxVisitor: + ViolationsSyntaxVisitor { + /// Whether to apply the effective access control level computation or to use the explicitly + /// declared access control level. + public let evaluateEffectiveAcl: Bool + + private var aclScope = Stack() + + /// Creates a new `EffectiveAccessControlSyntaxVisitor`. + /// + /// - Parameters: + /// - configuration: The rule configuration to use for this visitor. + /// - file: The file to analyze. + /// - evaluateEffectiveAcl: Whether to apply the effective access control level computation or to use the + /// explicitly declared access control level. + @inlinable + public init(configuration: Configuration, file: SwiftLintFile, evaluateEffectiveAcl: Bool = true) { + self.evaluateEffectiveAcl = evaluateEffectiveAcl + super.init(configuration: configuration, file: file) + } + + private enum AccessControlScope { + case `actor` + case local + case `class` + case `enum` + case `extension` + case `protocol` + case `struct` + } + + /// Computes the effective access control level for a declaration. + /// + /// - Parameters: + /// - modifiers: Declaration modifiers that may contain an explicit access control level. + /// - Returns: Effective access control level of this declaration. + public func effectiveAccessControlLevel(for modifiers: DeclModifierListSyntax) -> AccessControlLevel { + if let parentBehavior = aclScope.peek() { + let acl = modifiers.accessibility + switch parentBehavior { + case .local: + return .private + case .actor, .class, .struct, .enum: + if let acl { + let isEffectiveAclApplied = acl < parentBehavior.effectiveAcl || !evaluateEffectiveAcl + return isEffectiveAclApplied ? acl : parentBehavior.effectiveAcl + } + return parentBehavior.effectiveAcl >= .internal ? .internal : parentBehavior.effectiveAcl + case .protocol: + return parentBehavior.effectiveAcl + case .extension: + return acl ?? parentBehavior.effectiveAcl + } + } + return modifiers.accessibility ?? .internal + } + + /// Access control level of the current enum scope. + /// + /// - Returns: Access control level if currently in an enum scope. + public var enumAccessControlLevel: AccessControlLevel? { + guard case let .enum(acl) = aclScope.peek() else { + return nil + } + return acl ?? .internal + } + + /// Whether the current declaration context is local. + public var isInLocalAccessControlScope: Bool { + guard case .local = aclScope.peek() else { + return false + } + return true + } + + // MARK: - Automatic Scope Management + + override open func visit(_ node: ActorDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.actor, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: ActorDeclSyntax) { + aclScope.pop() + } + + override open func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.class, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: ClassDeclSyntax) { + aclScope.pop() + } + + override open func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.enum, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: EnumDeclSyntax) { + aclScope.pop() + } + + override open func visit(_ node: ExtensionDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.extension, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: ExtensionDeclSyntax) { + aclScope.pop() + } + + override open func visit(_ node: ProtocolDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.protocol, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: ProtocolDeclSyntax) { + aclScope.pop() + } + + override open func visit(_ node: StructDeclSyntax) -> SyntaxVisitorContinueKind { + pushAccessControlScope(.struct, modifiers: node.modifiers) + return .visitChildren + } + + override open func visitPost(_: StructDeclSyntax) { + aclScope.pop() + } + + private func pushAccessControlScope(_ scope: AccessControlScope, modifiers: DeclModifierListSyntax) { + let behavior = accessControlBehavior(scope, modifiers: modifiers) + if let parentBehavior = aclScope.peek() { + switch parentBehavior { + case .local: + aclScope.push(.local) + case .actor, .class, .struct, .enum: + if behavior.effectiveAcl <= parentBehavior.effectiveAcl || !evaluateEffectiveAcl { + aclScope.push(behavior) + } else { + aclScope.push(behavior.sameWith(acl: parentBehavior.effectiveAcl)) + } + case .extension, .protocol: + if behavior.explicitAcl != nil { + aclScope.push(behavior) + } else { + aclScope.push(behavior.sameWith(acl: parentBehavior.effectiveAcl)) + } + } + } else { + aclScope.push(behavior) + } + } + + private func pushLocalAccessControlScope() { + if let parentBehavior = aclScope.peek() { + switch parentBehavior { + case .local: + aclScope.push(.local) + case .actor, .class, .struct, .enum: + // Local scope should always be considered private. + aclScope.push(.local) + case .extension, .protocol: + aclScope.push(.local) + } + } else { + aclScope.push(.local) + } + } + + private func accessControlBehavior(_ scope: AccessControlScope, + modifiers: DeclModifierListSyntax) -> AccessControlBehavior { + let accessibility = modifiers.accessibility + switch scope { + case .actor: + return .actor(accessibility) + case .local: + return .local + case .class: + return .class(accessibility) + case .enum: + return .enum(accessibility) + case .extension: + return .extension(accessibility) + case .protocol: + return .protocol(accessibility) + case .struct: + return .struct(accessibility) + } + } +} + +private extension DeclModifierListSyntax { + var accessibility: AccessControlLevel? { + filter { $0.detail == nil }.compactMap { AccessControlLevel(description: $0.name.text) }.first + } +} + +private enum AccessControlBehavior { + case `actor`(AccessControlLevel?) + case local + case `class`(AccessControlLevel?) + case `enum`(AccessControlLevel?) + case `extension`(AccessControlLevel?) + case `protocol`(AccessControlLevel?) + case `struct`(AccessControlLevel?) + + var effectiveAcl: AccessControlLevel { + explicitAcl ?? .internal + } + + var explicitAcl: AccessControlLevel? { + switch self { + case let .actor(acl): acl + case .local: nil + case let .class(acl): acl + case let .enum(acl): acl + case let .extension(acl): acl + case let .protocol(acl): acl + case let .struct(acl): acl + } + } + + func sameWith(acl: AccessControlLevel) -> Self { + switch self { + case .actor: .actor(acl) + case .local: .local + case .class: .class(acl) + case .enum: .enum(acl) + case .extension: .extension(acl) + case .protocol: .protocol(acl) + case .struct: .struct(acl) + } + } +}