SwiftLint
SwiftLint copied to clipboard
realm
RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes
Hi, first time contributing here so just let me know if something is wrong or missing in my PR 🙂
This PR resolves #5366 by adding a new ignored_annotations configuration to this rule that accepts a set of strings listing annotations that should disable this rule for properties that are marked with it. Currently this is already done arbitrarily for @IBInspectable but now it is configurable. To solve the linked issue the configuration would be:
redundant_type_annotations:
ignored_attributes:
- Parameter
This configuration still defaults to only IBDesignable for retrocompatibility.
And, while I was at it and as @SimplyDanny suggested in the issue's comments, I took the opportunity to rewrite this rule using SwiftSyntax. Let me know what you think!
Generated by :no_entry_sign: Danger
I'm really excited about this work! After this merges, perhaps we can also introduce another configuration element named ignore_literals which can address the issues raised by this PR as well as the concerns of @SimplyDanny therein in terms of not limiting the configuration element to just ignoring booleans.
@SimplyDanny Thanks for the review!
After your comments I've rewrote the rule with a new approach that seems to be more flexible. Instead of visiting VariableDeclSyntax nodes I use PatternBindingSyntax and OptionalBindingConditionSyntax as entry points and moved the logic into a TypeAnnotationSyntax extension. That way it will check for all binding patterns even if there are multiple for a single var/let keyword, and it also works the same of guard/if clauses.
I've also improved the handling of generics to handle more complex cases as you suggested, so var s: Set = Set<Int>([]) will trigger and not var s: Set<Int> = Set([]). I've added a bunch of tests with various configurations to ensure that this is working as expected, let me know if you think of other cases that haven't been handled here :)
Looking at the OSS findings, most of them look valid.
However, there are cases where a sequence of MemberAccessExpressions leads to the final element/function. An easy example would be let a: A = A.b.c They were previously discovered by the rule, so they should also be discoverable by the rewritten rule.
Admittedly, these cases are debatable, but for the sake of a rewrite, the rule should behave as close to the previous version as it can.
@tonell-m: Would you consider these cases in your PR?
Looking at the OSS findings, most of them look valid.
However, there are cases where a sequence of MemberAccessExpressions leads to the final element/function. An easy example would be let a: A = A.b.c They were previously discovered by the rule, so they should also be discoverable by the rewritten rule.
Admittedly, these cases are debatable, but for the sake of a rewrite, the rule should behave as close to the previous version as it can.
@tonell-m: Would you consider these cases in your PR?
@SimplyDanny This case is now handled as well. Do you think we also need to handle cases where there is a mix of MemberAccessExprSyntax and FunctionCallExprSyntax? Like let a: A = A.f().b
Looking at the OSS findings, most of them look valid. However, there are cases where a sequence of MemberAccessExpressions leads to the final element/function. An easy example would be let a: A = A.b.c They were previously discovered by the rule, so they should also be discoverable by the rewritten rule. Admittedly, these cases are debatable, but for the sake of a rewrite, the rule should behave as close to the previous version as it can. @tonell-m: Would you consider these cases in your PR?
@SimplyDanny This case is now handled as well. Do you think we also need to handle cases where there is a mix of
MemberAccessExprSyntaxandFunctionCallExprSyntax? Likelet a: A = A.f().b
That depends on what the previous implementation did. I guess, it triggered, hence the rewritten rule should trigger too.
That depends on what the previous implementation did. I guess, it triggered, hence the rewritten rule should trigger too.
Rebased and addressed this point.