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

Colon Rule doesn't apply to dictionaries when used as a type #1074

Closed
koke opened this Issue Dec 27, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@koke
Contributor

koke commented Dec 27, 2016

I've noticed SwiftLint 0.15 doesn't correct an expression like let abc: [Void:Void] (should correct to let abc: [Void: Void]).

I tried to fix it myself, but I'm not sure what's causing the problem. The File structure is different when the dictionary is used just as a type than when it's used as a constructor, but I don't know if it's a problem in SwiftLint, SourceKitten, or a SourceKit bug.

(lldb) expr print(File(contents: "let abc: [Void:Void]\n").structure.description)
{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.substructure" : [
    {
      "key.nameoffset" : 4,
      "key.typename" : "[Void:Void]",
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.length" : 20,
      "key.name" : "abc",
      "key.kind" : "source.lang.swift.decl.var.global",
      "key.namelength" : 3,
      "key.offset" : 0
    }
  ],
  "key.offset" : 0,
  "key.length" : 21
}
(lldb) expr print(File(contents: "let abc = [Void:Void]()\n").structure.description)
{
  "key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
  "key.substructure" : [
    {
      "key.nameoffset" : 4,
      "key.accessibility" : "source.lang.swift.accessibility.internal",
      "key.length" : 23,
      "key.name" : "abc",
      "key.kind" : "source.lang.swift.decl.var.global",
      "key.namelength" : 3,
      "key.offset" : 0
    },
    {
      "key.bodylength" : 0,
      "key.nameoffset" : 10,
      "key.substructure" : [
        {
          "key.bodylength" : 9,
          "key.nameoffset" : 0,
          "key.elements" : [
            {
              "key.kind" : "source.lang.swift.structure.elem.expr",
              "key.offset" : 11,
              "key.length" : 4
            },
            {
              "key.kind" : "source.lang.swift.structure.elem.expr",
              "key.offset" : 16,
              "key.length" : 4
            }
          ],
          "key.length" : 11,
          "key.bodyoffset" : 11,
          "key.offset" : 10,
          "key.kind" : "source.lang.swift.expr.dictionary",
          "key.namelength" : 0
        }
      ],
      "key.length" : 13,
      "key.name" : "[Void:Void]",
      "key.bodyoffset" : 22,
      "key.kind" : "source.lang.swift.expr.call",
      "key.offset" : 10,
      "key.namelength" : 11
    }
  ],
  "key.offset" : 0,
  "key.length" : 24
}
$ xcodebuild -version
Xcode 8.2.1
Build version 8C1002
@koke

This comment has been minimized.

Show comment
Hide comment
@koke

koke Dec 27, 2016

Contributor

It seems SwiftLint 0.15 also missed some other cases, here's what I had to replace manually: wordpress-mobile/WordPress-iOS@df5a04a

Contributor

koke commented Dec 27, 2016

It seems SwiftLint 0.15 also missed some other cases, here's what I had to replace manually: wordpress-mobile/WordPress-iOS@df5a04a

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Dec 27, 2016

Collaborator

Yeah, that structure is expected. However, that violation should be caught by the "old" implementation.

In this case, both tokens are .typeidentifiers, but the rule validates if they're [.identifier, .typeidentifier] (https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/ColonRule.swift#L202).

Would you like to try to open a PR to allow two . typeidentifiers? I'm not sure whether this case should be a part of configuration.applyToDictionaries or not.

Collaborator

marcelofabri commented Dec 27, 2016

Yeah, that structure is expected. However, that violation should be caught by the "old" implementation.

In this case, both tokens are .typeidentifiers, but the rule validates if they're [.identifier, .typeidentifier] (https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/ColonRule.swift#L202).

Would you like to try to open a PR to allow two . typeidentifiers? I'm not sure whether this case should be a part of configuration.applyToDictionaries or not.

@koke

This comment has been minimized.

Show comment
Hide comment
@koke

koke Dec 27, 2016

Contributor

I guess looking at the rule description it makes sense:

Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.

The [Void:Void] here is neither an identifier declaration nor a dictionary literal.

Would you like to try to open a PR to allow two . typeidentifiers

Another cases that doesn't match the pattern is let abc: [String:Any], as it'd be [.typeidentifier, .keyword]. I'm not sure if there are more combinations that I'm missing

Contributor

koke commented Dec 27, 2016

I guess looking at the rule description it makes sense:

Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.

The [Void:Void] here is neither an identifier declaration nor a dictionary literal.

Would you like to try to open a PR to allow two . typeidentifiers

Another cases that doesn't match the pattern is let abc: [String:Any], as it'd be [.typeidentifier, .keyword]. I'm not sure if there are more combinations that I'm missing

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Dec 27, 2016

Collaborator

Fair enough. I'll change the label from "bug" to "enhancement" then 😅

Collaborator

marcelofabri commented Dec 27, 2016

Fair enough. I'll change the label from "bug" to "enhancement" then 😅

@marcelofabri marcelofabri added enhancement and removed bug labels Dec 27, 2016

@koke

This comment has been minimized.

Show comment
Hide comment
@koke

koke Dec 28, 2016

Contributor

I've tried adding a few more options to that check:

    // let ↓abc:Void
guard syntaxKinds.starts(with: [.identifier, .typeidentifier])
    // let ↓abc:Any
    || syntaxKinds.starts(with: [.identifier, .keyword])
    // let abc: [↓Void:Void]
    || syntaxKinds.starts(with: [.typeidentifier, .typeidentifier])
    // let abc: [↓Void:Any]
    || syntaxKinds.starts(with: [.typeidentifier, .keyword]) else {
        return false
}

However this is matching a couple other things that I thing shouldn't be matched:

-        case .null:
-            return "Null"
+        case .null: return "Null"
-        let even = ranges.enumerated().flatMap { $0 % 2 == 0 ? $1 : nil }
+        let even = ranges.enumerated().flatMap { $0 % 2 == 0 ? $1: nil }

I've pushed what I have so far to koke@3788392

Contributor

koke commented Dec 28, 2016

I've tried adding a few more options to that check:

    // let ↓abc:Void
guard syntaxKinds.starts(with: [.identifier, .typeidentifier])
    // let ↓abc:Any
    || syntaxKinds.starts(with: [.identifier, .keyword])
    // let abc: [↓Void:Void]
    || syntaxKinds.starts(with: [.typeidentifier, .typeidentifier])
    // let abc: [↓Void:Any]
    || syntaxKinds.starts(with: [.typeidentifier, .keyword]) else {
        return false
}

However this is matching a couple other things that I thing shouldn't be matched:

-        case .null:
-            return "Null"
+        case .null: return "Null"
-        let even = ranges.enumerated().flatMap { $0 % 2 == 0 ? $1 : nil }
+        let even = ranges.enumerated().flatMap { $0 % 2 == 0 ? $1: nil }

I've pushed what I have so far to koke@3788392

@koke

This comment has been minimized.

Show comment
Hide comment
@koke

koke Dec 28, 2016

Contributor

There's also functionName(↓param:value)\n, which is not related to dictionaries, but is also not caught by that rule, as it's [.identifier, .identifier]

Contributor

koke commented Dec 28, 2016

There's also functionName(↓param:value)\n, which is not related to dictionaries, but is also not caught by that rule, as it's [.identifier, .identifier]

@DG0BAB

This comment has been minimized.

Show comment
Hide comment
@DG0BAB

DG0BAB Oct 4, 2017

With Version 0.23 and apply_to_dictionaries: false the colon rule now reports a dictionary type like [String : Any] as an error. If I set apply_to_dictionaries: false, I expect the colon rule not to check dictionaries at all. It worked as expected in 0.21 and 0.22.

DG0BAB commented Oct 4, 2017

With Version 0.23 and apply_to_dictionaries: false the colon rule now reports a dictionary type like [String : Any] as an error. If I set apply_to_dictionaries: false, I expect the colon rule not to check dictionaries at all. It worked as expected in 0.21 and 0.22.

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Oct 4, 2017

Collaborator

@DG0BAB please file a new issue. We've created a new issue template that asks lots of useful questions that would help us fix issues faster, providing critical context and repro steps that drive-by comments on closed issues often doesn't include.

Collaborator

jpsim commented Oct 4, 2017

@DG0BAB please file a new issue. We've created a new issue template that asks lots of useful questions that would help us fix issues faster, providing critical context and repro steps that drive-by comments on closed issues often doesn't include.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment