Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability for SwiftLint to lint files with full type-checked AST awareness #2379

Merged
merged 20 commits into from
Sep 2, 2018

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Sep 1, 2018

Note: very experimental, continued from #2343

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:

  • dead code
  • unused/duplicate imports
  • over-exposed declarations (e.g. internal that could be private)
  • superfluous type annotations (e.g. view.tintColor = UIColor.black vs view.tintColor = .black)
  • use of explicit or implicit self.

It requires passing a file with the full output from a clean & successful xcodebuild invocation (no incremental builds):

$ swiftlint analyze [--autocorrect] --compiler-log-path /path/to/xcodebuild.log

I'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:

$ # Ensure derived data for SwiftLint is clean
$ xcodebuild -workspace SwiftLint.xcworkspace -scheme swiftlint > xcodebuild.log
$ echo "included:
  - Source
analyzer_rules:
  - explicit_self" > .swiftlint.yml
$ swift build -c release
$ .build/release/swiftlint analyze --autocorrect --compiler-log-path xcodebuild.log

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.

@SwiftLintBot
Copy link

SwiftLintBot commented Sep 1, 2018

1 Warning
⚠️ Big PR
12 Messages
📖 Linting Aerial with this PR took 0.43s vs 0.37s on master (16% slower)
📖 Linting Alamofire with this PR took 3.8s vs 3.11s on master (22% slower)
📖 Linting Firefox with this PR took 14.56s vs 11.77s on master (23% slower)
📖 Linting Kickstarter with this PR took 21.66s vs 18.02s on master (20% slower)
📖 Linting Moya with this PR took 2.16s vs 1.94s on master (11% slower)
📖 Linting Nimble with this PR took 2.14s vs 1.74s on master (22% slower)
📖 Linting Quick with this PR took 0.57s vs 0.52s on master (9% slower)
📖 Linting Realm with this PR took 3.74s vs 3.36s on master (11% slower)
📖 Linting SourceKitten with this PR took 1.06s vs 1.03s on master (2% slower)
📖 Linting Sourcery with this PR took 4.68s vs 4.54s on master (3% slower)
📖 Linting Swift with this PR took 29.81s vs 28.78s on master (3% slower)
📖 Linting WordPress with this PR took 16.84s vs 17.17s on master (1% faster)

Generated by 🚫 Danger

Rules.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@marcelofabri
Copy link
Collaborator

Very excited to see this coming to life 🎉

README.md Show resolved Hide resolved
@jpsim jpsim merged commit 2bcea4b into master Sep 2, 2018
@jpsim jpsim deleted the analyze branch September 2, 2018 07:13
@norio-nomura
Copy link
Collaborator

I opened jpsim/SourceKitten#555 that parsing compiler arguments from llbuild manifest used in New Build System by Xcode. That can avoid rebuilding to ensure compiler arguments from xcodebuild.log.
I think the same method can be used with SwiftLint's type-checked AST linting.

@jpsim
Copy link
Collaborator Author

jpsim commented Sep 2, 2018

Nice, thanks! I was going to do something like that next. We should expose this kind of stuff in SourceKitten IMO so that downstream tools like SwiftLint don't have to worry about it.

Although I'm not 100% sure that the compiler arguments without building yields the same result as when the targets are fully built. I think some of the SourceKit requests might return different data, like USRs not being fully resolved, among other things. This will need further investigation.

@jpsim
Copy link
Collaborator Author

jpsim commented Sep 2, 2018

Although I'm not 100% sure that the compiler arguments without building yields the same result as when the targets are fully built.

Ah, I see now that your PR still builds the targets fully, what I was talking about was just getting the compiler arguments and not building at all. I think that works for some SourceKit requests but not all.

@norio-nomura
Copy link
Collaborator

Oh, if the correct compiler arguments were passed to sourcekit, I did not think that rebuilding are unnecessary. And it seems to work without rebuilding.

@jpsim jpsim mentioned this pull request Sep 10, 2018
@SRozhina
Copy link

SRozhina commented Oct 25, 2018

Could you please explain how to use this feature? Now I have 0.27.0 version and there is no "swiftlint analyze" command

@jpsim
Copy link
Collaborator Author

jpsim commented Nov 17, 2018

This feature was released in 0.28.0.

sjavora pushed a commit to sjavora/SwiftLint that referenced this pull request Mar 9, 2019
…areness (realm#2379)

* Add LintableFilesVisitor

* Move LintCommand logic into LintOrAnalyzeCommand

to prepare for the upcoming analyze command

* Add AnalyzeCommand (not fully implemented yet in SwiftLintFramework)

* Add analyzerRules configuration member

* Add AnalyzerRule protocol

* Pass compiler arguments to validate/correct

* Add requiresFileOnDisk member to RuleDescription

This will be used by AnalyzerRules because they need a file on disk
to pass in the compiler arguments to SourceKit.

* Exclusively run AnalyzerRules when the Linter has compiler arguments

* Enable testing AnalyzerRules in TestHelpers

* Add ExplicitSelfRule

* Update documentation

* Fix `analyze --autocorrect`

* Improve performance of CompilerArgumentsExtractor

* Fix lint command actually running analyze

* Move File operations in TestHelpers into a private extension

* Add analyzer column to rules command and markdown documentation

* Use a Set literal

* Make AnalyzerRule inherit from OptInRule

* Mention analyzer_rules in readme

* Mention that analyzer rules are slow
jpsim added a commit that referenced this pull request Jul 28, 2019
jpsim added a commit that referenced this pull request Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants