Implemented super call rule (#803) #809

angelolloqui · 2016-09-07T06:39:48Z

PR for issue #803

It basically looks for methods in the whitelist (and marked as overriden) and checks that there is exactly one super call somewhere in its implementation.

The rule comes with a custom provider that allows configuring the list of methods to include or exclude. The default configuration comes with the most common methods in Cocoa.

Example only checking 2 custom methods:

overridden_super_call:
  severity: warning
  excluded: *
  included: 
    - testMethod1()
    - testMethod2(_:)

…/super_rule

angelolloqui · 2016-09-07T07:13:23Z

Current predefined list of included methods:

var defaultIncluded = [
        //NSObject
        "awakeFromNib()",
        "prepareForInterfaceBuilder()",
        //UIViewController
        "transitionCoordinator()",
        "viewDidLoad()",
        "viewWillAppear(_:)",
        "viewWillDisappear(_:)",
        "viewDidAppear(_:)",
        "viewDidDisappear(_:)",
        "addChildViewController(_:)",
        "removeFromParentViewController()",
        "transition(from:to:duration:options:animations:completion:)",
        "transitionFromViewController(_:toViewController:duration:options:animations:completion:)",
        "didReceiveMemoryWarning()",
        "encodeRestorableState(with:)",
        "encodeRestorableStateWithCoder(_:)",
        "decodeRestorableState(with:)",
        "decodeRestorableStateWithCoder(_:)",
        "setEditing(_:animated:)",
        //UIView
        "updateConstraints()",
        "prepareForReuse()",
        //UICollectionViewLayout
        "invalidateLayout()",
        "invalidateLayout(with:)",
        "invalidateLayoutWithContext(_:)",
        //XCTestCase
        "tearDown()",
        "setUp()"
    ]

codecov-io · 2016-09-26T09:37:52Z

Current coverage is 84.08% (diff: 86.99%)

Merging #809 into master will increase coverage by 0.08%

@@             master       #809   diff @@
==========================================
  Files           102        104     +2   
  Lines          4105       4228   +123   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3448       3555   +107   
- Misses          657        673    +16   
  Partials          0          0

Powered by Codecov. Last update 3ecefe9...6161c9b

norio-nomura

Sorry for late reviews, and thanks for doing this!
Looks mostly good other than my feedbacks.

norio-nomura · 2016-10-15T06:13:28Z

CHANGELOG.md

@@ -6,6 +6,10 @@

 ##### Enhancements

+* Add `SuperCallRule` Opt-In rule that warns against overriden methods not calling to super in well known cases (ex: `viewWillAppear`).  


Could you please wrap at 80 characters?

norio-nomura · 2016-10-15T06:57:05Z

Source/SwiftLintFramework/Rules/RuleConfigurations/SuperCallConfiguration.swift

+import Foundation
+
+public struct SuperCallConfiguration: RuleConfiguration, Equatable {
+    var defaultIncluded = [


Sorting this list by class+name alphabetically would be helpful.

norio-nomura · 2016-10-15T07:00:40Z

Source/SwiftLintFramework/Rules/RuleConfigurations/SuperCallConfiguration.swift

+    var excluded: [String] = []
+    var included: [String] = ["*"]
+
+    public var consoleDescription: String {


consoleDescription is used on swiftlint rules.

+--------------------------------+--------+-------------+------------------------+--------------------------------------------------+ | identifier | opt-in | correctable | enabled in your config | configuration | +--------------------------------+--------+-------------+------------------------+--------------------------------------------------+ | closing_brace | no | yes | yes | warning | … | overridden_super_call | yes | no | yes | warning, excluded: [[]], included: [["*"]] |

The existence of defaultIncluded is not represented here.
Are there any idea for that?

you expect to see all the method list? It will pollute the table but I think it can be done easily. I am using * to reference the default list, so I can just return the evaluated list on:

public var consoleDescription: String { return severityConfiguration.consoleDescription + ", excluded: [\(excluded)]" + ", included: [\(included)]" }

Ok, never mind. 👍

norio-nomura · 2016-10-15T07:12:50Z

Source/SwiftLintFramework/Rules/RuleConfigurations/SuperCallConfiguration.swift

+        "invalidateLayoutWithContext(_:)",
+        //XCTestCase
+        "tearDown()",
+        "setUp()"


Are there any documentation XCTestCase .tearDown() and XCTestCase .setUp() require calling super?

I haven't seen anywhere explicitly, but the default template includes calling super: https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/testing_with_xcode/chapters/04-writing_tests.html#//apple_ref/doc/uid/TP40014132-CH4-SW2

It is indeed not mentioned for XCTestCase but as @marcelofabri comments, it is just there in the templates and assumed in all Apple test example code I have seen so far. We could remove it if you want, although I think since this is an Opt-In rule and it is configurable, I would leave it there as a good practice.

norio-nomura · 2016-10-15T08:05:39Z

Source/SwiftLintFramework/Rules/SuperCallRule.swift

+                    "\t\tself.method()\n" +
+                "\t}\n" +
+            "}\n",
+        ]


Could you please add multiple calling to super triggering example?

…/super_rule

norio-nomura

I guess the false negative pattern needs to be fixed.

norio-nomura · 2016-10-18T09:11:27Z

Source/SwiftLintFramework/Rules/RuleConfigurations/SuperCallConfiguration.swift

+    var excluded: [String] = []
+    var included: [String] = ["*"]
+
+    public var consoleDescription: String {


Ok, never mind. 👍

norio-nomura · 2016-10-18T09:14:03Z

Source/SwiftLintFramework/Rules/RuleConfigurations/SuperCallConfiguration.swift

+        return severityConfiguration.severity
+    }
+
+    public var resolvedMethodNames: [String] {


I guess we would not need to re-create resolvedMethodNames on every calling.
Can we change this to initialize once on first calling?

norio-nomura · 2016-10-18T10:12:03Z

Source/SwiftLintFramework/Rules/SuperCallRule.swift

+
+        guard   let offset = dictionary["key.bodyoffset"] as? Int64,
+                let name = dictionary["key.name"] as? String,
+                let substructure = dictionary["key.substructure"] as? [SourceKitRepresentable]


I found false negative pattern:

class ViewController: UIViewController { override func didReceiveMemoryWarning() { } }

that does not have "key.substructure".

ouch! thanks for pointing this out! I will check it out ASAP

…/super_rule

angelolloqui · 2016-10-19T09:42:07Z

Take a look now... 1 thing I am not sure if should be changed is that the rule id is overridden_super_call but the class implementing is SuperCallRule. If you want consistency, which name do you think makes more sense? maybe the first one?

norio-nomura · 2016-10-20T00:47:35Z

maybe the first one?

👍

angelolloqui · 2016-10-20T07:45:17Z

Renamed! Let's hope this now is good enough :)

norio-nomura · 2016-10-20T09:09:16Z

Thanks! 🙏

angelolloqui added 8 commits September 4, 2016 21:04

Added first implementation for super call check

6b5d2df

Improved code to check for override attribute to reduce false positives

77cbcd2

Added super call configuration list

96de4ee

Changed id

e37bfb3

Added more methods to list of super calls

47ef5ca

Updated contributions

cdd130c

Merge remote-tracking branch 'refs/remotes/realm/master' into feature…

f77b957

…/super_rule

Fixed typo in name

8f52cb3

fixed white spacing

b3edc67

norio-nomura requested changes Oct 15, 2016

View reviewed changes

angelolloqui added 4 commits October 17, 2016 13:23

Merge remote-tracking branch 'refs/remotes/realm/master' into feature…

51862ea

…/super_rule

Wrapped changeling to 80 chars

bdf7bae

List of methods sorted by class+method name

83141c1

Added example of duplicate call and changed viewWillAppear declaration

d499316

norio-nomura requested changes Oct 18, 2016

View reviewed changes

angelolloqui added 3 commits October 19, 2016 11:02

Fixed issue with empty body methods not triggering

811281c

Changed resolvedMethodNames to compute on configuration changes only

2d758dd

Merge remote-tracking branch 'refs/remotes/realm/master' into feature…

aec1679

…/super_rule

Renamed rule and configuration to OverridenSuperCall

6161c9b

norio-nomura approved these changes Oct 20, 2016

View reviewed changes

norio-nomura merged commit 8b65811 into realm:master Oct 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented super call rule (#803) #809

Implemented super call rule (#803) #809

angelolloqui commented Sep 7, 2016 •

edited

Loading

angelolloqui commented Sep 7, 2016

codecov-io commented Sep 26, 2016 •

edited

Loading

norio-nomura left a comment

norio-nomura Oct 15, 2016

norio-nomura Oct 15, 2016

norio-nomura Oct 15, 2016

angelolloqui Oct 17, 2016

norio-nomura Oct 18, 2016

norio-nomura Oct 15, 2016

marcelofabri Oct 15, 2016

angelolloqui Oct 17, 2016

norio-nomura Oct 15, 2016

norio-nomura left a comment

norio-nomura Oct 18, 2016

norio-nomura Oct 18, 2016

angelolloqui Oct 18, 2016

norio-nomura Oct 18, 2016

angelolloqui Oct 18, 2016

angelolloqui commented Oct 19, 2016

norio-nomura commented Oct 20, 2016

angelolloqui commented Oct 20, 2016

norio-nomura commented Oct 20, 2016

		@@ -6,6 +6,10 @@

		##### Enhancements

		* Add `SuperCallRule` Opt-In rule that warns against overriden methods not calling to super in well known cases (ex: `viewWillAppear`).

Implemented super call rule (#803) #809

Implemented super call rule (#803) #809

Conversation

angelolloqui commented Sep 7, 2016 • edited Loading

angelolloqui commented Sep 7, 2016

codecov-io commented Sep 26, 2016 • edited Loading

Current coverage is 84.08% (diff: 86.99%)

norio-nomura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norio-nomura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelolloqui commented Oct 19, 2016

norio-nomura commented Oct 20, 2016

angelolloqui commented Oct 20, 2016

norio-nomura commented Oct 20, 2016

angelolloqui commented Sep 7, 2016 •

edited

Loading

codecov-io commented Sep 26, 2016 •

edited

Loading