Added ExplicitSelfRule #829

IanKeen · 2016-10-07T19:21:48Z

Requires the explicit use of self. when accessing instance members
Issue: #321

IanKeen · 2016-10-07T20:03:14Z

Not sure what to do with those failing tests? - looks like its swiftlint integration tests?

scottrhoyt · 2016-10-07T20:15:09Z

I’m away from a computer right now, so I can’t be sure. But usually that means that the swiftlint code is generating errors or warnings while being linted by swiftlint itself. Try linting your local copy to see if this is the case.

On Oct 7, 2016, at 1:03 PM, Ian Keen notifications@github.com wrote:

Not sure what to do with those failing tests? - looks like its swiftlint integration tests?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #829 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AED9suZlCUAgHhyR9cMkbg5Pdqj74P0eks5qxqWFgaJpZM4KRXxV.

IanKeen · 2016-10-07T20:25:19Z

Ahh you might be right - that trailing whitespace always gets me!

codecov-io · 2016-10-07T20:41:38Z

Current coverage is 86.59% (diff: 100%)

Merging #829 into master will increase coverage by 0.58%

@@             master       #829   diff @@
==========================================
  Files           112        113     +1   
  Lines          4953       5162   +209   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4260       4470   +210   
+ Misses          693        692     -1   
  Partials          0          0

Powered by Codecov. Last update e65437c...f5ddc3a

NachoSoto

Awesome! I've been looking forward to this rule 👍

NachoSoto · 2016-10-08T05:30:20Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+
+        //get the members body (functions and calculated properties will have this data)
+        guard
+            let bodyLocation = (dictionary["key.bodyoffset"] as? Int64).flatMap({ Int($0) }),


You probably want as! Int64? so that this doesn't ignore wrong types.

🤔 as? seems to be the convention throughout - I think it would be preferential to a crash anyway

No, it would be preferable to crash over a silent bug. Here you just want to know if the value is not present.

I'm still inclined to leave it this way, every other rule is using code like this right now

NachoSoto · 2016-10-08T05:31:10Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+        //get the members body (functions and calculated properties will have this data)
+        guard
+            let bodyLocation = (dictionary["key.bodyoffset"] as? Int64).flatMap({ Int($0) }),
+            let bodyLength = (dictionary["key.bodylength"] as? Int64).flatMap({ Int($0) })


Can you use flatMap(Int.init) here?

Nope, It cant disambiguate (they are all .init(_ value: Foo))

NachoSoto · 2016-10-08T05:32:01Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+            "class Good2 { let value: Int = 42; func function() -> Int { return self.value } }",
+            "class Good3 { let value: Int; init() { self.value = 42 } }",
+        ],
+        triggeringExamples: [


Awesome 👍

NachoSoto · 2016-10-08T05:32:51Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+        name: "Explict Self",
+        description: "Require explicit self for instance members",
+        nonTriggeringExamples: [
+            "class Good1 { let value: Int = 42; var property: Int { return self.value } }",


I would add a few more cases here to verify that global variables, members of other types, etc. don't trigger violations.

And maybe same for accessing variables in a scope other than a type (like global values or local variables in a global function scope.
Also static functions! (Sorry, just thinking of more as I go hehe)

Keep them coming!! 👍

NachoSoto · 2016-10-08T05:34:09Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+        triggeringExamples: [
+            "class Bad1 { let value: Int = 42; var property: Int { return value } }",
+            "class Bad2 { let value: Int = 42; func function() -> Int { return value } }",
+            "class Bad3 { let value: Int; init() { value = 42 } }",


I can think of a few other cases:

Subclassing: accessing a member from the parent class.

struct and enum (because why not)

extension: verify that accessing a member from a function in an extension triggers this too.

NachoSoto · 2016-10-08T05:38:48Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+
+    func suffix(matches items: [[Generator.Element]]) -> Bool {
+        for test in items
+            where self.suffix(matches: test) { return true }


This can be simpler using Array.contains, I think simply return items.contains(self.suffix)

it calls the extension function (not the built in .suffix) above, this one is just a convenience to perform multiple checks at once

I know. You can use the appropriate function as the parameter to contas then. My point is that you don't need the loop, this is a common construct: return true if any element meets the criterion, which is what Array.contains does.

perhaps I'm misunderstanding what you mean? this function takes an Array<Array<Generator.Element>> and neither this function or the one it calls is equivalent to .contains - they are both checking the tail of the array for a set of elements in a specific order. can you perhaps show me a snippet of what you mean?

keith · 2016-10-08T06:07:21Z

Here's 2 interesting examples we might see that I'm not sure are covered here:

struct Foo {
    var a = 1
}

extension Foo {
    func bar() {
        print(a)
    }
}

Foo().bar()

and

struct Foo {
    var something = 1
}

extension Foo {
    func bar(something: Int) {
        print(something)
    }
}

Foo().bar(something: 1)

IanKeen · 2016-10-08T06:11:01Z

ah nice @keith ! @NachoSoto pointed out the first in his comments but the second will be an interesting one 😄

I'll dive into the suggestions this weekend at some point.

IanKeen · 2016-10-08T23:37:26Z

So I've significantly improved the test coverage thanks to your suggestions guys which has made the code much more robust 👌

The covered cases are (all tested with class and struct (except subclasses obviously)):

Member 👇 / Accessed From 👉	Init	Function	Calculated Property	Extension Calculated Property	Extension Function	Static Property	Static Function	Subclass Property	Subclass Function
Property	✅	✅	✅	✅	✅	✅	✅	✅	✅
Function	✅	✅	✅	✅	✅	✅	✅	✅	✅

as well as some edge cases:

Function parameter shadowing wont trigger this rule, eg:

class Edge1 { 
    let value: Int = 42
    func function(value: Int) -> Int { return value /* refers to parameter */ } 
}

class Edge2 { 
    func value() -> Int { return 42 }
    func function(value: Int) -> Int { return value /* refers to parameter */ } 
}

Thanks @NachoSoto and @keith for the suggestions 👍

NachoSoto · 2016-10-10T04:00:08Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+        + "class Bad18_B: Bad18_A { func function() -> Int { return value() } }",
+]
+
+let nonTriggeringExamples = ["class", "struct"].flatMap(nonTriggering)


NachoSoto

Fantastic 👌

NachoSoto · 2016-10-10T04:01:16Z

keith · 2016-10-10T05:35:57Z

I just ran this on our codebase and found a few false positives. Here's one:

struct Foo {
    var bar: Int

    func baz() {
        let bar = 123
        _ = bar
    }
}

keith · 2016-10-10T05:36:39Z

Here's another interesting one:

struct Foo {
    func bar() {
        self.foo.bar()
    }
}

keith · 2016-10-10T05:38:08Z

And another:

struct Foo {
    func baz() {
        let foo = 1
        foo.something()
    }
}

struct Qux {
    func something() {}
}

keith · 2016-10-10T05:39:03Z

I think some of these require more semantic understanding that what we have in SwiftLint. So maybe it isn't really feasible, but I can provide a few more examples if needed.

IanKeen · 2016-10-10T22:10:13Z

@keith hmm I can investigate that first one, but the others don't appear to be valid code? I'm not sure it's within the scope of a rule to lint code that otherwise doesn't compile?

IanKeen · 2016-10-10T23:49:00Z

@keith Updated to handle local scope shadowing
re: #829 (comment)

keith · 2016-10-11T00:03:36Z

Sorry, I didn't provide full enough examples here, but each of these cases was pulled from our real code and just made smaller for examples.

IanKeen · 2016-10-11T02:49:20Z

Ah right, well if you can turn examples 2 & 3 above into full working snippets I'm happy to take a look 😄

norio-nomura · 2016-10-12T10:29:54Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+    public init() { }
+
+    public static let description = RuleDescription(
+        identifier: "explicitSelf",


Maybe rule identifier should be snake case.

norio-nomura · 2016-10-12T13:30:09Z

I tried to enable this rule in .swiftlint.yml of SwiftLint repository and ran integration test.

I feel it generates too much false positives yet.

NachoSoto · 2016-10-12T14:46:44Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+    public static let description = RuleDescription(
+        identifier: "explicitSelf",
+        name: "Explict Self",
+        description: "Require explicit self for instance members",


The wording seems weird. How about "Explicit self for instance members and functions is required".

jpsim · 2016-11-25T18:32:08Z

Given the number of false positives this triggers, I'm glad this is implemented as an opt-in rule.

@IanKeen would you mind rebasing this against master so we can make a last push to land this?

On a side note, this type of rule would benefit from being implemented in a new mode of SwiftLint that I've been considering: one that operates after the language Parse phase, actually after the Semantic Analysis phase, which would let SwiftLint know the difference between instance bar and local bar in @keith's comment above:

struct Foo {
    var bar: Int

    func baz() {
        let bar = 123
        _ = bar
    }
}

This type of mode would require that the module being analyzed 1) already be built by the compiler and 2) that SwiftLint would somehow know all the compiler flags needed to build the module. But if we can find a good user experience for this, it would enable much more powerful rules, from multi-file aware linting (e.g. #836) to static analysis.

This is much more straightforward for using with the Swift Package Manger since it generates a build manifest which includes all the compiler arguments (.build/debug.yaml), but it's doable with Xcode too.

The way jazzy does this is by straight up cleaning and building the Xcode project, parsing the xcodebuild log to extract arguments for a module, but that's very time consuming as the project has to be built from scratch to work reliably.

I hope I can sit down at one point and file an issue tracking exactly how building such a mode for SwiftLint could be built. Until then, I'm sharing these loose thoughts here to spur ideas among SwiftLint contributors and users.

IanKeen · 2016-11-25T23:02:47Z

@jpsim weird, should be good now

jpsim

I'd like to see someone that follows this style in their codebase run it against this rule before signing off... Maybe @NachoSoto?

jpsim · 2016-11-25T23:34:31Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+
+extension SyntaxToken {
+    func name(file: File) -> String {
+        return file.contents.substring(self.offset, length: self.length)


Even though that's the opposite of what this rule is enforcing, this project prefers omitting self. wherever possible. Could you please adapt your code to be consistent with this?

jpsim · 2016-11-25T23:35:54Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+            .componentsSeparatedByString(",")
+
+        var signature = ""
+        for param in params {


this could be refactored as let signature = reduce("") { ... }

jpsim · 2016-11-25T23:37:18Z

Source/SwiftLintFramework/Rules/ExplictSelfRule.swift

+import Foundation
+import SourceKittenFramework
+
+func nonTriggering(type: String) -> [String] {


all the implicitly internal members in this file that aren't needed in other files should be marked as private or fileprivate.

NachoSoto · 2016-11-26T15:41:42Z

@jpsim I'll test this on Monday!

jpsim · 2016-11-29T20:55:17Z

@NachoSoto friendly reminder to test this when you get a chance 👍

jpsim · 2016-12-12T01:35:20Z

@IanKeen sorry for landing more changes recently and causing merge conflicts with this. Can you please rebase this so we can confirm it builds and passes tests on Linux?

Can anyone think of a popular OSS project that already implicitly adheres to this rule (in spirit at least) so that we can run this against a real codebase to make sure it works as intended, without too many false positives?

JohnEstropia · 2017-01-16T02:20:17Z

Can anyone think of a popular OSS project that already implicitly adheres to this rule

If you still need libraries to test this against, CoreStore observes this rule.

kimdv · 2017-03-20T13:43:58Z

Any status on this @IanKeen ? 😄

elland · 2017-04-05T09:00:06Z

Can we expect this to be merged soon?

sammy-SC · 2017-04-18T21:00:59Z

@IanKeen @jpsim
Is it alright if I try to continue the work on this PR?

IanKeen · 2017-04-18T21:02:57Z

ugh, very sorry I've had 0 time to jump back on this... I would love it if you wanna take this one @sammy-SC ... work + personal duties has left me no time for anything fun 😢

…xplicit-self

sammy-SC · 2017-04-25T16:57:43Z

@IanKeen if you don't mind, could you give me push access to this branch? Otherwise I would have to open new PR.

…icit-self

IanKeen · 2017-04-26T19:54:43Z

@sammy-SC no idea how to do that, just added you as a collaborator on the repo 👍

SwiftLintBot · 2017-04-29T08:00:45Z

	1 Error
🚫	Could not build branch

	1 Warning
⚠️	Big PR

Generated by 🚫 danger

sammy-SC · 2017-04-29T19:57:17Z

@IanKeen Thanks, that's exactly what I had in mind.

I tried running this rule against https://github.com/JohnEstropia/CoreStore, it produces lots of false negatives, I am looking into it.

keith · 2017-04-29T20:10:58Z

FWIW I've ran it on our codebase of ~70K LOC where we require self. and it generated about 4k false positives. For more context, running ag "\Wself\." shows that we have 12k self. uses.

It seems like a rule that needs this much context should really be run outside of SwiftLint against the AST, since otherwise it will never not produce significant numbers of false positives. Just my 2 cents.

marcelofabri · 2017-07-02T12:57:23Z

I'm closing this for now due to lack of activity and the high number of false positives. Feel free to open another pull request if the issues are fixed.

NachoSoto reviewed Oct 8, 2016

View reviewed changes

norio-nomura added the enhancement Ideas for improvements of existing features and rules. label Oct 8, 2016

NachoSoto reviewed Oct 10, 2016

View reviewed changes

norio-nomura reviewed Oct 12, 2016

View reviewed changes

NachoSoto reviewed Oct 12, 2016

View reviewed changes

jpsim mentioned this pull request Nov 25, 2016

Fix empty_count false-positives #827

Closed

Ian Keen and others added 5 commits November 25, 2016 13:34

Added ExplicitSelfRule

59b51be

removed print

8e218d0

fix for swiftlint errors

eab8172

removed trailing whitespace

6d2c58a

huge increase in test cases and the appropriate additions to handle them

477675a

minor identifier/description updates

f5ddc3a

IanKeen force-pushed the explicit-self branch from 9870802 to f5ddc3a Compare November 25, 2016 22:11

jpsim requested changes Nov 25, 2016

View reviewed changes

masters3d mentioned this pull request Dec 5, 2016

Fix stylistic warnings exercism/swift#210

Merged

jpsim mentioned this pull request Dec 15, 2016

explicit_init - creating an instance from a meta-type #984

Closed

Merge branch 'master' of https://github.com/sammy-SC/SwiftLint into e…

b85820f

…xplicit-self

sammy-SC added 4 commits April 25, 2017 22:44

Migrate to Swift 3 syntax

14ee28a

Use helpers

7b31462

Add ExplicitSelf to tests

4a287c7

Merge branch 'master' of https://github.com/realm/SwiftLint into expl…

3f98fab

…icit-self

sammy-SC added 2 commits April 29, 2017 09:57

Fix swift 3 syntax in triggering examples

3f1d9d4

Use byteranges

acffe2b

sammy-SC added 2 commits April 29, 2017 10:07

Make all implicitly internal members private or fileprivate

86c3545

Move file in xcode navigation

6450ddc

marcelofabri closed this Jul 2, 2017

Added ExplicitSelfRule #829

Added ExplicitSelfRule #829

Conversation

IanKeen commented Oct 7, 2016

IanKeen commented Oct 7, 2016

scottrhoyt commented Oct 7, 2016

IanKeen commented Oct 7, 2016

codecov-io commented Oct 7, 2016 • edited Loading

Current coverage is 86.59% (diff: 100%)

NachoSoto 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

IanKeen Oct 8, 2016 • edited Loading

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

IanKeen Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith commented Oct 8, 2016

IanKeen commented Oct 8, 2016

IanKeen commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

NachoSoto commented Oct 10, 2016

keith commented Oct 10, 2016

keith commented Oct 10, 2016

keith commented Oct 10, 2016

keith commented Oct 10, 2016

IanKeen commented Oct 10, 2016

IanKeen commented Oct 10, 2016

keith commented Oct 11, 2016

IanKeen commented Oct 11, 2016

Choose a reason for hiding this comment

norio-nomura commented Oct 12, 2016

Choose a reason for hiding this comment

jpsim commented Nov 25, 2016

IanKeen commented Nov 25, 2016

jpsim 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

NachoSoto commented Nov 26, 2016

jpsim commented Nov 29, 2016

jpsim commented Dec 12, 2016

JohnEstropia commented Jan 16, 2017 • edited Loading

kimdv commented Mar 20, 2017

elland commented Apr 5, 2017

sammy-SC commented Apr 18, 2017

IanKeen commented Apr 18, 2017

sammy-SC commented Apr 25, 2017

IanKeen commented Apr 26, 2017

SwiftLintBot commented Apr 29, 2017 • edited Loading

sammy-SC commented Apr 29, 2017

keith commented Apr 29, 2017

marcelofabri commented Jul 2, 2017

codecov-io commented Oct 7, 2016 •

edited

Loading

IanKeen Oct 8, 2016 •

edited

Loading

IanKeen Oct 8, 2016 •

edited

Loading

IanKeen commented Oct 8, 2016 •

edited

Loading

JohnEstropia commented Jan 16, 2017 •

edited

Loading

SwiftLintBot commented Apr 29, 2017 •

edited

Loading