diff --git a/.swiftlint.yml b/.swiftlint.yml index 35d88937ca..e40f3244d7 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -62,6 +62,7 @@ attributes: identifier_name: excluded: - id + previous_function_behavior: true large_tuple: 3 number_separator: minimum_length: 5 diff --git a/CHANGELOG.md b/CHANGELOG.md index beca33dc87..3314d27bb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,10 @@ #### Breaking -* None. +* `identifier_name` now evaluates function names for both length and non + alphanumeric violations. Set the option + `previous_function_behavior: true` in your config file to revert to older + behavior #### Experimental @@ -19,6 +22,11 @@ * Rewrite `control_statement` rule using SwiftSyntax. [SimplyDanny](https://github.com/SimplyDanny) +* `identifier_name` now has the option to ignore minimum variable length + within short closures. + [mredig](https://github.com/mredig) + [5140](https://github.com/realm/SwiftLint/pull/5140) + #### Bug Fixes * Fix false positive in `control_statement` rule that triggered on conditions @@ -52,6 +60,11 @@ [Martin Redington](https://github.com/mildm8nnered) [#5120](https://github.com/realm/SwiftLint/issues/5120) +* `identifier_name` behavior corrected for accuracy to its description. See the + breaking change for more info + [mredig](https://github.com/mredig) + [5140](https://github.com/realm/SwiftLint/pull/5140) + ## 0.52.4: Lid Switch #### Breaking diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d9f33ac0fa..4f290b49fd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -57,8 +57,8 @@ with Swift Package Manager on Linux. When contributing code changes, please ensure that all three supported build methods continue to work and pass tests. ```shell -$ xcodebuild -scheme swiftlint test -$ swift test +$ xcodebuild -scheme swiftlint clean test -destination 'platform=macOS' | xcpretty +$ swift test | xcpretty $ make docker_test ``` diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/NameConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/NameConfiguration.swift index ce1c1c5df9..b264942f47 100644 --- a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/NameConfiguration.swift +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/NameConfiguration.swift @@ -18,6 +18,18 @@ struct NameConfiguration: RuleConfiguration, Equatable { private(set) var unallowedSymbolsSeverity = Severity.error @ConfigurationElement(key: "validates_start_with_lowercase") private(set) var validatesStartWithLowercase = StartWithLowercaseConfiguration.error + /// Only valid for `identifier_name` + @ConfigurationElement(key: "ignore_min_length_for_short_closure_content") + private(set) var ignoreMinLengthForShortClosureContent = false + /// Only valid for `identifier_name` + /// Before this update, the code skipped over functions in evaluating both their length and their special + /// characters. The code read `if !SwiftDeclarationKind.functionKinds.contains(kind) {` which is incredibly hard + /// to follow double negative logic, plus the description for the rule made no indication that functions were + /// excluded from this rule. And given the history of this rule as previously being variables only, but refactored + /// to the generic concept of identifiers (which should include function *identifiers*, this leads me to believe it + /// was unintentional. Hence, I'm defaulting to `false`. + @ConfigurationElement(key: "previous_function_behavior") + private(set) var previousFunctionBehavior = false var minLengthThreshold: Int { return max(minLength.warning, minLength.error ?? minLength.warning) @@ -38,7 +50,9 @@ struct NameConfiguration: RuleConfiguration, Equatable { excluded: [String] = [], allowedSymbols: [String] = [], unallowedSymbolsSeverity: Severity = .error, - validatesStartWithLowercase: StartWithLowercaseConfiguration = .error) { + validatesStartWithLowercase: StartWithLowercaseConfiguration = .error, + ignoreMinLengthForShortClosureContent: Bool = false, + previousFunctionBehavior: Bool = false) { minLength = SeverityLevels(warning: minLengthWarning, error: minLengthError) maxLength = SeverityLevels(warning: maxLengthWarning, error: maxLengthError) self.excludedRegularExpressions = Set(excluded.compactMap { @@ -47,6 +61,8 @@ struct NameConfiguration: RuleConfiguration, Equatable { self.allowedSymbols = Set(allowedSymbols) self.unallowedSymbolsSeverity = unallowedSymbolsSeverity self.validatesStartWithLowercase = validatesStartWithLowercase + self.ignoreMinLengthForShortClosureContent = ignoreMinLengthForShortClosureContent + self.previousFunctionBehavior = previousFunctionBehavior } mutating func apply(configuration: Any) throws { @@ -84,6 +100,12 @@ struct NameConfiguration: RuleConfiguration, Equatable { """ ).print() } + if let ignoreMinLengthForClosures = configurationDict[$ignoreMinLengthForShortClosureContent] as? Bool { + self.ignoreMinLengthForShortClosureContent = ignoreMinLengthForClosures + } + if let previousFunctionBehavior = configurationDict[$previousFunctionBehavior] as? Bool { + self.previousFunctionBehavior = previousFunctionBehavior + } } } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRule.swift index 5314ddfd8a..723edf8c22 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRule.swift @@ -1,148 +1,329 @@ import Foundation -import SourceKittenFramework +import SwiftSyntax -struct IdentifierNameRule: ASTRule, ConfigurationProviderRule { - var configuration = NameConfiguration(minLengthWarning: 3, - minLengthError: 2, - maxLengthWarning: 40, - maxLengthError: 60, - excluded: ["id"]) +struct IdentifierNameRule: SwiftSyntaxRule, ConfigurationProviderRule { + var configuration = NameConfiguration( + minLengthWarning: 3, + minLengthError: 2, + maxLengthWarning: 40, + maxLengthError: 60, + excluded: ["id"]) static let description = RuleDescription( identifier: "identifier_name", name: "Identifier Name", - description: "Identifier names should only contain alphanumeric characters and " + - "start with a lowercase character or should only contain capital letters. " + - "In an exception to the above, variable names may start with a capital letter " + - "when they are declared as static. Variable names should not be too " + - "long or too short.", + description: """ + Identifier names should only contain alphanumeric characters and \ + start with a lowercase character or should only contain capital letters. \ + In an exception to the above, variable names may start with a capital letter \ + when they are declared as static. Variable names should not be too \ + long or too short + """, kind: .style, nonTriggeringExamples: IdentifierNameRuleExamples.nonTriggeringExamples, triggeringExamples: IdentifierNameRuleExamples.triggeringExamples, deprecatedAliases: ["variable_name"] ) - func validate( - file: SwiftLintFile, - kind: SwiftDeclarationKind, - dictionary: SourceKittenDictionary - ) -> [StyleViolation] { - guard !dictionary.enclosedSwiftAttributes.contains(.override) else { - return [] + func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor { + Visitor(configuration: configuration, sourceLocationConverter: file.locationConverter) + } +} + +extension IdentifierNameRule { + final class Visitor: ViolationsSyntaxVisitor { + typealias Parent = IdentifierNameRule // swiftlint:disable:this nesting + + private var configuration: NameConfiguration + private var configurationStack: [NameConfiguration] = [] + + private static let maximumClosureLineCount = 10 + + let sourceLocationConverter: SourceLocationConverter + + init( + configuration: NameConfiguration, + sourceLocationConverter: SourceLocationConverter) { + self.sourceLocationConverter = sourceLocationConverter + self.configuration = configuration + super.init(viewMode: .sourceAccurate) + } + + // MARK: - Conformance + override func visitPost(_ node: EnumCaseElementSyntax) { + validateIdentifierNode(node.identifier, ofType: .enumElement) } - return validateName(dictionary: dictionary, kind: kind).map { name, offset in - guard let firstCharacter = name.first, !configuration.shouldExclude(name: name) else { - return [] + override func visitPost(_ node: FunctionDeclSyntax) { + let identifier = node.identifier + + switch identifier.tokenKind { + case .binaryOperator, .prefixOperator, .postfixOperator: + return + default: break } - let type = self.type(for: kind) - if !SwiftDeclarationKind.functionKinds.contains(kind) { - if !configuration.allowedSymbolsAndAlphanumerics.isSuperset(of: CharacterSet(charactersIn: name)) { - return [ - StyleViolation(ruleDescription: Self.description, - severity: configuration.unallowedSymbolsSeverity.severity, - location: Location(file: file, byteOffset: offset), - reason: """ - \(type) name '\(name)' should only contain alphanumeric and other \ - allowed characters - """) - ] - } - if let severity = configuration.severity(forLength: name.count) { - let reason = "\(type) name '\(name)' should be between " + - "\(configuration.minLengthThreshold) and " + - "\(configuration.maxLengthThreshold) characters long" - return [ - StyleViolation(ruleDescription: Self.description, - severity: severity, - location: Location(file: file, byteOffset: offset), - reason: reason) - ] + validateIdentifierNode(identifier, ofType: .function) + } + + override func visitPost(_ node: IdentifierPatternSyntax) { + validateIdentifierNode(node.identifier, ofType: .variable) + } + + override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { + if + configuration.ignoreMinLengthForShortClosureContent, + closureLineCount(node) <= Self.maximumClosureLineCount { + do { + let currentConfig = self.configuration + + try configuration.apply(configuration: ["min_length": 0]) + configurationStack.append(currentConfig) + } catch { + queuedFatalError("Failed creating temporary config") } } + return .visitChildren + } - if configuration.allowedSymbols.contains(String(firstCharacter)) { - return [] + override func visitPost(_ node: ClosureExprSyntax) { + if + configuration.ignoreMinLengthForShortClosureContent, + closureLineCount(node) <= Self.maximumClosureLineCount { + configuration = configurationStack.popLast() ?? configuration } - if let caseCheckSeverity = configuration.validatesStartWithLowercase.severity, - kind != .varStatic && name.isViolatingCase && !name.isOperator { - let reason = "\(type) name '\(name)' should start with a lowercase character" - return [ - StyleViolation(ruleDescription: Self.description, - severity: caseCheckSeverity, - location: Location(file: file, byteOffset: offset), - reason: reason) - ] + } + + private func validateIdentifierNode( + _ identifier: TokenSyntax, + ofType identifierType: IdentifierType) { + let name = cleanupName(identifier.text) + // confirm this node isn't in the exclusion list + // and that it has at least one character + guard + let firstCharacter = name.first.map(String.init), + configuration.shouldExclude(name: name) == false + else { return } + + // confirm this isn't an override + let previousNodes = lastThreeNodes(identifier: identifier) + guard nodeIsOverridden(previousNodes: previousNodes) == false else { return } + guard + let previousNode = previousNodes.first + else { queuedFatalError("No declaration node") } + + // alphanumeric characters + let nameForValidation = nodeIsPrivate(previousNodes: previousNodes) ? privateName(name) : name + let nameValidation = validate( + name: nameForValidation, + of: identifierType, + isValidWithin: configuration.allowedSymbolsAndAlphanumerics) + if case .fail(let reason, let severity) = nameValidation { + appendViolation( + at: previousNode.positionAfterSkippingLeadingTrivia, + reason: reason, + severity: severity) + return + } + + // identifier length + let lengthValidation = validateLength(of: name, ofType: identifierType, previousNode: previousNode) + if case .fail(let reason, let severity) = lengthValidation { + appendViolation( + at: previousNode.positionAfterSkippingLeadingTrivia, + reason: reason, + severity: severity) + return + } + + // at this point, the characters are all valid, it's just a matter of checking + // specifics regarding conditions on character positioning + + // allowed starter symbols + guard + configuration.allowedSymbols.contains(firstCharacter) == false + else { return } + + // nix CapitalCase values. + let camelValidation = validateCamelCase(of: name, ofType: identifierType, previousNodes: previousNodes) + if case .fail(let reason, let severity) = camelValidation { + let locationOffset: Int + switch identifierType { + case .enumElement: + locationOffset = sourceLocationConverter + .location(for: identifier.positionAfterSkippingLeadingTrivia) + .offset + default: + locationOffset = sourceLocationConverter + .location(for: previousNode.positionAfterSkippingLeadingTrivia) + .offset + } + appendViolation( + at: AbsolutePosition(utf8Offset: locationOffset), + reason: reason, + severity: severity) + } } - return [] - } ?? [] - } - - private func validateName( - dictionary: SourceKittenDictionary, - kind: SwiftDeclarationKind - ) -> (name: String, offset: ByteCount)? { - guard - var name = dictionary.name, - let offset = dictionary.offset, - kinds.contains(kind), - !name.hasPrefix("$") - else { return nil } - - if - kind == .enumelement, - let parenIndex = name.firstIndex(of: "("), - parenIndex > name.startIndex - { - let index = name.index(before: parenIndex) - name = String(name[...index]) + // MARK: - Metadata + private func nodeIsPrivate(previousNodes: [TokenSyntax]) -> Bool { + previousNodes.contains(where: { $0.tokenKind == .keyword(.private) }) + } + + private func privateName(_ name: String) -> String { + guard name.first == "_" else { return name } + return String(name[name.index(after: name.startIndex)...]) } - return (name.nameStrippingLeadingUnderscoreIfPrivate(dictionary), offset) - } - - private let kinds: Set = { - return SwiftDeclarationKind.variableKinds - .union(SwiftDeclarationKind.functionKinds) - .union([.enumelement]) - }() - - private func type(for kind: SwiftDeclarationKind) -> String { - if SwiftDeclarationKind.functionKinds.contains(kind) { - return "Function" - } else if kind == .enumelement { - return "Enum element" - } else { - return "Variable" + private func nodeIsStaticVariable(_ previousNodes: [TokenSyntax]) -> Bool { + nodeIsStatic(previousNodes: previousNodes) && nodeIsVariable(previousNodes: previousNodes) } - } -} -private extension String { - var isViolatingCase: Bool { - let firstCharacter = String(self[startIndex]) - guard firstCharacter.isUppercase() else { - return false + private func nodeIsVariable(previousNodes: [TokenSyntax]) -> Bool { + previousNodes.contains(where: { $0.tokenKind == .keyword(.let) }) || + previousNodes.contains(where: { $0.tokenKind == .keyword(.var) }) + } + + private func nodeIsStatic(previousNodes: [TokenSyntax]) -> Bool { + previousNodes.contains(where: { $0.tokenKind == .keyword(.static) }) + } + + private func nodeIsOverridden(previousNodes: [TokenSyntax]) -> Bool { + previousNodes.contains(where: { $0.tokenKind == .keyword(.override) }) + } + + private func closureLineCount(_ node: ClosureExprSyntax) -> Int { + let startLine = node.startLocation(converter: sourceLocationConverter).line + let endLine = node.endLocation(converter: sourceLocationConverter).line + return endLine - startLine + } + + enum IdentifierType: String { // swiftlint:disable:this nesting + case variable + case function + case enumElement = "enum element" } - guard count > 1 else { - return true + + // MARK: - Utility + private func cleanupName(_ name: String) -> String { + guard + name.first == "`", + name.last == "`", + name.count >= 3 + else { return name } + let oneInFront = name.index(after: name.startIndex) + let oneInBack = name.index(before: name.endIndex) + return String(name[oneInFront.. [TokenSyntax] { + var out: [TokenSyntax] = [] + + var current: TokenSyntax? = node + while + let previous = current?.previousToken(viewMode: .sourceAccurate), + out.count < 3 { + defer { current = current?.previousToken(viewMode: .sourceAccurate) } + out.append(previous) + } + + return out } - let secondCharacter = String(self[index(after: startIndex)]) - return secondCharacter.isLowercase() - } - - var isOperator: Bool { - let operators = ["/", "=", "-", "+", "!", "*", "|", "^", "~", "?", ".", "%", "<", ">", "&"] - return operators.contains(where: hasPrefix) - } - - func nameStrippingLeadingUnderscoreIfPrivate(_ dict: SourceKittenDictionary) -> String { - if let acl = dict.accessibility, - acl.isPrivate && first == "_" { - return String(self[index(after: startIndex)...]) + + // MARK: - Validation + private func validate( + name: String, + of identifierType: IdentifierType, + isValidWithin characterSet: CharacterSet) -> Validation { + if identifierType == .function, configuration.previousFunctionBehavior { + return .pass + } + guard characterSet.isSuperset(of: CharacterSet(charactersIn: name)) else { + let reason = """ + \(identifierType.rawValue.localizedCapitalized) name '\(name)' should only contain \ + alphanumeric and other allowed characters + """ + return .fail(reason: reason, severity: configuration.unallowedSymbolsSeverity.severity) + } + return .pass + } + + private func validateLength( + of name: String, + ofType identifierType: IdentifierType, + previousNode: TokenSyntax) -> Validation { + if identifierType == .function, configuration.previousFunctionBehavior { + return .pass + } + if let severity = configuration.severity(forLength: name.count) { + let reason = """ + \(identifierType.rawValue.localizedCapitalized) name '\(name)' should be between \ + \(configuration.minLengthThreshold) and \(configuration.maxLengthThreshold) characters long \ + (\(name.count) characters) + """ + return .fail(reason: reason, severity: severity) + } + return .pass + } + + private func validateCamelCase( + of name: String, + ofType identifierType: IdentifierType, + previousNodes: [TokenSyntax]) -> Validation { + if + let severity = configuration.validatesStartWithLowercase.severity, + name.first?.isUppercase == true, + doesNameStartCapitalCase(name) { + let reason = """ + \(identifierType.rawValue.localizedCapitalized) name '\(name)' should start \ + with a lowercase character + """ + // make an exeption for CamelCase static var/let + if nodeIsStaticVariable(previousNodes) == false { + return .fail(reason: reason, severity: severity) + } + } + return .pass + } + + private func doesNameStartCapitalCase(_ name: String) -> Bool { + guard + let firstCharacter = name.first + else { + return true // name is empty - shouldn't be possible + } + if firstCharacter.isLowercase { + return false + } + + guard + let secondIndex = name.index( + name.startIndex, + offsetBy: 1, + limitedBy: name.endIndex) + else { + return true + } + let secondCharacter = name[secondIndex] + return secondCharacter.isLowercase + } + + @discardableResult + private func appendViolation( + at position: AbsolutePosition, + reason: String, + severity: ViolationSeverity) -> ReasonedRuleViolation { + let violation = ReasonedRuleViolation( + position: position, + reason: reason, + severity: severity) + violations.append(violation) + return violation + } + + enum Validation { // swiftlint:disable:this nesting + case pass + case fail(reason: String, severity: ViolationSeverity) } - return self - } + } } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRuleExamples.swift index f704f03854..ab1d5776c5 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRuleExamples.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/IdentifierNameRuleExamples.swift @@ -23,13 +23,38 @@ internal struct IdentifierNameRuleExamples { class Foo { static var Bar = 0 } - """) + """), + Example(""" + class Foo { + let operationQueue: OperationQueue = { + let q = OperationQueue() + q.maxConcurrentOperationCount = ProcessInfo.processInfo.activeProcessorCount + return q + }() + } + """, + configuration: ["ignore_min_length_for_short_closure_content": true], + excludeFromDocumentation: true), + Example( + "private func h1(_ text: String) -> String { \"# \\(text)\" }", + configuration: ["previous_function_behavior": true], + excludeFromDocumentation: true), + Example( + """ + func hasAccessibilityElementChildrenIgnoreModifier(in file: SwiftLintFile) -> Bool { false } + """, + configuration: ["previous_function_behavior": true], + excludeFromDocumentation: true), + Example( + "func testLineAndCharacterForByteOffset_forContentsContainingMultibyteCharacters() {}", + configuration: ["previous_function_behavior": true], + excludeFromDocumentation: true) ] static let triggeringExamples = [ Example( "↓let MyLet = 0", - configuration: ["validates_start_with_lowercase": true], + configuration: ["validates_start_with_lowercase": "warning"], excludeFromDocumentation: true ), Example("↓let _myLet = 0"), @@ -48,7 +73,31 @@ internal struct IdentifierNameRuleExamples { Example( "enum Foo { case ↓MyEnum }", configuration: ["validates_start_with_lowercase": "error"], - excludeFromDocumentation: true - ) + excludeFromDocumentation: true), + Example(""" + class Foo { + let operationQueue: OperationQueue = { + ↓let q = OperationQueue() + q.maxConcurrentOperationCount = ProcessInfo.processInfo.activeProcessorCount + return q + }() + } + """, + excludeFromDocumentation: true), + + // previously passed, now error + Example("private ↓func h1(_ text: String) -> String { \"# \\(text)\" }"), + Example("↓func firstConfigurationFileInParentDirectories() -> Path? {}"), + Example( + """ + ↓func bodyLineCountIgnoringCommentsAndWhitespace( + leftBraceLine: Int, rightBraceLine: Int + ) -> Int { 0 } + """), + Example( + """ + ↓func hasAccessibilityElementChildrenIgnoreModifier(in file: SwiftLintFile) -> Bool { false } + """), + Example("↓func testLineAndCharacterForByteOffset_forContentsContainingMultibyteCharacters() {}") ] } diff --git a/Tests/SwiftLintFrameworkTests/IdentifierNameRuleTests.swift b/Tests/SwiftLintFrameworkTests/IdentifierNameRuleTests.swift index 492435326f..6f085a445c 100644 --- a/Tests/SwiftLintFrameworkTests/IdentifierNameRuleTests.swift +++ b/Tests/SwiftLintFrameworkTests/IdentifierNameRuleTests.swift @@ -3,18 +3,20 @@ class IdentifierNameRuleTests: SwiftLintTestCase { func testIdentifierNameWithExcluded() { let baseDescription = IdentifierNameRule.description - let nonTriggeringExamples = baseDescription.nonTriggeringExamples + [ - Example("let Apple = 0"), - Example("let some_apple = 0"), - Example("let Test123 = 0") - ] - let triggeringExamples = baseDescription.triggeringExamples + [ - Example("let ap_ple = 0"), - Example("let AppleJuice = 0") - ] + let nonTriggeringExamples = baseDescription + .nonTriggeringExamples + [ + Example("let Apple = 0"), + Example("let some_apple = 0"), + Example("let Test123 = 0") + ] + let triggeringExamples = baseDescription + .triggeringExamples + [ + Example("↓let ap_ple = 0"), + Example("↓let AppleJuice = 0") + ] let description = baseDescription.with(nonTriggeringExamples: nonTriggeringExamples, triggeringExamples: triggeringExamples) - verifyRule(description, ruleConfiguration: ["excluded": ["Apple", "some.*", ".*\\d+.*"]]) + verifyRule(description, ruleConfiguration: ["excluded": ["Apple", "some.*", "T\\w*\\d+.*"]]) } func testIdentifierNameWithAllowedSymbols() { @@ -56,7 +58,7 @@ class IdentifierNameRuleTests: SwiftLintTestCase { let description = baseDescription.with(nonTriggeringExamples: nonTriggeringExamples) .with(triggeringExamples: triggeringExamples) - verifyRule(description, ruleConfiguration: ["validates_start_with_lowercase": false]) + verifyRule(description, ruleConfiguration: ["validates_start_with_lowercase": "off"]) } func testStartsWithLowercaseCheck() { @@ -75,7 +77,7 @@ class IdentifierNameRuleTests: SwiftLintTestCase { IdentifierNameRule.description .with(triggeringExamples: triggeringExamples) .with(nonTriggeringExamples: nonTriggeringExamples), - ruleConfiguration: ["validates_start_with_lowercase": true] + ruleConfiguration: ["validates_start_with_lowercase": "error"] ) verifyRule(