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

Add compiler_protocol_init rule #1101

Merged
merged 3 commits into from
Jan 4, 2017
Merged

Add compiler_protocol_init rule #1101

merged 3 commits into from
Jan 4, 2017

Conversation

marcelofabri
Copy link
Collaborator

Fixes #1096

@codecov-io
Copy link

codecov-io commented Dec 31, 2016

Current coverage is 82.50% (diff: 97.43%)

Merging #1101 into master will increase coverage by 0.10%

@@             master      #1101   diff @@
==========================================
  Files           152        153     +1   
  Lines          7406       7443    +37   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6103       6141    +38   
+ Misses         1303       1302     -1   
  Partials          0          0          

Powered by Codecov. Last update 0ea3caf...ccdff37

return [range]
}

private let initCallNames: Set<String> = {
Copy link

Choose a reason for hiding this comment

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

Should this be made static as well? It doesn't look like this will ever produce a different Set.

@jpsim
Copy link
Collaborator

jpsim commented Jan 2, 2017

There are a handful of other literal syntax protocols that may need to be handled in the same way:

  • ExpressibleByNilLiteral
  • ExpressibleByBooleanLiteral
  • ExpressibleByFloatLiteral
  • ExpressibleByIntegerLiteral
  • ExpressibleByUnicodeScalarLiteral
  • ExpressibleByExtendedGraphemeClusterLiteral
  • ExpressibleByStringLiteral
  • ExpressibleByStringInterpolation
  • ExpressibleByDictionaryLiteral

@marcelofabri marcelofabri changed the title Add expressible_by_array_literal_init rule Add compiler_protocol_init rule Jan 2, 2017
@marcelofabri
Copy link
Collaborator Author

There are a handful of other literal syntax protocols that may need to be handled in the same way

Done in 7d6600a

@jpsim
Copy link
Collaborator

jpsim commented Jan 4, 2017

Is there a reason this rule shouldn't be called literal_syntax_init/LiteralSyntaxInitRule ?

Also, I didn't actually confirm that the protocols listed in #1101 (comment) have the same remark about their initializers. Did you confirm that it was the case?

private func violationRangesInFile(_ file: File,
kind: SwiftExpressionKind,
dictionary: [String: SourceKitRepresentable]) -> [NSRange] {
guard kind == .call,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a few things that could be improved in this function IMO. Especially since many variables are computed in every loop iteration even though they can't change.

Consider incorporating some of these changes:

         guard kind == .call,
-            let name = dictionary["key.name"] as? String else {
-                return []
+            let name = dictionary["key.name"] as? String,
+            let offset = (dictionary["key.offset"] as? Int64).flatMap({ Int($0) }),
+            let length = (dictionary["key.length"] as? Int64).flatMap({ Int($0) }) else {
+            return []
         }
 
+        let arguments = dictionary.enclosedArguments.flatMap({ $0["key.name"] as? String })
+
         for compilerProtocol in ExpressibleByCompiler.allProtocols {
-            guard compilerProtocol.initCallNames.contains(name),
-                case let arguments = dictionary.enclosedArguments.flatMap({ $0["key.name"] as? String }),
+            if compilerProtocol.initCallNames.contains(name),
                 compilerProtocol.matchArguments(arguments),
-                let offset = (dictionary["key.offset"] as? Int64).flatMap({ Int($0) }),
-                let length = (dictionary["key.length"] as? Int64).flatMap({ Int($0) }),
-                let range = file.contents.bridge().byteRangeToNSRange(start: offset, length: length) else {
-                    continue
+                let range = file.contents.bridge().byteRangeToNSRange(start: offset, length: length) {
+                return [range]
             }
-
-            return [range]
         }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only one that would actually be computed in every loop would be arguments (since the other ones shouldn't really return nil). However, computing it earlier actually makes the rule a bit slower because most of times a .call won't be one of the forbidden ones, so we'd be computing arguments unnecessarily.

}

private struct ExpressibleByCompiler {
let protocolName: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

private?

byStringInterpolation, byDictionaryLiteral]

func matchArguments(_ arguments: [String]) -> Bool {
for item in self.arguments {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't feel strongly about this one, so ignore if you don't like:

return self.arguments.first(where: { $0 == arguments }) != nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like the procedural style is more readable in this case 😅

@marcelofabri
Copy link
Collaborator Author

Is there a reason this rule shouldn't be called literal_syntax_init/LiteralSyntaxInitRule?

Mainly because ExpressibleByStringInterpolation does not contain Literal in its name 😅

Also, I didn't actually confirm that the protocols listed in #1101 (comment) have the same remark about their initializers. Did you confirm that it was the case?

I've checked and some of them contain the note, some don't. IMO that is probably a documentation omission and I don't see why one would call these methods directly.

@jpsim
Copy link
Collaborator

jpsim commented Jan 4, 2017

I had proposed that rule name based on Swift Evolution proposal SE-0115, which refers to these protocols as "Literal Syntax Protocols".

@@ -70,6 +70,13 @@
[JP Simard](https://github.com/jpsim)
[#1077](https://github.com/realm/SwiftLint/issues/1077)

* Add `compiler_protocol_init` rule that flags usage of initializers
declared in protocols used by compiler such as `ExpressibleByArrayLiteral`
Copy link
Collaborator

Choose a reason for hiding this comment

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

used by the compiler

@marcelofabri marcelofabri merged commit 9539cac into realm:master Jan 4, 2017
@marcelofabri marcelofabri deleted the expressible_by_array_literal_init branch January 4, 2017 19:02
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