Add ability for SwiftLint to lint files with full type-checked AST awareness #2343
Conversation
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #2343 +/- ##
==========================================
- Coverage 91.89% 91.15% -0.74%
==========================================
Files 304 307 +3
Lines 15254 15503 +249
==========================================
+ Hits 14017 14132 +115
- Misses 1237 1371 +134
Continue to review full report at Codecov.
|
jpsim
changed the title
|
I've added support for testing compiler argument rules and added changelog entries, so this is no longer WIP if anyone's interested in giving this a proper review. |
| public protocol CompilerArgumentRule: Rule {} | ||
|
|
||
| public extension CompilerArgumentRule { | ||
| func validate(file: File) -> [StyleViolation] { |
There was a problem hiding this comment.
These default extensions don't seem like they would ever work for this type of rule? Are they required or you could omit them and require implementors to add them?
| } | ||
| """ | ||
| ], | ||
| requiresFileOnDisk: true |
There was a problem hiding this comment.
This property isn't very clear to me, which files does it require?
There was a problem hiding this comment.
Requires that Files that are passed in have a valid path on disk, unlike other uses of File, some of which are initialized with File(contents:) and have no path on disk.
There was a problem hiding this comment.
Ah got it. I wonder to avoid the force unwrapping here if it would be worth a different File type. But maybe that would be an unreasonably large change.
There was a problem hiding this comment.
Definitely a large change.
|
|
||
| private func violationRanges(in file: File, compilerArguments: [String]) -> [NSRange] { | ||
| guard !compilerArguments.isEmpty else { | ||
| return [] |
There was a problem hiding this comment.
Should this be an error of some kind instead?
There was a problem hiding this comment.
I'll log an error here.
| } | ||
|
|
||
| var compileCommand: [String]? | ||
| compilerLogs.enumerateLines { line, stop in |
There was a problem hiding this comment.
Maybe a future optimization, but I wonder if it would be worth creating a dictionary of file path to commands up front that you could more easily access. I have to assume that doing this over and over is expensive with how large these files can be?
There was a problem hiding this comment.
This will for sure need to be done before adding more compiler argument rules.
|
I spoke with @keith and @soffes about this earlier today and decided to take the following next steps:
|
|
Continued in #2379. I opted to open a new PR to keep this one for historical purposes, given I rewrote large parts of it. |
Note: very experimental and still a work in progress
This gives SwiftLint the ability to lint files with the assistance of the fully type-checked AST. This can enable much more powerful rules, such as linting for:
internalthat could beprivate)view.tintColor = UIColor.blackvsview.tintColor = .black)self.It requires passing a file with the full output from a clean & successful
xcodebuildinvocation (no incremental builds):$ swiftlint [lint|autocorrect] --no-cache --compiler-log-path /path/to/xcodebuild.logI've written a first rule to get started, which enforces the use of explicit references to
self.when accessing instance members.You can test this out on SwiftLint itself (which doesn't abide by this rule) by checking out this branch and running the following:
This will add explicit references to
self.to all files linted.The rules are expensive and awkward to run, which is why I've punted on building this for a while, but it's clear to me that it's very useful nonetheless, so hopefully we can improve this on all fronts from this starting point.
I'd like to get some basic tests running for this and clean up a few things before merging, which is why this is marked as WIP.