SwiftLint
SwiftLint copied to clipboard
realm
Add RedundantTypeAnnotationConfiguration for ignore_booleans option
- Add
RedundantTypeAnnotationConfigurationforignore_booleansoption - Integrate with
RedundantTypeAnnotationRule - Add
RedundantTypeAnnotationConfigurationTests - Add
RedundantTypeAnnotationRule - Refactor
isFalsePositivelogic for readability
Generated by :no_entry_sign: Danger
An observation for this rule in general: The exception for Bool seems to be quite arbitrary. What about var j: Int = 7 or var d: Double = 1.3 or let s: String = "my string"?
I wonder if we should rather extend the rule for these cases and rename the new option ignore_booleans to ignore_literals.
@revolter: You added the exception for Bool back then. Anything that speaks against my suggestions?
An observation for this rule in general: The exception for
Boolseems to be quite arbitrary. What aboutvar j: Int = 7orvar d: Double = 1.3orlet s: String = "my string"?I wonder if we should rather extend the rule for these cases and rename the new option
ignore_booleanstoignore_literals.@revolter: You added the exception for
Boolback then. Anything that speaks against my suggestions?
Hi @SimplyDanny, thank you for your observation. The examples you gave are not currently triggering examples and booleans are the only literals that currently violate this rule. It makes sense that numeric and string literals are not violations since you may need to be explicit that the type in let foo = 0 is actually an Int, Double, or Float. Or, in let foo = "someString", you may need to be explicit that foo is a String or StaticString. This is why we made the determination to implement this as ignore_booleans instead of something like ignore_literals.
Well, without the explizit types, the variables j, d and s would have the same type as specified. They are the defaults.
The current implementation checks exactly for the combinations Bool = true and Bool = false. The same could be done for Int = <any integer>, Double = <any float>, ...
Moreover, a variable being assigned a Boolean literal is not necessarily a Bool. That also works for types conforming to ExpressibleByBooleanLiteral.
The exception for Bool is still not clear to me yet.
Well, without the explizit types, the variables
j,dandswould have the same type as specified. They are the defaults.The current implementation checks exactly for the combinations
Bool = trueandBool = false. The same could be done forInt = <any integer>,Double = <any float>, ...Moreover, a variable being assigned a Boolean literal is not necessarily a
Bool. That also works for types conforming toExpressibleByBooleanLiteral.The exception for
Boolis still not clear to me yet.
Thank you for your reply @SimplyDanny. The original intent for the special handling of Bool is not discernible from the PR or the issue.
My guess is that we would not want any of the following to be violations because, in certain cases, we might need to be explicit about the type:
let someInt: Int = 0
let someDouble: Double = 0
let someFloat: Float = 0
let someString: String = "foo"
let someStaticString: StaticString = "bar"
let someCustomBool: CustomBool = false
However, in the case of Bool, we do not need to be explicit when defining one because Bool is the only conforming type to ExpressibleByBooleanLiteral in the Swift Standard Library.
https://developer.apple.com/documentation/swift/expressiblebystringliteral#conforming-types https://developer.apple.com/documentation/swift/expressiblebyintegerliteral#conforming-types https://developer.apple.com/documentation/swift/expressiblebyfloatliteral#conforming-types
Granted, you can create your own type that is ExpressibleByBooleanLiteral (like CustomBool above), but in that case you would want to be explicit about the type and you would not want the rule to trigger a violation.
Although I think your point is a valuable one, I believe it is outside the scope of this PR and so I'd like to suggest the following way forward:
- Move ahead with this PR as is
- Separately from this PR, other PRs in the future could consider removing custom handling of booleans or alternatively adding custom handling for all literal types
Although I think your point is a valuable one, I believe it is outside the scope of this PR and so I'd like to suggest the following way forward:
- Move ahead with this PR as is
- Separately from this PR, other PRs in the future could consider removing custom handling of booleans or alternatively adding custom handling for all literal types
That might be an acceptable way to go. However, I still take exception to the option's name ignore_booleans which is obviously very specific for Booleans. Changing an options name later on might cause trouble for users, unfortunately. That's why I'm still opposed to just adding it immediately.
Reading through the comments, it really feels like the decision to have let someBool: Bool = true violate a rule named RedundantTypeAnnotation was possibly not the best decision in the first place, and is likely the cause for some confusion I am seeing in the comments.
I would like to reiterate that var j: Int = 7, var d: Double = 1.3 and let s: String = "my string" do NOT violate RedundantTypeAnnotation. For some reason only the boolean check was added to RedundantTypeAnnotation (and hence why this PR is proposing only adding the exception for booleans – there would be no reason for the option to be ignore_literals since the other literals are not violations).
I would like to kindly suggest an alternative way forward:
- Revise this pull request to instead completely remove the boolean check from
RedundantTypeAnnotation. - Submit another pull request to add a new opt-in rule to treat boolean literals with explicit type annotations as violations named
BooleanLiteralTypeAnnotation– orLiteralTypeAnnotationif it is preferred that all literals with explicit types are violations.
With this approach, there is no need for a configuration option at all.
@SimplyDanny Would this alternative approach be accepted? And do you prefer BooleanLiteralTypeAnnotation or LiteralTypeAnnotation for the new rule?
Thank you 🙏
For clarity: I was the one that requested this (in #3423), but I didn't make any code change for it.
As far as the current situation goes, I would be fine with reverting that change completely.
Boolis the only conforming type toExpressibleByBooleanLiteralin the Swift Standard Library
but this can change in the future, so I would say that considering
let test: Bool = true
to be redundant is a guess, while considering
let test: URL = URL()
to be redundant is a certainty.
I'd like to avoid exceptions for certain cases that actually should follow the same rules as others. Breaking changes are acceptable if they really fix an inconstancy or harmonize implementations.
The exception for booleans bugs me, with or without the ignore_booleans option.
Types can conform to any ExpressibleBy...Literal to make them be initializable with the corresponding literal. But all literals are as well associated with a default type. So if the rule talks about "redundant types" I'd consider explicit default types for literals redundant. Booleans follow the same logic; I don't see why there ought to be any special handling for them (as of now or with an option).
Since there might be people not agreeing on the statement that default types can be considered redundant, my favorite is still this ignore_literals or perhaps the better consider_default_literal_types_redundant to be more specific. With that set to true,
let k: Int = 0
let d: Double = 0.0
let s: String = ""
let b: Bool = false
would all trigger while
let k: Int8 = 0
let d: Double = 0
let s: Name = ""
let b: OnOff = true
would not.
@SimplyDanny Thank you for weighing in. Very much appreciated 👍
So, to confirm, you are suggesting that on this PR, the ignore_booleans option be renamed to consider_default_literal_types_redundant, defaulted to true, is that correct?
As far as treating Int, Double and String literals as redundant, are you suggesting that this capability is also added on this PR as well? Asking since those are not treated as redundant currently and we are seeking to have this PR merged in sooner than later, so would like to optimize the changes for your approval. Please confirm. Thank you!
So, to confirm, you are suggesting that on this PR, the
ignore_booleansoption be renamed toconsider_default_literal_types_redundant, defaulted totrue, is that correct?
Yes, that's what feels most reasonable to me. However, the default should be false so that for most types/literals this doesn't change the rule's behavior - only for Bool in that the rule with default settings wouldn't trigger on let b: Bool = true while it currently does. For people already having the rule active, no surprising behavioral changes would come with an update to a new SwiftLint version in that way.
Any noticeable flaw in my reasoning?
@SimplyDanny Sounds good 👍 … consider_default_literal_types_redundant will default to false. Thank you for confirming that.
And may treating Int, Double and String literals as redundant be added on a separate PR (potentially by someone else), to limit the scope of the change on this PR?
And may treating
Int,DoubleandStringliterals as redundant be added on a separate PR (potentially by someone else), to limit the scope of the change on this PR?
Well, ideally we would have all types properly supported right from the new option's introduction on. 😉
@SimplyDanny Thanks again for all the feedback. I investigated adding validations for Int, Double and String literals and determined that an overhaul of the rule, potentially adopting SwiftSyntax, would likely be required. Since that would significantly increase the scope and complexity of the PR, I propose that the PR is limited to adding the new config option only. I've updated to use the option renamed as you requested, and it's working really well! My hope is that you will consider merging this as is and allow the additional type support to be added later. I look forward to hearing back from you. Thank you 😄
@SimplyDanny Thanks again for all the feedback. I investigated adding validations for Int, Double and String literals and determined that an overhaul of the rule, potentially adopting SwiftSyntax, would likely be required. Since that would significantly increase the scope and complexity of the PR, I propose that the PR is limited to adding the new config option only. I've updated to use the option renamed as you requested, and it's working really well! My hope is that you will consider merging this as is and allow the additional type support to be added later. I look forward to hearing back from you. Thank you 😄
The rewrite with SwiftSyntax is now done with #5389 just being merged. I would still appreciate you implementing all cases to have this in a complete and reasonable shape. With the rewrite, this ought not to be that complex. Otherwise, we need to wait for me or another contributor to find the time to address this.
@SimplyDanny The rewrite of the rule with SwiftSyntax makes this much more straightforward! Very cool! 👍
@garricn Thank you for implementing this! ❤️
I do have one question though, which is whether we need to indicate somewhere that
Boolliterals will no longer be considered redundant by default. Since this is a change in behavior, does it need to be listed as a "breaking" change?
Yes, I'd like that.
@SimplyDanny Thanks for all your guidance and attention on this! Very much appreciated ⭐
@garricn Great work! Thank you again for making this improvement.
Just to let you guys know: I made a few cleanups to reduce some duplications in #5554. Didn't want to prolong the review here while being pedantic. 😅
I also enabled the option directly in SwiftLint itself (#5555).