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

redundant_objc_attribute incorrectly flagging inner @objc enum #2539

Closed
JaviSoto opened this Issue Jan 2, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@JaviSoto
Copy link

JaviSoto commented Jan 2, 2019

The following code triggers redundant_objc_attribute:

@objcMembers 
class Foo {
  @objc enum Bar { }
}

The code to flag a redundant @objc should only trigger if the declaration is a member, not an inner type.

I'm gonna take a stab at fixing this myself.

@JaviSoto

This comment has been minimized.

Copy link

JaviSoto commented Jan 2, 2019

I tried to fix this like this:

diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/RedundantObjcAttributeRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/RedundantObjcAttributeRule.swift
index 1eae87f4..bd82af14 100644
--- a/Source/SwiftLintFramework/Rules/Idiomatic/RedundantObjcAttributeRule.swift
+++ b/Source/SwiftLintFramework/Rules/Idiomatic/RedundantObjcAttributeRule.swift
@@ -38,6 +38,13 @@ public struct RedundantObjcAttributeRule: ASTRule, ConfigurationProviderRule, Au
             }
             """,
             """
+            @objcMembers
+            class Foo {
+              @objc
+              enum Bar { }
+            }
+            """,
+            """
             @objc
             extension Foo {
               var bar: Int {
@@ -142,9 +149,10 @@ public struct RedundantObjcAttributeRule: ASTRule, ConfigurationProviderRule, Au
                          dictionary: [String: SourceKitRepresentable]) -> [StyleViolation] {
         let enclosedSwiftAttributes = Set(dictionary.enclosedSwiftAttributes)
         guard let offset = dictionary.offset,
-              enclosedSwiftAttributes.contains(.objc),
-              !dictionary.isObjcAndIBDesignableDeclaredExtension else {
-            return []
+            enclosedSwiftAttributes.contains(.objc),
+            kind.inheritsObjFromObjCMembers,
+            !dictionary.isObjcAndIBDesignableDeclaredExtension else {
+                return []
         }

         let isInObjcVisibleScope = { () -> Bool in
@@ -167,6 +175,15 @@ public struct RedundantObjcAttributeRule: ASTRule, ConfigurationProviderRule, Au
     }
 }

+private extension SwiftDeclarationKind {
+    /// Whether `@objcMembers` in its parent type applies to this kind of declaration, making it implicitly `@objc`.
+    var inheritsObjFromObjCMembers: Bool {
+        let applicableKinds = SwiftDeclarationKind.variableKinds.union(SwiftDeclarationKind.functionKinds)
+
+        return applicableKinds.contains(self)
+    }
+}
+
 private extension Dictionary where Key == String, Value == SourceKitRepresentable {
     var objcAttributeLocationRanges: RedundantObjcAttributeRule.ObjcAttributeLocations {
         var objcImpliedAttributeLocatioRanges = RedundantObjcAttributeRule.ObjcAttributeLocations()

But this is not correct, because then this avoids flagging things like @objc @NSManaged class Foo.
However, while trying to understand how the implementation works, I discovered another fundamental issue with it, that unfortunately I don't have the time right now to try to fix. My recommendation is to, for the time being, make this rule not be enabled by default, since it has a couple of IMO big issues.

This other issue I was referring to is the following:

@objcMembers 
class Foo {
  class Bar: NSObject { 
    @objc foo: Any
  }
}

I think that the code is right now assuming that the whole range from class Foo { to the closing } of that class to objc visible scope. This is not correct, since @objcMembers doesn't apply to members of inner types.

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