Adds AttributeRule #847

marcelofabri · 2016-10-16T01:37:25Z

In short, here's the logic I've implemented:

If the attribute is @IBAction or @NSManaged, it should always be on
the same line as the declaration
If the attribute has parameters, it should always be on the line above
the declaration
Otherwise:
- if the attribute is applied to a variable, it should be on the same line
- if it's applied to a type or function, it should be on the line above
- if it's applied to an import (the only option is @testable import),
  it should be on the same line.

Check #846 for more details on the idea.

TODO:

Make tests green on Swift 2.2
Add tests for Swift 2.2 attributes
Handle parameter in @objc

marcelofabri · 2016-10-16T01:38:12Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+    }
+
+    private func validateTestableImport(file: File) -> [StyleViolation] {
+        let pattern = "@testable[\n]+\\s*import*"


there's no info in AST about imports:

$ sourcekitten structure --text "@testable import Module" { "key.offset" : 0, "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse", "key.length" : 23 }

marcelofabri · 2016-10-16T01:38:42Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+            let previousAttributes = try attributesFromPreviousLines(lineNumber - 1, file: file)
+
+            let alwaysInSameLineAttributes = ["@IBAction", "@NSManaged"]
+            let alwaysInNewLineAttributes = ["@available"]


maybe these should be configured?

marcelofabri · 2016-10-16T01:39:36Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+        ]
+    }
+
+    private func attributesFromPreviousLines(lineNumber: Int, file: File) throws -> [String] {


I don't particularly like this, but it was the only way I figured out to get the texts for the attributes, as there aren't exposed on AST: #846 (comment)

marcelofabri · 2016-10-16T01:49:32Z

When running the tests on SwiftLintFramework they pass, but on swiftlint they don't 😱

marcelofabri · 2016-10-16T02:08:48Z

The failing cases are:

"@GKInspectable\n var maxSpeed: Float"
"@discardableResult func a() -> Int"
"@objc\n @discardableResult func a() -> Int"
"@objc\n\n @discardableResult\n func a() -> Int"

I think they're failing because they were only introduced on Swift 3 (at least @discardableResult - couldn't find information about @GKInspectable). Is that expected?

norio-nomura · 2016-10-29T06:09:10Z

When running the tests on SwiftLintFramework they pass, but on swiftlint they don't 😱

swiftlint scheme has environment settings since 997de45 and 940fa37, but SwiftLintFramework scheme does not.
So, testing on Xcode 8.0 or later, swiftlint scheme uses SourceKit from Swift 2.3, SwiftLintFramework scheme uses from Swift 3.x (default).

marcelofabri · 2016-10-29T15:24:04Z

Thanks for explaining it, now it makes sense!

Should we wait on this until we support Swift 3.0 officially then?

norio-nomura · 2016-10-31T13:36:51Z

Should we wait on this until we support Swift 3.0 officially then?

IMO, we can use #if swift(>=3.0) to disable failing examples.

codecov-io · 2016-11-23T13:13:36Z

Current coverage is 82.00% (diff: 94.78%)

Merging #847 into master will increase coverage by 0.86%

@@             master       #847   diff @@
==========================================
  Files           123        127     +4   
  Lines          5853       6263   +410   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4749       5136   +387   
- Misses         1104       1127    +23   
  Partials          0          0

Powered by Codecov. Last update c5bcece...46c69e6

jpsim · 2016-11-25T20:01:45Z

@marcelofabri thanks for your work on this. What's the next step?

marcelofabri · 2016-11-25T21:35:51Z

I need to detect and handle attributes with parameters (I think only @objc would benefit for this now) as IMO attributes with parameters should be on their own lines as well.

Also, it'd be nice to have some feedback in the rules applied, as their reflect mostly my opinion + what I found in the docs examples.

jpsim · 2016-11-25T21:41:57Z

I need to detect and handle attributes with parameters (I think only @objc would benefit for this now) as IMO attributes with parameters should be on their own lines as well.

@available is also parameterized.

marcelofabri · 2016-11-25T21:43:46Z

I need to detect and handle attributes with parameters (I think only @objc would benefit for this now) as IMO attributes with parameters should be on their own lines as well.

@available is also parameterized.

@available is already handled in a special way: https://github.com/realm/SwiftLint/pull/847/files#diff-cd01cc2805cb6eda59b220cf4a7b0648R185. (Although we could remove that when the parameters are handled)

marcelofabri · 2016-11-27T00:07:26Z

@jpsim This is ready for review. It got waaaay more complex than I thought it would be, so feel free to throw any ideas or suggestions to make it better 💯

jpsim

Great stuff! I have lots of comments, but I'm generally very happy to see how you did this.

jpsim · 2016-11-29T07:50:47Z

CHANGELOG.md

@@ -43,6 +43,23 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#883](https://github.com/realm/SwiftLint/issues/883)

+* Add `AttributesRule` which validates if an attribute (`@objc`, `@IBOutlet`,
+  `@discardableResult`, etc) is on the right position:


"in the right position"

jpsim · 2016-11-29T07:51:27Z

CHANGELOG.md

+      - if it's applied to a type or function, it should be on the line above
+      - if it's applied to an import (the only option is `@testable import`), 
+      it should be on the same line.
+  You can also configure what attributes should be always in a new line or in


"on a new line or on the same line"

jpsim · 2016-11-29T07:51:39Z

CHANGELOG.md

+      - if it's applied to an import (the only option is `@testable import`), 
+      it should be on the same line.
+  You can also configure what attributes should be always in a new line or in
+  the same line as the declaration with the `always_in_same_line` and 


always_on_same_line

jpsim · 2016-11-29T07:52:09Z

CHANGELOG.md

+      it should be on the same line.
+  You can also configure what attributes should be always in a new line or in
+  the same line as the declaration with the `always_in_same_line` and 
+  `always_in_new_line` keys.  


always_on_line_above?

jpsim · 2016-11-29T07:56:13Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+
+    private func validateTestableImport(file: File) -> [StyleViolation] {
+        let pattern = "@testable[\n]+\\s*import"
+        let excludingKinds = SyntaxKind.commentAndStringKinds()


@testable is always a source.lang.swift.syntaxtype.attribute.builtin and import is always a source.lang.swift.syntaxtype.keyword, so you should use those rather than excluding comments and strings.

jpsim · 2016-11-29T07:57:51Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+            } else {
+
+                // ignore whitelisted attributes
+                let operation: Set<String> -> Set<String> -> Set<String> =


operation is a very generic name for this. Can you find a better name?

I couldn't find a better name 😅
Maybe we should drop this clever way and just have an if with two paths?

maybe call this let excludeWhitelistedAttributes?

I don't know, the more I think the more I realize this is too clever. Are you ok with splitting it in two paths?

jpsim · 2016-11-29T07:59:02Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+            StyleViolation(ruleDescription: self.dynamicType.description,
+                severity: configuration.severityConfiguration.severity,
+                location: location
+            )


open parens hugs the parameters, so should the closing one.

jpsim · 2016-11-29T08:01:01Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+
+        #if swift(>=3.0)
+            let swift3Only = [
+            "@GKInspectable var maxSpeed: Float",


indentation

jpsim · 2016-11-29T08:01:16Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+
+        #if swift(>=3.0)
+            let swift3Only = [
+            "@GKInspectable\n ↓var maxSpeed: Float",


indentation

jpsim · 2016-11-29T08:01:37Z

Source/SwiftLintFramework/Rules/CyclomaticComplexityRule.swift

@@ -40,7 +40,7 @@ public struct CyclomaticComplexityRule: ASTRule, ConfigurationProviderRule {

    public func validateFile(file: File, kind: SwiftDeclarationKind,
                             dictionary: [String: SourceKitRepresentable]) -> [StyleViolation] {
-        if !functionKinds.contains(kind) {
+        if !SwiftDeclarationKind.functionKinds().contains(kind) {


sweet refactoring!

jpsim · 2016-11-29T19:57:09Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

-                            previousAttributes.intersect(alwaysOnNewLineAttributes)
-                        )
-                    )(attributesTokens.intersect(alwaysOnSameLineAttributes))
+                let attributesAfterWhitelist: Set<String>


Yeah, this is definitely clearer!

Fixes #846

marcelofabri · 2016-12-09T19:52:36Z

@jpsim I'll take a look at this, thanks for letting me know!

marcelofabri · 2016-12-09T21:24:34Z

@jpsim I was able to reduce the violations to 10:

/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/List.swift:36:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:291:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:295:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:302:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:309:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:315:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:324:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:360:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/RealmSwift/Object.swift:365:19: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)
/Users/marcelofabri/Downloads/realm-cocoa-2.1.1/Realm/Swift/RLMSupport.swift:23:25: warning: Attributes Violation: Attributes should be on their own lines in functions and types, but on the same line as variables and imports. (attributes_rule)

which are all funcs with @objc or @nonobjc in the same line as the declaration.

I'll commit it shortly.

jpsim · 2016-12-12T01:33:29Z

Can you please rebase this so we can confirm it builds and passes tests on Linux?

marcelofabri · 2016-12-12T02:50:29Z

The tests are failing on Linux because of

guard let config = makeConfig(ruleConfiguration, ruleDescription.identifier) else {
    XCTFail()
    return
}

Any ideas? 😬

UPDATE: I was able to fix in fd09fde by using [String: Any] instead of [String: AnyObject] when casting the configuration object.

jpsim

Looks good, but I want to run on Realm again.

jpsim · 2016-12-12T19:32:57Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+//  AttributesRule.swift
+//  SwiftLint
+//
+//  Created by Marcelo Fabri on 10/15/16.


this is right actually 😅

jpsim · 2016-12-12T19:35:37Z

Source/SwiftLintFramework/Rules/AttributesRulesExamples.swift

+//  AttributesRulesExamples.swift
+//  SwiftLint
+//
+//  Created by Marcelo Fabri on 09/12/16.


jpsim · 2016-12-12T19:35:53Z

Source/SwiftLintFramework/Rules/RuleConfigurations/AttributesConfiguration.swift

+//  AttributesConfiguration.swift
+//  SwiftLint
+//
+//  Created by Marcelo Fabri on 26/11/16.


jpsim · 2016-12-12T19:46:18Z

Personally, I'd keep @objc and @nonobjc on the same line, so I'm trying to configure Realm's .swiftlint.yml to do so, but violations are still being reported:

attributes_rule:
  always_on_same_line:
    - '@objc'
    - '@nonobjc'

am I missing something?

marcelofabri · 2016-12-12T22:13:20Z

@jpsim I've checked why violations are still being reported and almost all of them seems to be because the attribute had a parameter. Another one was because there was two properties without blank lines between them, so the algorithm thought that the attributes from both of them belonged to the last one:

@objc private var object: RLMWeakObjectHandle?
@objc private var property: RLMProperty?

I'll try to fix these soon.
(Ugh, this rule is complicated 😅)

marcelofabri · 2016-12-13T01:05:52Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

+                    return false
+                }
+
+                return ["func", "var", "let"].contains(keyword)


this probably could be a bit safer if I used AST, but I thought it would be more complex and slower, since I already had the tokens.

marcelofabri · 2016-12-13T01:11:10Z

@jpsim OK, so I had another look on this and fixed a bug in e4a556d. The remaining violations of this rule on Realm with the updated .swiftlint.yml are because of @objc with parameters in a new line, when the configuration says they should be on the same line always. For example:

@objc(RealmSwiftObjectUtil)
public class ObjectUtil

We could update the rule implementation to prioritize the "attributes with parameters should be on their own line" over the always_on_same_line configuration (and maybe always_on_new_line?), but that might be inconvenient for a few people.

In reality, there're so many cases that this rule is probably the most complex one in SwiftLint, so I understand if you feel we shouldn't merge this or make it opt-in.

jpsim · 2016-12-13T01:15:24Z

In reality, there're so many cases that this rule is probably the most complex one in SwiftLint, so I understand if you feel we shouldn't merge this or make it opt-in.

I'm thinking opt-in might make sense for those reasons.

marcelofabri · 2016-12-13T01:43:16Z

I've updated it to be opt-in and also renamed the identifier to attributes as no other rule has _rule in the identifier.

jpsim · 2016-12-13T01:52:04Z

Source/SwiftLintFramework/Rules/AttributesRule.swift

@@ -26,7 +26,7 @@ public struct AttributesRule: ASTRule, ConfigurationProviderRule {
    public init() {}

    public static let description = RuleDescription(
-        identifier: "attributes_rule",
+        identifier: "attributes",


jpsim

👍 this looks good to me now. @marcelofabri I'll let you merge this when you're done working on it.

marcelofabri · 2016-12-13T02:03:53Z

Let's just wait for CI to be sure 😅

marcelofabri · 2016-12-13T04:05:37Z

jpsim · 2016-12-13T06:46:08Z

🎉 congrats @marcelofabri, I know this was a massive undertaking.

marcelofabri commented Oct 16, 2016

View reviewed changes

marcelofabri mentioned this pull request Oct 16, 2016

"Attributes should be on their own line" rule #846

Closed

marcelofabri changed the title ~~Adds AttributeRule~~ [WIP] Adds AttributeRule Nov 23, 2016

marcelofabri changed the title ~~[WIP] Adds AttributeRule~~ Adds AttributeRule Nov 27, 2016

jpsim requested changes Nov 29, 2016

View reviewed changes

jpsim reviewed Nov 29, 2016

View reviewed changes

marcelofabri added 11 commits November 30, 2016 11:51

Moving functionKinds and typeKinds to extension

af15654

Adding attributes rule

d94b5d8

Fixes #846

Adding changelog entry

59d3053

Using byte range

4e3a0b0

Updating CHANGELOG

774256d

Handling attributes that don’t start with @

a851717

Making tests green on Swift 2.2 ✅

17c1464

Adding Swift 2.2 attributes

7f259d2

Handling cases with multiple attributes

508f284

Handling parameters and custom configurations

12dae9c

PR feedback

5e9095a

Fix false negatives in AttributesRule

09fb1eb

marcelofabri added 3 commits December 11, 2016 23:46

Merge branch 'master' into attributes-rule

9c43a21

Running AttributesRulesTests on Linux

8da7f06

Using .bridge() to run on Linux

6752739

marcelofabri added 2 commits December 12, 2016 01:32

Trying to fix Linux build

fd09fde

Using character offset for regex result

7a43252

jpsim requested changes Dec 12, 2016

View reviewed changes

marcelofabri added 3 commits December 12, 2016 22:36

Merge branch 'master' into attributes-rule

85dede2

Handle consecutive lines with attributes

e4a556d

Dates

482dd16

marcelofabri commented Dec 13, 2016

View reviewed changes

Make AttributesRule opt-in

b599dc4

jpsim reviewed Dec 13, 2016

View reviewed changes

add missing word in changelog entry

46c69e6

jpsim approved these changes Dec 13, 2016

View reviewed changes

marcelofabri merged commit 4f4c6c6 into realm:master Dec 13, 2016

marcelofabri deleted the attributes-rule branch December 13, 2016 04:05

marcelofabri mentioned this pull request Dec 19, 2016

Closure end indentation rule #1004

Merged

3 tasks

Adds AttributeRule #847

Adds AttributeRule #847

Conversation

marcelofabri commented Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Oct 16, 2016

marcelofabri commented Oct 16, 2016 • edited Loading

norio-nomura commented Oct 29, 2016 • edited Loading

marcelofabri commented Oct 29, 2016

norio-nomura commented Oct 31, 2016

codecov-io commented Nov 23, 2016 • edited Loading

Current coverage is 82.00% (diff: 94.78%)

jpsim commented Nov 25, 2016

marcelofabri commented Nov 25, 2016 • edited Loading

jpsim commented Nov 25, 2016

marcelofabri commented Nov 25, 2016

marcelofabri commented Nov 27, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Dec 9, 2016

marcelofabri commented Dec 9, 2016 • edited Loading

jpsim commented Dec 12, 2016

marcelofabri commented Dec 12, 2016 • edited Loading

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

Choose a reason for hiding this comment

jpsim commented Dec 12, 2016

marcelofabri commented Dec 12, 2016

Choose a reason for hiding this comment

marcelofabri commented Dec 13, 2016

jpsim commented Dec 13, 2016

marcelofabri commented Dec 13, 2016

Choose a reason for hiding this comment

jpsim left a comment • edited Loading

Choose a reason for hiding this comment

marcelofabri commented Dec 13, 2016

marcelofabri commented Dec 13, 2016

jpsim commented Dec 13, 2016

marcelofabri commented Oct 16, 2016 •

edited

Loading

marcelofabri commented Oct 16, 2016 •

edited

Loading

norio-nomura commented Oct 29, 2016 •

edited

Loading

codecov-io commented Nov 23, 2016 •

edited

Loading

marcelofabri commented Nov 25, 2016 •

edited

Loading

marcelofabri commented Dec 9, 2016 •

edited

Loading

marcelofabri commented Dec 12, 2016 •

edited

Loading

jpsim left a comment •

edited

Loading