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

Identical operands rule using syntaxmap #2918

Conversation

PaulTaykalo
Copy link
Collaborator

Rewrite identical operands rule which using syntax map and tokens access instead of full Regex solution.

This solutions search for the operator first and then tries to find operands to the right and to the left of the operator. This is done by searching tokens which has dots between them.

|   asd    ?? |      self   .    a      ==    self   .    a
| [token]     |    [token]     [token]       [token]     [token]
| identifier  |    keyword    identifier     keyword    identifier

Operands are matching if the next is true:

  • they have same tokens count
  • they have tokens of the same type
  • they have identical tokens strings
  • if there's a token to the left of the left token, string between them doesn't have the nil-coalescing operator

@PaulTaykalo
Copy link
Collaborator Author

That's weird. Will fix in evening 😊

@SwiftLintBot
Copy link

SwiftLintBot commented Oct 25, 2019

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 3.09s vs 2.63s on master (17% slower)
📖 Linting Alamofire with this PR took 6.38s vs 4.24s on master (50% slower)
📖 Linting Firefox with this PR took 16.16s vs 13.59s on master (18% slower)
📖 Linting Kickstarter with this PR took 38.37s vs 28.59s on master (34% slower)
📖 Linting Moya with this PR took 2.54s vs 2.37s on master (7% slower)
📖 Linting Nimble with this PR took 2.53s vs 2.69s on master (5% faster)
📖 Linting Quick with this PR took 0.98s vs 1.2s on master (18% faster)
📖 Linting Realm with this PR took 4.02s vs 4.19s on master (4% faster)
📖 Linting SourceKitten with this PR took 1.78s vs 1.78s on master (0% slower)
📖 Linting Sourcery with this PR took 4.61s vs 5.13s on master (10% faster)
📖 Linting Swift with this PR took 21.81s vs 26.25s on master (16% faster)
📖 Linting WordPress with this PR took 32.41s vs 29.6s on master (9% slower)

Generated by 🚫 Danger

@PaulTaykalo
Copy link
Collaborator Author

Well, at least it's faster

@PaulTaykalo
Copy link
Collaborator Author

Message: Linting Aerial with this PR took 2.11s vs 2.12s on master (0% faster)
Message: Linting Alamofire with this PR took 3.76s vs 3.75s on master (0% slower)
Message: Linting Firefox with this PR took 11.88s vs 12.12s on master (1% faster)
Message: Linting Kickstarter with this PR took 27.16s vs 27.91s on master (2% faster)
Message: Linting Moya with this PR took 2.38s vs 2.3s on master (3% slower)
Message: Linting Nimble with this PR took 2.29s vs 2.41s on master (4% faster)
Message: Linting Quick with this PR took 0.95s vs 0.97s on master (2% faster)
Message: Linting Realm with this PR took 3.52s vs 3.67s on master (4% faster)
Message: Linting SourceKitten with this PR took 1.51s vs 1.62s on master (6% faster)
Message: Linting Sourcery with this PR took 4.0s vs 4.14s on master (3% faster)
Message: Linting Swift with this PR took 18.9s vs 19.78s on master (4% faster)
Message: Linting WordPress with this PR took 30.3s vs 31.38s on master (3% faster)

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

👍 nice work! Removing regex use is probably one of the more impactful ways to speed up SwiftLint 😄

Please add a changelog entry, consider addressing the nitpicks I highlighted, and merge when ready.

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Oops, looks like this introduces some new false positives according to OSSCheck's comment.

@PaulTaykalo
Copy link
Collaborator Author

@jpsim yeah. also, it seems that this solution also can handle bit complex examples like

a?.b == a?.b

@PaulTaykalo PaulTaykalo force-pushed the feature/identical-operands-rule-using-syntaxmap branch from 9ec712d to 1db3eb7 Compare October 25, 2019 16:53
@PaulTaykalo
Copy link
Collaborator Author

Let's call it a day.

@jpsim
Copy link
Collaborator

jpsim commented Oct 25, 2019

Let's call it a day.

Let's fix the remaining false positives first please. They're missing from the latest OSSCheck comment, but here's one that I copied from before: https://github.com/mozilla-mobile/firefox-ios/blob/48798608a791bbaea7ba3e22d58694318a40da0b/Shared/Extensions/URLExtensions.swift#L363:108

@PaulTaykalo PaulTaykalo requested a review from jpsim October 25, 2019 16:59
@PaulTaykalo
Copy link
Collaborator Author

PaulTaykalo commented Oct 25, 2019

Let's call it a day.

Let's fix the remaining false positives first please. They're missing from the latest OSSCheck comment, but here's one that I copied from before: https://github.com/mozilla-mobile/firefox-ios/blob/48798608a791bbaea7ba3e22d58694318a40da0b/Shared/Extensions/URLExtensions.swift#L363:108

it's done in the latest commits.
The issue was with Optional chainings like a?.b == b
Another one with the fact that we wrent' checking that there is only an operator between right and left tokens.
So examples like this were triggering function(something: nil) != nil

@jpsim
Copy link
Collaborator

jpsim commented Oct 25, 2019

@PaulTaykalo you should now have push access, feel free to merge when you're ready.

@PaulTaykalo PaulTaykalo merged commit 5892478 into realm:master Oct 26, 2019
@PaulTaykalo PaulTaykalo deleted the feature/identical-operands-rule-using-syntaxmap branch October 26, 2019 00:23
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.

3 participants