From d756541204afa2f497d9085caf21d5662dd5c971 Mon Sep 17 00:00:00 2001 From: Paul Taykalo Date: Wed, 23 Mar 2022 18:30:43 +0200 Subject: [PATCH] Rewrite SyntacticSugarRule using SwiftSyntax (#3866) Rewrite SyntacticSugarRule with SwiftSyntax --- CHANGELOG.md | 3 + .../Extensions/StringView+SwiftSyntax.swift | 25 + .../Rules/Idiomatic/SyntacticSugarRule.swift | 450 ++++++++++++++---- 3 files changed, 373 insertions(+), 105 deletions(-) create mode 100644 Source/SwiftLintFramework/Extensions/StringView+SwiftSyntax.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 503c538b18..216d9ef786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,6 +126,9 @@ #### Experimental * None. +* Fix incorrect autocorrection in `syntactic_sugar` rule + [Paul Taykalo](https://github.com/PaulTaykalo) + [#3866](https://github.com/realm/SwiftLint/issues/3866) #### Enhancements diff --git a/Source/SwiftLintFramework/Extensions/StringView+SwiftSyntax.swift b/Source/SwiftLintFramework/Extensions/StringView+SwiftSyntax.swift new file mode 100644 index 0000000000..35e7fa5ddd --- /dev/null +++ b/Source/SwiftLintFramework/Extensions/StringView+SwiftSyntax.swift @@ -0,0 +1,25 @@ +import Foundation +import SourceKittenFramework +import SwiftSyntax + +extension StringView { + /// Converts two absolute positions from SwiftSyntax to valid NSRange if possible + /// - Parameters: + /// - start: starting poisiition + /// - end: end position + /// - Returns: NSRange or nil in case of empty string + func NSRange(start: AbsolutePosition, end: AbsolutePosition) -> NSRange? { + precondition(end.utf8Offset >= start.utf8Offset, "End position should be beigger than start position") + return NSRange(start: start, length: end.utf8Offset - start.utf8Offset) + } + + /// Converts absolute position with length from SwiftSyntax to valid NSRange if possible + /// - Parameters: + /// - start: starting position + /// - length: length in bytes + /// - Returns: NSRange or nil in case of empty string + private func NSRange(start: AbsolutePosition, length: Int) -> NSRange? { + let byteRange = ByteRange(location: ByteCount(start.utf8Offset), length: ByteCount(length)) + return byteRangeToNSRange(byteRange) + } +} diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/SyntacticSugarRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/SyntacticSugarRule.swift index 7c9ed9b997..ff0862d6d1 100644 --- a/Source/SwiftLintFramework/Rules/Idiomatic/SyntacticSugarRule.swift +++ b/Source/SwiftLintFramework/Rules/Idiomatic/SyntacticSugarRule.swift @@ -1,14 +1,18 @@ +// swiftlint:disable file_length import Foundation import SourceKittenFramework +import SwiftSyntax -public struct SyntacticSugarRule: SubstitutionCorrectableRule, ConfigurationProviderRule, AutomaticTestableRule { - public var configuration = SeverityConfiguration(.warning) +private let warnSyntaxParserFailureOnceImpl: Void = { + queuedPrintError("The syntactic_sugar rule is disabled because the Swift Syntax tree could not be parsed") +}() - private var pattern: String { - let types = ["Optional", "ImplicitlyUnwrappedOptional", "Array", "Dictionary"] - let negativeLookBehind = "(?:(?" - } +private func warnSyntaxParserFailureOnce() { + _ = warnSyntaxParserFailureOnceImpl +} + +public struct SyntacticSugarRule: CorrectableRule, ConfigurationProviderRule, AutomaticTestableRule { + public var configuration = SeverityConfiguration(.warning) public init() {} @@ -37,161 +41,397 @@ public struct SyntacticSugarRule: SubstitutionCorrectableRule, ConfigurationProv Example("var currentIndex: Array.Index?"), Example("func x(a: [Int], b: Int) -> Array.Index"), Example("unsafeBitCast(nonOptionalT, to: Optional.self)"), + Example("unsafeBitCast(someType, to: Swift.Array.self)"), + Example("IndexingIterator>>.self"), + Example("let y = Optional.Type"), + Example("type is Optional.Type"), - Example("let x: Foo.Optional") + Example("let x: Foo.Optional"), + + Example("let x = case Optional.none = obj"), + Example("let a = Swift.Optional.none") ], triggeringExamples: [ Example("let x: ↓Array"), Example("let x: ↓Dictionary"), Example("let x: ↓Optional"), Example("let x: ↓ImplicitlyUnwrappedOptional"), + Example("let x: ↓Swift.Array"), + Example("func x(a: ↓Array, b: Int) -> [Int: Any]"), + Example("func x(a: ↓Swift.Array, b: Int) -> [Int: Any]"), + Example("func x(a: [Int], b: Int) -> ↓Dictionary"), - Example("func x(a: ↓Array, b: Int) -> ↓Dictionary"), + Example("let x = y as? ↓Array<[String: Any]>"), + Example("let x = Box>()"), + Example("func x() -> Box<↓Array>"), + Example("func x() -> ↓Dictionary?"), + + Example("typealias Document = ↓Dictionary"), + Example("func x(_ y: inout ↓Array)"), + Example("let x:↓Dictionary>"), + Example("func x() -> Any { return ↓Dictionary()}"), + Example("let x = ↓Array.array(of: object)"), - Example("let x: ↓Swift.Optional") + Example("let x = ↓Swift.Array.array(of: object)"), + + Example(""" + @_specialize(where S == ↓Array) + public init(_ elements: S) + """) ], corrections: [ Example("let x: Array"): Example("let x: [String]"), - Example("let x: Array< String >"): Example("let x: [ String ]"), + Example("let x: Array< String >"): Example("let x: [String]"), Example("let x: Dictionary"): Example("let x: [Int: String]"), - Example("let x: Dictionary"): Example("let x: [Int : String]"), Example("let x: Optional"): Example("let x: Int?"), Example("let x: Optional< Int >"): Example("let x: Int?"), Example("let x: ImplicitlyUnwrappedOptional"): Example("let x: Int!"), Example("let x: ImplicitlyUnwrappedOptional< Int >"): Example("let x: Int!"), - Example("func x(a: Array, b: Int) -> [Int: Any]"): Example("func x(a: [Int], b: Int) -> [Int: Any]"), - Example("func x(a: [Int], b: Int) -> Dictionary"): - Example("func x(a: [Int], b: Int) -> [Int: String]"), - Example("let x = Array.array(of: object)"): Example("let x = [String].array(of: object)"), - Example("let x: Swift.Optional"): Example("let x: String?") + + Example("let x: Dictionary"): Example("let x: [Int: String]"), + Example("let x: Swift.Optional"): Example("let x: String?"), + Example("let x:↓Dictionary>"): Example("let x:[String: [Int: Int]]"), + Example("let x:↓Dictionary<↓Dictionary, String>"): Example("let x:[[Int: Int]: String]"), + Example("let x:↓Dictionary<↓Dictionary<↓Dictionary, Int>, String>"): + Example("let x:[[[Int: Int]: Int]: String]"), + Example("let x:↓Array<↓Dictionary>"): Example("let x:[[Int: Int]]"), + Example("let x:↓Optional<↓Dictionary>"): Example("let x:[Int: Int]?"), + Example("let x:↓ImplicitlyUnwrappedOptional<↓Dictionary>"): Example("let x:[Int: Int]!") ] ) public func validate(file: SwiftLintFile) -> [StyleViolation] { - let contents = file.stringView - return violationResults(in: file).map { - let typeString = contents.substring(with: $0.range(at: 1)) + guard let tree = file.syntaxTree else { + warnSyntaxParserFailureOnce() + return [] + } + let visitor = SyntacticSugarRuleVisitor() + visitor.walk(tree) + + let allViolations = flattenViolations(visitor.violations) + return allViolations.map { violation in return StyleViolation(ruleDescription: Self.description, severity: configuration.severity, - location: Location(file: file, characterOffset: $0.range.location), - reason: message(for: typeString)) + location: Location(file: file, byteOffset: ByteCount(violation.position.utf8Offset)), + reason: message(for: violation.type)) } } - public func violationRanges(in file: SwiftLintFile) -> [NSRange] { - return violationResults(in: file).map { $0.range } + private func flattenViolations(_ violations: [SyntacticSugarRuleViolation]) -> [SyntacticSugarRuleViolation] { + return violations.flatMap { [$0] + flattenViolations($0.children) } } - public func substitution(for violationRange: NSRange, in file: SwiftLintFile) -> (NSRange, String)? { - let contents = file.stringView - let declaration = contents.substring(with: violationRange) - let originalRange = NSRange(location: 0, length: declaration.count) - var substitutionResult = declaration - guard - let typeRange = regex(pattern).firstMatch(in: declaration, options: [], range: originalRange)?.range(at: 1) - else { - return (violationRange, substitutionResult) + public func correct(file: SwiftLintFile) -> [Correction] { + guard let tree = file.syntaxTree else { + warnSyntaxParserFailureOnce() + return [] } + let visitor = SyntacticSugarRuleVisitor() + visitor.walk(tree) - let containerType = declaration.bridge().substring(with: typeRange) + var context = CorrectingContex(rule: self, file: file, contents: file.contents) + context.correctViolations(visitor.violations) - switch containerType { + file.write(context.contents) + + return context.corrections + } + + private func message(for originalType: String) -> String { + let typeString: String + let sugaredType: String + + switch originalType { case "Optional": - let genericType = declaration.bridge().substring(from: typeRange.upperBound) - .replacingOccurrences(of: " ", with: "") - .replacingOccurrences(of: "<", with: "") - .replacingOccurrences(of: ">", with: "") - substitutionResult = "\(genericType)?" + typeString = "Optional" + sugaredType = "Int?" case "ImplicitlyUnwrappedOptional": - let genericType = declaration.bridge().substring(from: typeRange.upperBound) - .replacingOccurrences(of: " ", with: "") - .replacingOccurrences(of: "<", with: "") - .replacingOccurrences(of: ">", with: "") - substitutionResult = "\(genericType)!" + typeString = "ImplicitlyUnwrappedOptional" + sugaredType = "Int!" case "Array": - substitutionResult = declaration.bridge().substring(from: typeRange.upperBound) - .replacingOccurrences(of: "<", with: "[") - .replacingOccurrences(of: ">", with: "]") + typeString = "Array" + sugaredType = "[Int]" case "Dictionary": - substitutionResult = declaration.bridge().substring(from: typeRange.upperBound) - .replacingOccurrences(of: "<", with: "[") - .replacingOccurrences(of: ">", with: "]") - .replacingOccurrences(of: ",", with: ":") + typeString = "Dictionary" + sugaredType = "[String: Int]" default: - break + return Self.description.description } - return (violationRange, substitutionResult) + return "Shorthand syntactic sugar should be used, i.e. \(sugaredType) instead of \(typeString)." } +} - private func violationResults(in file: SwiftLintFile) -> [NSTextCheckingResult] { - let excludingKinds = SyntaxKind.commentAndStringKinds - let contents = file.stringView - return regex(pattern).matches(in: contents).compactMap { result in - let range = result.range - guard let byteRange = contents.NSRangeToByteRange(start: range.location, length: range.length) else { - return nil - } +private struct SyntacticSugarRuleViolation { + struct Correction { + let typeStart: AbsolutePosition + let correction: CorrectionType - let kinds = file.syntaxMap.kinds(inByteRange: byteRange) - guard excludingKinds.isDisjoint(with: kinds), - isValidViolation(range: range, file: file) else { - return nil - } + let leftStart: AbsolutePosition + let leftEnd: AbsolutePosition + + let rightStart: AbsolutePosition + let rightEnd: AbsolutePosition + } + enum CorrectionType { + case optional + case dictionary(commaStart: AbsolutePosition, commaEnd: AbsolutePosition) + case array + case implicitlyUnwrappedOptional + } + + let position: AbsolutePosition + let type: String + + let correction: Correction - return result + var children: [SyntacticSugarRuleViolation] = [] +} + +private final class SyntacticSugarRuleVisitor: SyntaxAnyVisitor { + private let types = ["Optional", "ImplicitlyUnwrappedOptional", "Array", "Dictionary"] + + var violations: [SyntacticSugarRuleViolation] = [] + + override func visitPost(_ node: TypeAnnotationSyntax) { + // let x: ↓Swift.Optional + // let x: ↓Optional + if let violation = violation(in: node.type) { + violations.append(violation) } } - private func isValidViolation(range: NSRange, file: SwiftLintFile) -> Bool { - let contents = file.stringView + override func visitPost(_ node: FunctionParameterSyntax) { + // func x(a: ↓Array, b: Int) -> [Int: Any] + if let violation = violation(in: node.type) { + violations.append(violation) + } + } - // avoid triggering when referring to an associatedtype - let start = range.location + range.length - let restOfFileRange = NSRange(location: start, length: contents.nsString.length - start) - if regex("\\s*\\.").firstMatch(in: file.contents, options: [], - range: restOfFileRange)?.range.location == start { - guard let byteOffset = contents.NSRangeToByteRange(start: range.location, - length: range.length)?.location else { - return false - } + override func visitPost(_ node: ReturnClauseSyntax) { + // func x(a: [Int], b: Int) -> ↓Dictionary + if let violation = violation(in: node.returnType) { + violations.append(violation) + } + } + + override func visitPost(_ node: AsExprSyntax) { + // json["recommendations"] as? ↓Array<[String: Any]> + if let violation = violation(in: node.typeName) { + violations.append(violation) + } + } + + override func visitPost(_ node: TypeInitializerClauseSyntax) { + // typealias Document = ↓Dictionary + if let violation = violation(in: node.value) { + violations.append(violation) + } + } + + override func visitPost(_ node: AttributedTypeSyntax) { + // func x(_ y: inout ↓Array) + if let violation = violation(in: node.baseType) { + violations.append(violation) + } + } + + override func visitPost(_ node: SameTypeRequirementSyntax) { + // @_specialize(where S == ↓Array) + if let violation = violation(in: node.leftTypeIdentifier) { + violations.append(violation) + } + if let violation = violation(in: node.rightTypeIdentifier) { + violations.append(violation) + } + } + + override func visitPost(_ node: SpecializeExprSyntax) { + // let x = ↓Array.array(of: object) + let tokens = Array(node.expression.tokens) + + // Remove Swift. module prefix if needed + var tokensText = tokens.map { $0.text }.joined() + if tokensText.starts(with: "Swift.") { + tokensText.removeFirst("Swift.".count) + } + + // Skip checks for 'self' or \T Dictionary.self + if let parent = node.parent?.as(MemberAccessExprSyntax.self), + let lastToken = Array(parent.tokens).last?.tokenKind, + lastToken == .selfKeyword || lastToken == .identifier("Type") || lastToken == .identifier("none") { + return + } - let kinds = file.structureDictionary.structures(forByteOffset: byteOffset).compactMap { $0.expressionKind } - guard kinds.contains(.call) else { - return false + if types.contains(tokensText) { + if let violation = violation(from: node) { + violations.append(violation) } + return + } - if let (range, kinds) = file.match(pattern: "\\s*\\.(?:self|Type)", range: restOfFileRange).first, - range.location == start, kinds == [.keyword] || kinds == [.identifier] { - return false + // If there's no type let's check all inner generics like in case of Box> + node.genericArgumentClause.arguments + .compactMap { violation(in: $0.argumentType) } + .first + .map { violations.append($0) } + } + + private func violation(in typeSyntax: TypeSyntax?) -> SyntacticSugarRuleViolation? { + if let optionalType = typeSyntax?.as(OptionalTypeSyntax.self) { + return violation(in: optionalType.wrappedType) + } + + if let simpleType = typeSyntax?.as(SimpleTypeIdentifierSyntax.self) { + if types.contains(simpleType.name.text) { + return violation(from: simpleType) } + + // If there's no type let's check all inner generics like in case of Box> + guard let genericArguments = simpleType.genericArgumentClause else { return nil } + let innerTypes = genericArguments.arguments.compactMap { violation(in: $0.argumentType) } + return innerTypes.first } - return true + // Base class is "Swift" for cases like "Swift.Array" + if let memberType = typeSyntax?.as(MemberTypeIdentifierSyntax.self), + let baseType = memberType.baseType.as(SimpleTypeIdentifierSyntax.self), + baseType.name.text == "Swift" { + guard types.contains(memberType.name.text) else { return nil } + return violation(from: memberType) + } + return nil } - private func message(for originalType: String) -> String { - let typeString: String - let sugaredType: String + private func violation(from node: SyntaxProtocol & SyntaxWithGenericClause) -> SyntacticSugarRuleViolation? { + guard + let generic = node.genericArguments, + let firstGenericType = generic.arguments.first, + let lastGenericType = generic.arguments.last, + var typeName = node.typeName + else { return nil } - switch originalType { - case "Optional": - typeString = "Optional" - sugaredType = "Int?" - case "ImplicitlyUnwrappedOptional": - typeString = "ImplicitlyUnwrappedOptional" - sugaredType = "Int!" - case "Array": - typeString = "Array" - sugaredType = "[Int]" - case "Dictionary": - typeString = "Dictionary" - sugaredType = "[String: Int]" - default: - return Self.description.description + if typeName.hasPrefix("Swift.") { + typeName.removeFirst("Swift.".count) } - return "Shorthand syntactic sugar should be used, i.e. \(sugaredType) instead of \(typeString)." + var type = SyntacticSugarRuleViolation.CorrectionType.array + if typeName.isEqualTo("Dictionary") { + guard let comma = firstGenericType.trailingComma else { return nil } + let lastArgumentEnd = firstGenericType.argumentType.endPositionBeforeTrailingTrivia + type = .dictionary(commaStart: lastArgumentEnd, commaEnd: comma.endPosition) + } + if typeName.isEqualTo("Optional") { + type = .optional + } + if typeName.isEqualTo("ImplicitlyUnwrappedOptional") { + type = .implicitlyUnwrappedOptional + } + + let firstInnerViolation = violation(in: firstGenericType.argumentType) + let secondInnerViolation = generic.arguments.count > 1 ? violation(in: lastGenericType.argumentType) : nil + + return SyntacticSugarRuleViolation( + position: node.positionAfterSkippingLeadingTrivia, + type: typeName, + correction: .init(typeStart: node.position, + correction: type, + leftStart: generic.leftAngleBracket.position, + leftEnd: generic.leftAngleBracket.endPosition, + rightStart: lastGenericType.endPositionBeforeTrailingTrivia, + rightEnd: generic.rightAngleBracket.endPositionBeforeTrailingTrivia), + children: [ firstInnerViolation, secondInnerViolation].compactMap { $0 } + ) + } +} + +// MARK: - Private + +private struct CorrectingContex { + let rule: Rule + let file: SwiftLintFile + var contents: String + var corrections: [Correction] = [] + + mutating func correctViolations(_ violations: [SyntacticSugarRuleViolation]) { + let sortedVolations = violations.sorted(by: { $0.correction.typeStart > $1.correction.typeStart }) + sortedVolations.forEach { violation in + correctViolation(violation) + } + } + + mutating func correctViolation(_ violation: SyntacticSugarRuleViolation) { + let stringView = file.stringView + let correction = violation.correction + + guard let violationNSRange = stringView.NSRange(start: correction.leftStart, end: correction.rightEnd), + file.ruleEnabled(violatingRange: violationNSRange, for: rule) != nil else { return } + + guard let rightRange = stringView.NSRange(start: correction.rightStart, end: correction.rightEnd), + let leftRange = stringView.NSRange(start: correction.typeStart, end: correction.leftEnd) else { + return + } + + switch correction.correction { + case .array: + replaceCharacters(in: rightRange, with: "]") + correctViolations(violation.children) + replaceCharacters(in: leftRange, with: "[") + + case let .dictionary(commaStart, commaEnd): + + replaceCharacters(in: rightRange, with: "]") + guard let commaRange = stringView.NSRange(start: commaStart, end: commaEnd) else { return } + + let violationsAfterComma = violation.children.filter { $0.position.utf8Offset > commaStart.utf8Offset } + correctViolations(violationsAfterComma) + + replaceCharacters(in: commaRange, with: ": ") + + let violationsBeforeComma = violation.children.filter { $0.position.utf8Offset < commaStart.utf8Offset } + correctViolations(violationsBeforeComma) + replaceCharacters(in: leftRange, with: "[") + + case .optional: + replaceCharacters(in: rightRange, with: "?") + correctViolations(violation.children) + replaceCharacters(in: leftRange, with: "") + + case .implicitlyUnwrappedOptional: + replaceCharacters(in: rightRange, with: "!") + correctViolations(violation.children) + replaceCharacters(in: leftRange, with: "") + } + + let location = Location(file: file, byteOffset: ByteCount(correction.typeStart.utf8Offset)) + corrections.append(Correction(ruleDescription: type(of: rule).description, location: location)) + } + + private mutating func replaceCharacters(in range: NSRange, with replacement: String) { + contents = contents.bridge().replacingCharacters(in: range, with: replacement) + } +} + +private protocol SyntaxWithGenericClause { + var typeName: String? { get } + var genericArguments: GenericArgumentClauseSyntax? { get } +} + +extension MemberTypeIdentifierSyntax: SyntaxWithGenericClause { + var typeName: String? { name.text } + var genericArguments: GenericArgumentClauseSyntax? { genericArgumentClause } +} + +extension SimpleTypeIdentifierSyntax: SyntaxWithGenericClause { + var typeName: String? { name.text } + var genericArguments: GenericArgumentClauseSyntax? { genericArgumentClause } +} + +extension SpecializeExprSyntax: SyntaxWithGenericClause { + var typeName: String? { + expression.as(IdentifierExprSyntax.self)?.firstToken?.text ?? + expression.as(MemberAccessExprSyntax.self)?.name.text } + var genericArguments: GenericArgumentClauseSyntax? { genericArgumentClause } }