Rebased to nn-swift-4.1 #2

dirtydanee · 2018-03-05T20:01:34Z

As we discussed, i have rebased to nn-swift-4.1.

I would have a couple of questions regarding your implementation. I thought this PR could be a good place to discuss it.

First question: Shall i rather rebase you original branch attibute_and_modifier_order to nn-swift-4.1, than open this PR? 😜

``` opt_in_rules: - modifiers_order modifiers_order: before_acl: ["override"] after_acl: ["class"] ```

> warning: initializer 'init(configuration:)' nearly matches defaulted requirement 'init(configuration:)' of protocol 'Rule' > note: candidate has non-matching type '(configuration: SeverityLevelsConfiguration)' > note: move 'init(configuration:)' to an extension to silence this warning > SwiftLintFramework.Rule:5:12: note: requirement 'init(configuration:)' declared here > public init(configuration: Any) throws

``` opt_in_rules: - modifiers_order modifiers_order: before_acl: ["override"] after_acl: ["class"] ```

…fier_order

masters3d · 2018-03-05T20:17:37Z

Yeah, lets rebase. If the rebase goes well, go ahead a force push this to my branch. I made a back up on github. Then we can see the changes on the swiftlints PR.

masters3d · 2018-03-05T20:18:22Z

We can also point the PR on swift lint to the 4.1 PR

dirtydanee · 2018-03-05T21:17:40Z

I have rebased + fixed tests. Force pushed to origin. Could you retarget the PR on SwiftLint?

masters3d · 2018-03-05T21:54:04Z

retargeted

dirtydanee · 2018-03-06T12:41:10Z

My basic idea on how to solve this rule would be something like the following using this example:

@objc \n override public private(set) weak var foo: Bar?

Will produce this:

["key.diagnostic_stage": "source.diagnostic.stage.swift.parse", 
"key.substructure": [
	["key.attributes": [
			["key.attribute": "source.decl.attribute.weak"], 
                         ["key.attribute": "source.decl.attribute.override"],
			["key.attribute": "source.decl.attribute.objc"]
		], 
	"key.nameoffset": 37, 
	"key.accessibility": "source.lang.swift.accessibility.public", 
	"key.length": 26, 
	"key.typename": "Bar?", 
	"key.name": "foo", 
	"key.kind": "source.lang.swift.decl.var.global", 
	"key.offset": 33, 
	"key.namelength": 8, 
	"key.setter_accessibility": "source.lang.swift.accessibility.private"]
], 
"key.offset": 0, 
"key.length": 59]

We have:

@objc -> "key.attribute": "source.decl.attribute.objc"
public ->  "key.accessibility": "source.lang.swift.accessibility.public"
override -> "key.attribute": "source.decl.attribute.override"
private(set) ->  "key.setter_accessibility": "source.lang.swift.accessibility.private"
weak ->  "key.attribute": "source.decl.attribute.weak"
var ->  "key.kind": "source.lang.swift.decl.var.global"

Using the info above and switching to ASTRule observing SwiftDeclarationKind, we could get the line with the range containing key.offset from the file. Than break down the line into an array of String or something, and take a look if the order is as it is in ModifiersOrderConfiguration.

In ModifiersOrderConfiguration, we should create groups like it was mentioned:

ACLs
ACLs for setter
override ( ? )
final
mutating/nonmutating
static/class
weak/unowned

If i understand you approach correctly, you are doing something similar. What i can not understand, what does the extractContinuousSections function do.

Do you think my approach sounds okay, or would you prefer to somehow improve your initial idea?
You have an idea, why did the reviewer of the original PR thought of the original solution as a bit fragile?
What should i be paying extra attention to or what could be the pitfalls when writing this rule?
What could i do with the failing tests on the original PR? I am not sure how could i trigger them locally.

masters3d · 2018-03-06T15:33:53Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+        for token in tokens {
+            // static & class are not builtin attibutes so we check every keyword for static || class
+            if token.type == keyword,
+                token.length == "static".bridge().length || token.length == "class".bridge().length,


this part here is fragile because “static” is hard coded.

masters3d · 2018-03-06T15:34:23Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+                    case "static":
+                        tokenSection.append(token)
+                    // filter out `class` when used as a declaration
+                    case "class" :


hard coded here too which makes it fragile.

masters3d · 2018-03-06T15:37:21Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+        ]
+    )
+
+    private func extractContinuousSections(from tokens: [SyntaxToken], in file: File) -> [[SyntaxToken]] {


If I can remember right this will give you have arrays where builtins, keywords happen. As in weak override perhaps is one continuous sub array of same type of keywords.

masters3d · 2018-03-06T15:38:11Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+    private func extractContinuousSections(from tokens: [SyntaxToken], in file: File) -> [[SyntaxToken]] {
+        var builtInTokenSections = [[SyntaxToken]]()
+        var tokenSection = [SyntaxToken]()
+        let builtIn = "source.lang.swift.syntaxtype.attribute.builtin"


I think there are facilities to get this information from source kit. Pretty much I think everything hard coded like this needs to go.

masters3d · 2018-03-06T15:39:10Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+        return builtInTokenSections.filter({ $0.count > 1 })
+    }
+
+    private func isAccessControlLabel(_ input: String) -> Bool {


another part that may be fragile. I could not find the facilities to get this information from the source kit.

masters3d · 2018-03-06T15:50:51Z

sounds good. I am not sure about the groupings. Naming is hard. I opted for the config to pivot around ACL (including setters) so you would just need to specify which “keywords” should go before or after ACL. If you only specify before_acl then everything else is assumed it goes after the ACL.

Not sure if that is the best way though. Perhaps if you want to tackle getting it to depend on the new 4.1 functionality first? Then we can ask the community upstream what would be best.

dirtydanee · 2018-03-06T18:09:55Z

I see your point with the grouping, yeah, it could be trouble. Lets ask then the community, what do they think, or how to approach that part.

Looking at the PRs from Marcelo, ACL is added to the key.attributes and the offset should be there too. Using this we could simply remove the part of getting the line and tralala, just compare offSets. It should a lot more simple than my first idea.

~~However, i do not get those updates, but I have:~~

Xcode 9.3 Beta 4

I have switched SwiftLint, SwiftLintFramework and SwiftLintFrameworkTests to use Swift 4.1

The package file contains a dependency of SourceKitten from nn-swift-4.1.

~~What am i missing to really use 4.1 ?~~ -> command line tools..

🎉I get this structure with 4.1 🎉

[
	"key.attributes": [
		[
		"key.attribute": "source.decl.attribute.weak", 
		"key.offset": 37, 
		"key.length": 4
		], 
		[
		"key.attribute": "source.decl.attribute.setter_access.private", 
		"key.offset": 24, 
		"key.length": 12
		], 
		["key.attribute": "source.decl.attribute.public", 
		"key.offset": 17, 
		"key.length": 6
		], 
		[
		"key.attribute": "source.decl.attribute.override", 
		"key.offset": 8, 
		"key.length": 8], 
		[
		"key.attribute": "source.decl.attribute.objc", 
		"key.offset": 0, 
		"key.length": 5
		]
	], 
	"key.nameoffset": 46, 
	"key.accessibility": "source.lang.swift.accessibility.public", 
	"key.length": 13, 
	"key.typename": "Bar?", 
	"key.name": "foo", 
	"key.kind": "source.lang.swift.decl.var.global", 
	"key.offset": 42, 
	"key.namelength": 3, 
	"key.setter_accessibility": "source.lang.swift.accessibility.private"
]

This means, we can drop the idea of having to search for the line, just simply look at the offset. Looks much simpler, right?

…d/SwiftLint into dm/attribute_and_modifier_order

dirtydanee · 2018-03-06T20:52:42Z

Added an initial version of the modified modifiers rule.

I am seeing the need to introduce some enum for the declaration kinds (SwiftDeclarationAttributeKind), plus somehow they should be grep-ed from SourceKitService to have the latest for 4.1.

Somehow working with the groups seems very natural, but i can stick with beforeACL and afterACL, lets see how that goes.

Let me know what do you think about the approach.

masters3d · 2018-03-06T23:33:32Z

The implementation detail can use groups. I think this is great. The part that I am not sure is exposing that as a configuration? Perhaps you are able to expose them with some sort of different naming? The exposed “api” is what I am thinking about.

masters3d · 2018-03-06T23:50:28Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+    case `override`         = "source.decl.attribute.override"
+    case `weak`             = "source.decl.attribute.weak"
+    case `testable`         = "source.decl.attribute.testable"
+    case privateSetter      = "source.decl.attribute.setter_access.private"


Is there more options like public and internal open etc

There must be, but yesterday i was not able to grep the values from SwiftDeclarationAttribute . I only have the 9.3 beta, and i can not install it as my default, i still need 9.2 for work. And as far i have seen SourceKitService framework is only available with your default Xcode. I will try to figure something out...

masters3d · 2018-03-06T23:52:27Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+import Foundation
+import SourceKittenFramework
+
+// TODO: Get somehow `SwiftDeclarationAttribute` for Swift 4.1


class and static? class can be an attribute or a keyword

added TODO

This issue is painful. I will push an update in the following days, but static and class is not recognised as attributes by SourceKitten

can You file a radar?

I have added a workaround in my latest commit, but its an ugly hack. Please let me know what do you think, or we should wait until a further release.

I will open a radar meanwhile.

Actually, now that i am just about to open the radar, i can already see, add them as source.lang.swift.decl.var.static or source.lang.swift.decl.function.method.class. They might just answer it is not a declaration attribute, but a declaration. Maybe i could work out something based on that, rather than this regex search i am doing now. However, somehow i should still fake them i am affraid..

For example:

class Foo { static public let bar = 3 }

will give

{ "key.diagnostic_stage": "source.diagnostic.stage.swift.parse", "key.substructure": [ { "key.bodylength": 31, "key.nameoffset": 6, "key.accessibility": "source.lang.swift.accessibility.internal", "key.length": 43, "key.substructure": [ { "key.nameoffset": 32, "key.accessibility": "source.lang.swift.accessibility.public", "key.length": 25, "key.name": "bar", "key.kind": "source.lang.swift.decl.var.static", "key.namelength": 3, "key.offset": 14, "key.attributes": [ { "key.attribute": "source.decl.attribute.public", "key.offset": 21, "key.length": 6}] } ], "key.runtime_name": "_TtC8__main__3Foo", "key.name": "Foo", "key.kind": "source.lang.swift.decl.class", "key.namelength": 3, "key.offset": 0, "key.bodyoffset": 11 } ], "key.offset": 0, "key.length": 43 }

Meanwhile, i decided to raise the issue, lets see what they say.
https://bugs.swift.org/browse/SR-7252

masters3d · 2018-03-06T23:53:04Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+    case `required`         = "source.decl.attribute.required"
+    case `optional`         = "source.decl.attribute.optional"
+    case noreturn           = "source.decl.attribute.noreturn"
+    case `NSManaged`        = "source.decl.attribute.NSManaged"


These start with @ should we make the distinction?

masters3d · 2018-03-08T03:04:33Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+    case `iboutlet`         = "source.decl.attribute.iboutlet"
+    case `ibdesignable`     = "source.decl.attribute.ibdesignable"
+    case `ibinspectable`    = "source.decl.attribute.ibinspectable"
+    case `objc`             = "source.decl.attribute.objc" // TODO: This always has to be first. How to handle this case?


We can relay on the compiler to make sure that’s happens. We don’t have to worry about it. If somebody gives the wrong configuration we are not able to check it.

masters3d · 2018-03-08T03:05:39Z

Source/SwiftLintFramework/Rules/ModifiersOrderRule.swift

+        ]
+    )
+
+    private let observedDeclarationKinds: [SwiftDeclarationKind] =  [.class,


Why do we need to specify this?

I was thinking this might speeds up things, since we skip a few cases. It might makes no diff at all.

masters3d · 2018-03-08T06:12:30Z

Is there a way to know from source kit if an attribute is a modifier? As in it starts with @? Then all you have worry about is modifiers, ACL, other.

dirtydanee · 2018-03-21T17:37:54Z

Maybe we could filter out all these @objc, @objcmembers, @IBAction` etc... ?
There is no diff from the point of view of source kit, but i could filter these out from the dictionary. Right now i add these as hard coded first elements. Would you rather prefer the filtering?

dirtydanee · 2018-03-27T20:29:13Z

@masters3d I have pushed a much simpler solution, no fake cases, using SwiftDeclarationKind and SwiftDeclarationAttributeKind.

I think i am getting close to the final solution, i will start expending the possible test cases. If you would have time for a review, maybe suggest a few test cases, it would be great!

masters3d · 2018-03-27T20:47:27Z

Source/SwiftLintFramework/Helpers/SwiftDeclarationAttributeKindsGroup.swift

+                    .nonmutating]
+        case .override:
+            return [.override]
+        case .memoryReference:


masters3d · 2018-03-27T20:49:05Z

Source/SwiftLintFramework/Helpers/SwiftDeclarationAttributeKindsGroup.swift

+                    .public,
+                    .open]
+        case .setterACL:
+            return [.privateSetter,


is open setter a thing?

fileprivate?

open set is a thing 🤯

added these cases too.

masters3d

I think this would be hard to keep in sync with new "keywords" being added. I believe there is already some code to deal with ACL and attributes that start with @ .

dirtydanee · 2018-03-28T19:33:43Z

There is SwiftDeclarationKind+SwiftLint and AttributesRule are the ones i think you refer to. I can refactor the groups to be similar to SwiftDeclarationKind+SwiftLint. Do you have an other idea how could i approach the grouping?

Also, SwiftDeclarationAttributeKind should ideally be going to SourceKitten. I have opened an issue, let see what they think.

The attributes of the @ modifiers are just hardcoded strings in AttributeRule. I am not sure if they could be reused.

masters3d · 2018-03-28T21:53:22Z

I dont have more ideas on how to do the spitting of the groups other than what I already said.

The compiler already enforces attributes that start with @ to go at the beginning of the declaration. I think swift lint even has a rule to make sure these attributes go on a separate line.

dirtydanee · 2018-04-08T06:29:48Z

Declaration attribute has been merged to SourceKitten, and increased the Swift version to 4.1 🎉. Things starting to get together for this PR.

I will be keeping the groups approach and extending it with further tests, and try to see how to utilise on the @ attributes enforced by the compiler.

masters3d · 2018-04-10T19:36:20Z

I retargeted the other branch to go back to master. Let me know if you need anything from me.

# Conflicts: # Cartfile.private # Carthage/Checkouts/SourceKitten # Rules.md # Source/SwiftLintFramework/Extensions/Dictionary+SwiftLint.swift # SwiftLint.xcodeproj/project.pbxproj

dirtydanee · 2018-05-08T07:28:32Z

so it was merged. Thanks for the cooperation!

masters3d · 2018-05-08T14:12:09Z

Thank you for pushing this forward.

masters3d and others added 16 commits July 4, 2017 12:09

modifierOrderRule added

762f07f

``` opt_in_rules: - modifiers_order modifiers_order: before_acl: ["override"] after_acl: ["class"] ```

fixed swiftlint violations

fe62121

added rule documentation and ruleKind

304a223

moved changelog entry to correct spot

2bd6923

Update SourceKitten and Yams

eb05ad3

'flatMap' is deprecated: renamed to 'compactMap(_:)'

059ee7b

Swift 4.1 with -swift-version 3 is Swift 3.3

1705167

ObjCBool is no longer implicitly converted to Bool on Linux

e11690e

Changed incorrectly used flatMap to map

49f3d00

Add linux_swift_4.1 job to CircleCI

d988ae4

modifierOrderRule added

2ed4996

``` opt_in_rules: - modifiers_order modifiers_order: before_acl: ["override"] after_acl: ["class"] ```

fixed swiftlint violations

c8565da

added rule documentation and ruleKind

2c507c2

moved changelog entry to correct spot

3e13f22

Merge branch 'attibute_and_modifier_order' into dm/attribute_and_modi…

de737f9

…fier_order

dirtydanee force-pushed the attibute_and_modifier_order branch from 2bd6923 to d02a650 Compare March 5, 2018 21:11

masters3d reviewed Mar 6, 2018

View reviewed changes

Daniel Metzing added 2 commits March 6, 2018 21:47

Adding first draft of updated modifiers rule

38e8184

Merge branch 'dm/attribute_and_modifier_order' of github.com:masters3…

2136525

…d/SwiftLint into dm/attribute_and_modifier_order

masters3d reviewed Mar 6, 2018

View reviewed changes

dirtydanee force-pushed the dm/attribute_and_modifier_order branch 2 times, most recently from 83f292d to 1b09fd7 Compare March 7, 2018 18:28

masters3d reviewed Mar 8, 2018

View reviewed changes

masters3d reviewed Mar 27, 2018

View reviewed changes

dirtydanee force-pushed the dm/attribute_and_modifier_order branch from 89ad548 to 0e1c782 Compare April 10, 2018 19:53

Updating config, creating groups

664ce7b

dirtydanee force-pushed the dm/attribute_and_modifier_order branch from 0e1c782 to 664ce7b Compare April 10, 2018 19:58

Fixing further tests

5c806b6

dirtydanee changed the base branch from attibute_and_modifier_order to master April 10, 2018 20:03

dirtydanee changed the base branch from master to upstream_master April 10, 2018 20:08

Merge branch 'upstream_master' into dm/attribute_and_modifier_order

10d33f5

# Conflicts: # Cartfile.private # Carthage/Checkouts/SourceKitten # Rules.md # Source/SwiftLintFramework/Extensions/Dictionary+SwiftLint.swift # SwiftLint.xcodeproj/project.pbxproj

dirtydanee closed this May 8, 2018

Rebased to nn-swift-4.1 #2

Rebased to nn-swift-4.1 #2

Conversation

dirtydanee commented Mar 5, 2018 • edited Loading

masters3d commented Mar 5, 2018

masters3d commented Mar 5, 2018

dirtydanee commented Mar 5, 2018

masters3d commented Mar 5, 2018

dirtydanee commented Mar 6, 2018 • 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

masters3d commented Mar 6, 2018

dirtydanee commented Mar 6, 2018 • edited Loading

dirtydanee commented Mar 6, 2018

masters3d commented Mar 6, 2018

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

masters3d commented Mar 8, 2018

dirtydanee commented Mar 21, 2018

dirtydanee commented Mar 27, 2018

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

masters3d left a comment

Choose a reason for hiding this comment

dirtydanee commented Mar 28, 2018 • edited Loading

masters3d commented Mar 28, 2018

dirtydanee commented Apr 8, 2018

masters3d commented Apr 10, 2018

dirtydanee commented May 8, 2018

masters3d commented May 8, 2018

dirtydanee commented Mar 5, 2018 •

edited

Loading

dirtydanee commented Mar 6, 2018 •

edited

Loading

dirtydanee commented Mar 6, 2018 •

edited

Loading

dirtydanee commented Mar 28, 2018 •

edited

Loading