Skip to content

Commit

Permalink
Integrate Swift Syntax in the force_cast rule (#3867)
Browse files Browse the repository at this point in the history
We've tried adding Swift Syntax support to SwiftLint in the past but had to turn it off in #3107 due to distribution and portability issues.

With https://github.com/keith/StaticInternalSwiftSyntaxParser it should be possible to avoid those issues by statically linking the internal Swift syntax parser so it's available no matter where users have Xcode installed on their computer.

By removing all calls to SourceKit (collecting comment commands + checking the current Swift version), there's a really significant performance improvement.

| Framework | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| SourceKit | 517.8 ± 8.3 | 505.5 | 531.1 | 6.59 ± 0.43 |
| SwiftSyntax | 78.6 ± 5.0 | 72.6 | 92.1 | 1.00 |

In practice, the SourceKit overhead will continue being there for as long as any rule being run is still looking up the SwiftLint syntax map though.
  • Loading branch information
jpsim committed Mar 9, 2022
1 parent 51e06d2 commit 290598d
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 21 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@

#### Experimental

* None.
* The `force_cast` rule has been updated to use SwiftSyntax to find violations
instead of SourceKit. Please report any problems you encounter by opening a
GitHub issue. If this is successful, more rules may use Swift Syntax in the
future.
[JP Simard](https://github.com/jpsim)

#### Enhancements

Expand Down
33 changes: 18 additions & 15 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,13 @@ XCODEFLAGS=-scheme 'swiftlint' \
DSTROOT=$(TEMPORARY_FOLDER) \
OTHER_LDFLAGS=-Wl,-headerpad_max_install_names

SWIFT_BUILD_FLAGS=--configuration release
SWIFT_BUILD_FLAGS=--configuration release -Xlinker -dead_strip
UNAME=$(shell uname)
ifeq ($(UNAME), Darwin)
USE_SWIFT_STATIC_STDLIB:=$(shell test -d $$(dirname $$(xcrun --find swift))/../lib/swift_static/macosx && echo yes)
ifeq ($(USE_SWIFT_STATIC_STDLIB), yes)
SWIFT_BUILD_FLAGS+= -Xswiftc -static-stdlib
endif
# SwiftPM 5.3 uses the `XCBuild.framework` to generate a universal binary when it receives multiple `--arch` options.
SWIFT_BUILD_ARCHS:= arm64 x86_64
# If SwiftPM supports `--arch $(1)` and `swiftc` succeeds in building with `-target $(1)-apple-macos10.9`, then produce `--arch $(1)`.
ARCH_OPTION=$(shell swift build --show-bin-path --arch $(1) &>/dev/null && echo ''|swiftc -target $(1)-apple-macos10.9 - -o /dev/null &>/dev/null && echo "--arch" $(1))
SWIFT_BUILD_FLAGS+=$(foreach arch,$(SWIFT_BUILD_ARCHS),$(call ARCH_OPTION,$(arch)))
endif # Darwin

SWIFTLINT_EXECUTABLE=$(shell swift build $(SWIFT_BUILD_FLAGS) --show-bin-path)/swiftlint
SWIFTLINT_EXECUTABLE_X86=$(shell swift build $(SWIFT_BUILD_FLAGS) --arch x86_64 --show-bin-path)/swiftlint
SWIFTLINT_EXECUTABLE_ARM64=$(shell swift build $(SWIFT_BUILD_FLAGS) --arch arm64 --show-bin-path)/swiftlint
SWIFTLINT_EXECUTABLE_PARENT=.build/universal
SWIFTLINT_EXECUTABLE=$(SWIFTLINT_EXECUTABLE_PARENT)/swiftlint

TSAN_LIB=$(subst bin/swift,lib/swift/clang/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib,$(shell xcrun --find swift))
TSAN_SWIFT_BUILD_FLAGS=-Xswiftc -sanitize=thread
Expand Down Expand Up @@ -74,8 +66,19 @@ clean:
clean_xcode:
$(BUILD_TOOL) $(XCODEFLAGS) -configuration Test clean

build:
swift build $(SWIFT_BUILD_FLAGS)
build_x86_64:
swift build $(SWIFT_BUILD_FLAGS) --arch x86_64

build_arm64:
swift build $(SWIFT_BUILD_FLAGS) --arch arm64

build: clean build_x86_64 build_arm64
# Need to build for each arch independently to work around https://bugs.swift.org/browse/SR-15802
mkdir -p $(SWIFTLINT_EXECUTABLE_PARENT)
lipo -create -output \
"$(SWIFTLINT_EXECUTABLE)" \
"$(SWIFTLINT_EXECUTABLE_X86)" \
"$(SWIFTLINT_EXECUTABLE_ARM64)"

build_with_disable_sandbox:
swift build --disable-sandbox $(SWIFT_BUILD_FLAGS)
Expand Down
9 changes: 9 additions & 0 deletions Package.resolved
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@
"version": "1.0.3"
}
},
{
"package": "SwiftSyntax",
"repositoryURL": "https://github.com/apple/swift-syntax.git",
"state": {
"branch": null,
"revision": "cf40be70deaf4ce7d44eb1a7e14299c391e2363f",
"version": null
}
},
{
"package": "SwiftyTextTable",
"repositoryURL": "https://github.com/scottrhoyt/SwiftyTextTable.git",
Expand Down
21 changes: 18 additions & 3 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ private let addCryptoSwift = false
private let addCryptoSwift = true
#endif

#if os(macOS)
private let staticSwiftSyntax = true
#else
private let staticSwiftSyntax = false
#endif

let package = Package(
name: "SwiftLint",
platforms: [.macOS(.v10_12)],
Expand All @@ -16,6 +22,8 @@ let package = Package(
],
dependencies: [
.package(name: "swift-argument-parser", url: "https://github.com/apple/swift-argument-parser.git", from: "1.0.1"),
.package(name: "SwiftSyntax", url: "https://github.com/apple/swift-syntax.git",
.revision("cf40be70deaf4ce7d44eb1a7e14299c391e2363f")),
.package(url: "https://github.com/jpsim/SourceKitten.git", from: "0.31.1"),
.package(url: "https://github.com/jpsim/Yams.git", from: "4.0.2"),
.package(url: "https://github.com/scottrhoyt/SwiftyTextTable.git", from: "0.9.0"),
Expand All @@ -33,8 +41,11 @@ let package = Package(
name: "SwiftLintFramework",
dependencies: [
.product(name: "SourceKittenFramework", package: "SourceKitten"),
"SwiftSyntax",
"Yams",
] + (addCryptoSwift ? ["CryptoSwift"] : [])
]
+ (addCryptoSwift ? ["CryptoSwift"] : [])
+ (staticSwiftSyntax ? ["lib_InternalSwiftSyntaxParser"] : [])
),
.testTarget(
name: "SwiftLintFrameworkTests",
Expand All @@ -44,6 +55,10 @@ let package = Package(
exclude: [
"Resources",
]
)
]
),
] + (staticSwiftSyntax ? [.binaryTarget(
name: "lib_InternalSwiftSyntaxParser",
url: "https://github.com/keith/StaticInternalSwiftSyntaxParser/releases/download/5.5.2/lib_InternalSwiftSyntaxParser.xcframework.zip",
checksum: "96bbc9ab4679953eac9ee46778b498cb559b8a7d9ecc658e54d6679acfbb34b8"
)] : [])
)
29 changes: 27 additions & 2 deletions Source/SwiftLintFramework/Rules/Idiomatic/ForceCastRule.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
import SourceKittenFramework
import SwiftSyntax

private let warnSyntaxParserFailureOnceImpl: Void = {
queuedPrintError("The force_cast rule is disabled because the Swift Syntax tree could not be parsed")
}()

private func warnSyntaxParserFailureOnce() {
_ = warnSyntaxParserFailureOnceImpl
}

public struct ForceCastRule: ConfigurationProviderRule, AutomaticTestableRule {
public var configuration = SeverityConfiguration(.error)
Expand All @@ -17,10 +26,26 @@ public struct ForceCastRule: ConfigurationProviderRule, AutomaticTestableRule {
)

public func validate(file: SwiftLintFile) -> [StyleViolation] {
return file.match(pattern: "as!", with: [.keyword]).map {
guard let tree = try? SyntaxParser.parse(source: file.contents) else {
warnSyntaxParserFailureOnce()
return []
}
let visitor = ForceCastRuleVisitor()
visitor.walk(tree)
return visitor.positions.map { position in
StyleViolation(ruleDescription: Self.description,
severity: configuration.severity,
location: Location(file: file, characterOffset: $0.location))
location: Location(file: file, byteOffset: ByteCount(position.utf8Offset)))
}
}
}

private final class ForceCastRuleVisitor: SyntaxVisitor {
var positions: [AbsolutePosition] = []

override func visitPost(_ node: AsExprSyntax) {
if node.questionOrExclamationMark?.tokenKind == .exclamationMark {
positions.append(node.asTok.positionAfterSkippingLeadingTrivia)
}
}
}

0 comments on commit 290598d

Please sign in to comment.