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

Skip correcting files with parser diagnostics #3349

Merged
merged 3 commits into from
Sep 17, 2020

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Sep 17, 2020

We should avoid correcting files if they have parser errors or warnings, since SwiftLint isn't designed to handle all possibilities of invalid Swift code.

Generally, it's still useful for SwiftLint to lint Swift files that don't parse 100% cleanly as a feedback mechanism, but we shouldn't attempt to correct files with violations that don't parse cleanly, because that can mangle or delete code in unintended ways.

This PR also fixes many rule examples with parser diagnostics, but there are still over 500 rule examples with parser diagnostics, I don't have time to fix all of them, but we probably should do that eventually. I filed #3348 to track that, which is a good starter task if someone's interested in contributing to SwiftLint.

Also fix many rule examples with parser diagnostics.
@jpsim jpsim marked this pull request as ready for review September 17, 2020 20:35
@realm realm deleted a comment from SwiftLintBot Sep 17, 2020
@jpsim jpsim requested a review from keith September 17, 2020 20:42
@SwiftLintBot
Copy link

12 Messages
📖 Linting Aerial with this PR took 2.04s vs 1.88s on master (8% slower)
📖 Linting Alamofire with this PR took 2.67s vs 2.67s on master (0% slower)
📖 Linting Firefox with this PR took 9.07s vs 9.09s on master (0% faster)
📖 Linting Kickstarter with this PR took 15.18s vs 15.17s on master (0% slower)
📖 Linting Moya with this PR took 1.31s vs 1.33s on master (1% faster)
📖 Linting Nimble with this PR took 1.36s vs 1.34s on master (1% slower)
📖 Linting Quick with this PR took 0.6s vs 0.65s on master (7% faster)
📖 Linting Realm with this PR took 2.66s vs 2.67s on master (0% faster)
📖 Linting SourceKitten with this PR took 1.04s vs 1.02s on master (1% slower)
📖 Linting Sourcery with this PR took 7.13s vs 7.06s on master (0% slower)
📖 Linting Swift with this PR took 10.83s vs 10.82s on master (0% slower)
📖 Linting WordPress with this PR took 16.84s vs 16.9s on master (0% faster)

Generated by 🚫 Danger

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2020

Codecov Report

Merging #3349 into master will decrease coverage by 0.04%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
- Coverage   90.55%   90.51%   -0.05%     
==========================================
  Files         416      417       +1     
  Lines       20410    20432      +22     
==========================================
+ Hits        18483    18494      +11     
- Misses       1927     1938      +11     
Impacted Files Coverage Δ
...mework/Rules/Idiomatic/TrailingSemicolonRule.swift 100.00% <ø> (ø)
...ntFramework/Rules/Lint/AnyObjectProtocolRule.swift 100.00% <ø> (ø)
...mework/Rules/Lint/UnusedClosureParameterRule.swift 100.00% <ø> (ø)
...ftLintFramework/Rules/Lint/YodaConditionRule.swift 100.00% <ø> (ø)
...les/Style/ComputedAccessorsOrderRuleExamples.swift 100.00% <ø> (ø)
...ework/Rules/Style/ImplicitGetterRuleExamples.swift 98.83% <ø> (ø)
...es/Style/VerticalWhitespaceClosingBracesRule.swift 94.73% <ø> (ø)
...es/Style/VerticalWhitespaceOpeningBracesRule.swift 94.87% <ø> (ø)
...wiftLintFramework/Rules/Style/VoidReturnRule.swift 100.00% <ø> (ø)
...tFrameworkTests/AutomaticRuleTests.generated.swift 100.00% <ø> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b93b3e...0548f5a. Read the comment docs.

Copy link
Collaborator

@keith keith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@jpsim jpsim merged commit 6d2e8cf into master Sep 17, 2020
@jpsim jpsim deleted the skip-correcting-files-with-parser-diagnostics branch September 17, 2020 22:14
optionalendeavors added a commit to optionalendeavors/SwiftLint that referenced this pull request Oct 4, 2020
* master-upstream: (98 commits)
  Fix some false positives in rule `explicit_self` (realm#3368)
  Update SourceKitten to 0.30.1 (realm#3367)
  Fix issues with analyzer rules, Xcode 12 & SwiftUI (realm#3366)
  Add empty changelog section
  release 0.40.3
  Fix false positives for 'multiple_closures_with_trailing_closure' (realm#3353)
  [UnusedDeclarationRule] Work around SR-11985 (realm#3363)
  Revert "Fix finding the nested config when a single file path is passed (realm#3342)" (realm#3362)
  [CONTRIBUTING] Add building & running tips (realm#3360)
  Fix finding the nested config when a single file path is passed (realm#3342)
  Include Linux zip in list of GitHub release binaries (realm#3350)
  [UnusedDeclarationRule] Add more tests (realm#3359)
  Test CI with official Swift 5.3 release (realm#3356)
  Don't mark `@NSApplicationMain` or `@UIApplicationMain` classes as unused (realm#3355)
  [Fix] `UnusedCaptureListRule`: implicit self in @escaping closures (realm#3352)
  Skip correcting files with parser diagnostics (realm#3349)
  [SwiftLintFile] Remove lock in favor of UUID (realm#3347)
  [UnusedDeclarationRule] Speed up and detect more dead code (realm#3340)
  Add empty changelog section
  release 0.40.2
  ...

# Conflicts:
#	Source/swiftlint/Helpers/LintableFilesVisitor.swift
optionalendeavors added a commit to optionalendeavors/SwiftLint that referenced this pull request Oct 4, 2020
* master: (98 commits)
  Fix some false positives in rule `explicit_self` (realm#3368)
  Update SourceKitten to 0.30.1 (realm#3367)
  Fix issues with analyzer rules, Xcode 12 & SwiftUI (realm#3366)
  Add empty changelog section
  release 0.40.3
  Fix false positives for 'multiple_closures_with_trailing_closure' (realm#3353)
  [UnusedDeclarationRule] Work around SR-11985 (realm#3363)
  Revert "Fix finding the nested config when a single file path is passed (realm#3342)" (realm#3362)
  [CONTRIBUTING] Add building & running tips (realm#3360)
  Fix finding the nested config when a single file path is passed (realm#3342)
  Include Linux zip in list of GitHub release binaries (realm#3350)
  [UnusedDeclarationRule] Add more tests (realm#3359)
  Test CI with official Swift 5.3 release (realm#3356)
  Don't mark `@NSApplicationMain` or `@UIApplicationMain` classes as unused (realm#3355)
  [Fix] `UnusedCaptureListRule`: implicit self in @escaping closures (realm#3352)
  Skip correcting files with parser diagnostics (realm#3349)
  [SwiftLintFile] Remove lock in favor of UUID (realm#3347)
  [UnusedDeclarationRule] Speed up and detect more dead code (realm#3340)
  Add empty changelog section
  release 0.40.2
  ...
@marcelofabri
Copy link
Collaborator

I'm late to the party, but shouldn't we only skip files with errors? Warnings shouldn't affect SwiftLint's ability to format the code

@jpsim
Copy link
Collaborator Author

jpsim commented Apr 4, 2022

I agree, I'm supportive if you want to make that change @marcelofabri.

@marcelofabri
Copy link
Collaborator

I agree, I'm supportive if you want to make that change @marcelofabri.

will do!

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