RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes #5389

tonell-m · 2023-12-11T21:13:59Z

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!

SwiftLintBot · 2023-12-11T21:37:21Z

	19 Warnings
⚠️	This PR introduced a violation in Brave: /Sources/Growth/GrowthPreferences.swift:15:30: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Firefox: /firefox-ios/Client/Frontend/Widgets/SiteTableViewController.swift:30:13: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Firefox: /firefox-ios/Storage/ThirdParty/SwiftData.swift:328:19: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Kickstarter: /Kickstarter-iOS/Features/ProjectPage/Controller/ProjectPageViewControllerTests.swift:1057:26: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Kickstarter: /Library/ViewModels/ProjectNavigationSelectorViewModelTests.swift:33:64: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in NetNewsWire: /Account/Sources/Account/Folder.swift:24:26: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Realm: /RealmSwift/Tests/CustomColumnNameTests.swift:827:23: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Realm: /RealmSwift/Tests/CustomColumnNameTests.swift:865:23: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Sourcery: /SourceryTests/Stub/Performance-Code/Kiosk/App/Networking/Networking.swift:120:25: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/private/StdlibUnittest/RaceTest.swift:439:28: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/private/StdlibUnittest/StdlibUnittest.swift:992:27: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/private/StdlibUnittest/StdlibUnittest.swift:994:28: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/private/StdlibUnittest/StdlibUnittest.swift:996:28: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/private/SwiftPrivate/IO.swift:38:23: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/public/Backtracing/Context.swift:981:12: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/public/core/ThreadLocalStorage.swift:87:13: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Swift: /stdlib/public/libexec/swift-backtrace/TargetMacOS.swift:417:12: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in VLC: /Sources/Playback/Player/VideoPlayer-iOS/VideoPlayerViewController.swift:2218:33: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
⚠️	This PR introduced a violation in Wire: /wire-ios/Wire-iOS/Sources/Helpers/ColorScheme.swift:245:25: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)

	34 Messages
📖	Linting Aerial with this PR took 0.8s vs 0.82s on main (2% faster)
📖	Linting Alamofire with this PR took 1.13s vs 1.14s on main (0% faster)
📖	Linting Brave with this PR took 6.5s vs 6.52s on main (0% faster)
📖	Linting DuckDuckGo with this PR took 3.68s vs 3.63s on main (1% slower)
📖	Linting Firefox with this PR took 9.41s vs 9.37s on main (0% slower)
📖	Linting Kickstarter with this PR took 7.95s vs 8.42s on main (5% faster)
📖	Linting Moya with this PR took 0.49s vs 0.48s on main (2% slower)
📖	Linting NetNewsWire with this PR took 2.41s vs 2.47s on main (2% faster)
📖	Linting Nimble with this PR took 0.66s vs 0.66s on main (0% slower)
📖	Linting PocketCasts with this PR took 6.92s vs 7.01s on main (1% faster)
📖	Linting Quick with this PR took 0.32s vs 0.33s on main (3% faster)
📖	Linting Realm with this PR took 4.31s vs 4.16s on main (3% slower)
📖	Linting Sourcery with this PR took 2.07s vs 2.05s on main (0% slower)
📖	Linting Swift with this PR took 3.91s vs 3.94s on main (0% faster)
📖	Linting VLC with this PR took 1.1s vs 1.1s on main (0% slower)
📖	Linting Wire with this PR took 14.59s vs 14.36s on main (1% slower)
📖	Linting WordPress with this PR took 9.5s vs 9.45s on main (0% slower)
📖	This PR fixed a violation in Aerial: /Aerial/Source/Models/Cache/VideoDownload.swift:239:25: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Aerial: /Resources/MainUI/VideosViewController.swift:224:20: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Brave: /Sources/Brave/Frontend/Browser/Tabs/TabTray/Views/TabTrayCell.swift:33:14: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Brave: /Sources/Brave/Frontend/Settings/Debug/Rewards Internals/QA/RewardsDebugSettingsViewController.swift:127:17: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Brave: /Sources/BraveWallet/Chart/LineChartView.swift:58:12: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in DuckDuckGo: /PacketTunnelProvider/ProxyServer/Utils/UInt128.swift:453:28: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Swift: /stdlib/public/Windows/WinSDK.swift:186:15: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios-data-model/Source/Utilis/CryptoBox.swift:105:34: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios-sync-engine/Source/Data Model/Conversation+MessageDestructionTimeout.swift:94:28: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios/Wire-iOS/Sources/UserInterface/CustomAppLock/Setup/PasscodeError.swift:53:29: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios/Wire-iOS/Sources/UserInterface/Helpers/UIView+Zeta.swift:40:27: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios/Wire-iOS/Sources/Vendor/SCSiriWaveformView.swift:157:23: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in Wire: /wire-ios/WireCommonComponents/FileMetaDataGenerator.swift:87:18: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in WordPress: /WordPress/Classes/ViewRelated/Reader/OldReaderPostCardCell.swift:229:36: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in WordPress: /WordPress/Classes/ViewRelated/Reader/OldReaderPostCardCell.swift:230:35: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in WordPress: /WordPress/Classes/ViewRelated/Reader/ReaderSearchViewController.swift:247:24: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)
📖	This PR fixed a violation in WordPress: /WordPress/Classes/ViewRelated/Themes/ThemeBrowserViewController.swift:198:24: warning: Redundant Type Annotation Violation: Variables should not have redundant type annotation (redundant_type_annotation)

Generated by 🚫 Danger

Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift

SimplyDanny

Thanks for taking up this rewrite! This is actually a bit more involved than adding the option for ignored attributes only. So I have a few comments ...

CHANGELOG.md

Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift

...ce/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantTypeAnnotationConfiguration.swift

garricn · 2023-12-16T06:46:08Z

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.

tonell-m · 2023-12-17T12:08:47Z

@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 :)

CHANGELOG.md

Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift

CHANGELOG.md

Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift

SimplyDanny · 2024-01-03T21:40:25Z

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?

CHANGELOG.md

tonell-m · 2024-01-05T11:07:49Z

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

SimplyDanny · 2024-01-05T22:05:46Z

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

That depends on what the previous implementation did. I guess, it triggered, hence the rewritten rule should trigger too.

…ributes - Add an `ignoreAttributes` configuration option that defaults to ["IBInspectable"] for retro compatibility - Check for each node's attributes to see if they are contained in the ignore list

…essions E.g. var a: Int = Int.random(0...1)

…edge cases manually

SimplyDanny · 2024-04-21T15:12:21Z

That depends on what the previous implementation did. I guess, it triggered, hence the rewritten rule should trigger too.

Rebased and addressed this point.

tonell-m force-pushed the feature/add-attributes-ignore-config-to-redundanttypeannoations branch 2 times, most recently from 9c1904c to 72e1850 Compare December 12, 2023 16:10

Harry-KNIGHT reviewed Dec 13, 2023

View reviewed changes

Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantTypeAnnotationRule.swift Show resolved Hide resolved

SimplyDanny requested changes Dec 15, 2023

View reviewed changes

garricn reviewed Dec 16, 2023

View reviewed changes

...ce/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantTypeAnnotationConfiguration.swift Outdated Show resolved Hide resolved

SimplyDanny requested changes Dec 17, 2023

View reviewed changes

tonell-m force-pushed the feature/add-attributes-ignore-config-to-redundanttypeannoations branch from cd5eebf to ca246f5 Compare December 21, 2023 10:52

SimplyDanny requested changes Dec 21, 2023

View reviewed changes

SimplyDanny reviewed Jan 3, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

SimplyDanny mentioned this pull request Feb 9, 2024

Rewrite redundant_type_annotation with SwiftSyntax #4443

Closed

This was linked to issues Feb 9, 2024

Redundant type annotation rule ignores nested types #3146

Closed

redundant_type_annotation ignores arrays #3141

Closed

tonell-m added 12 commits April 21, 2024 16:45

Rewrite RedundantTypeAnnotationRule using SwiftSyntax

fc5fbe0

Add the possibility to ignore properties marked with configurable att…

d66f76d

…ributes - Add an `ignoreAttributes` configuration option that defaults to ["IBInspectable"] for retro compatibility - Check for each node's attributes to see if they are contained in the ignore list

Finetune implementation so that all tests pass

5e0f7e4

Fix configuration key to match property name

2c96fa6

Only flag Bool literal initializer as a redundancy

c6c827a

Break down line with too much characters

7e4d872

Take generic arguments into account when comparing types

6098f44

Fix linting errors

e83c67a

Add changelog entry

883d054

Add support of function call initializers that are member access expr…

0a81d0d

…essions E.g. var a: Int = Int.random(0...1)

Add some extra examples for edge cases testing

536e0fd

Add handling of force unwrapped initializers

9c100ad

tonell-m and others added 21 commits April 21, 2024 16:45

Remove trailing comma

2aa1778

Update changelog to only mention the new option

6f9d00c

Use @SwiftSyntaxRule and remove explicit makeVisitor implementation

8c67f8b

Move description back to the main class

609e8cd

Use binding nodes instead of variable declarations to avoid handling …

181ebda

…edge cases manually

Add a more in depth handling of generics

250607b

Rename ignored_annotations to ignore_attributes

703c157

Fix linter issues

7f4f758

Use PatternBindingSyntax parent's parent to check for attributes

4b1b2aa

Add tests for ignore_attributes configuration

9b08bc4

Use ExprSyntax as node type for isMemberAccess

ccd5c4e

Add credit in changelog for rewriting rule with SwiftSyntax

662b833

Fixed incorrect syntax in Examples

12d67df

Removed unused configuration parameters

c31d9f3

Changelog tweaks

5112e52

Added two spaces in Changelog to ensure linebreak

de51795

Handle chained MemberAccessExprSyntax in variable declarations

8083bb4

Adapt to current state of code base

fb74996

Support function calls in the access chain

8953de9

Let visitor define corrections

66d9cee

Use if instead of guard

fec3ea9

SimplyDanny force-pushed the feature/add-attributes-ignore-config-to-redundanttypeannoations branch from 8771e14 to fec3ea9 Compare April 21, 2024 15:11

Fix changelog

1830e3e

SimplyDanny approved these changes Apr 21, 2024

View reviewed changes

Add new attribute to test reference

c96aa2f

SimplyDanny mentioned this pull request Apr 21, 2024

Rewrite redundant_type_annotation with SwiftSyntax #5535

Closed

SimplyDanny merged commit b3dd890 into realm:main Apr 21, 2024
12 checks passed

SimplyDanny mentioned this pull request Apr 21, 2024

Add consider_default_literal_types_redundant option to RedundantTypeAnnotationRule #4756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes #5389

RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes #5389

tonell-m commented Dec 11, 2023

SwiftLintBot commented Dec 11, 2023 •

edited

Loading

SimplyDanny left a comment

garricn commented Dec 16, 2023

tonell-m commented Dec 17, 2023

SimplyDanny commented Jan 3, 2024

tonell-m commented Jan 5, 2024

SimplyDanny commented Jan 5, 2024

SimplyDanny commented Apr 21, 2024

RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes #5389

RedundantTypeAnnotationRule: Rewrite with SwiftSyntax and allow ignoring certain attributes #5389

Conversation

tonell-m commented Dec 11, 2023

SwiftLintBot commented Dec 11, 2023 • edited Loading

SimplyDanny left a comment

Choose a reason for hiding this comment

garricn commented Dec 16, 2023

tonell-m commented Dec 17, 2023

SimplyDanny commented Jan 3, 2024

tonell-m commented Jan 5, 2024

SimplyDanny commented Jan 5, 2024

SimplyDanny commented Apr 21, 2024

SwiftLintBot commented Dec 11, 2023 •

edited

Loading