-
Notifications
You must be signed in to change notification settings - Fork 236
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
[Lint] Add a rule to detect and transform [<Type>]()
into literal …
#617
[Lint] Add a rule to detect and transform [<Type>]()
into literal …
#617
Conversation
We can do this in expression context as well i.e. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule feels pretty opinionated. Is there some consensus/discussion you can point to that [Type]()
is considered archaic? Off the top of my head, I can't think of any community discussions about it.
We should probably make this rule opt-in at a minimum so people can choose whether they want to use it.
We can do this in expression context as well i.e. reduce(Int) { ... } but I'm not sure what to use as a replacement it could be either Array() or [] as [Int].
Replacing with Array<Int>()
would directly contradict UseShorthandTypeNames
. [] as [Int]
would keep that compatibility but feels like a downgrade from [Int]()
(in my opinion) but I suppose it would also be consistent with the use of as
in other places in Swift where there's not enough context to resolve a type generically.
/// Lint: Non-literal empty array initialization will yield a lint error. | ||
/// Format: All invalid use sites would be related with empty literal (with or without explicit type annotation). | ||
@_spi(Rules) | ||
public final class AlwaysUseLiteralForEmptyArrayInit : SyntaxFormatRule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add support for dictionaries as well? The rule could be named AlwaysUseLiteralForEmptyCollections
(I don't think we need Init
in the name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into that sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
This syntax is supported because that's how this operation looked in Objective-C which made sense at the beginning. The language doesn't support uses of type names without |
Sure, if there are compelling reasons to try to nudge folks toward a specific spelling, then I'm happy to have that discussion—I just want to make sure I have all the context first. (That's why making the rule opt-in at first is totally fine.) I might just be hesitant because it's the syntax I've been using for ages 🙂 But this does introduce an explicit inconsistency for initialization of empty collections, so I want to make sure we're doing the right thing style-wise. For example, using explicit type annotations for type members is probably always a good thing (for readability and for type checker performance), but it feels harder to tell users who usually do So I guess what I'm looking for is a good rationale for that inconsistency, one that isn't just about a special case in the compiler. (Although if you look at the complexity of the implementation of UseShorthandTypeNames, I feel that pain too.) |
I think the main motivation here is that it’s inconsistent special case in the surface language which is not supported in any other constructs but this bracket syntax, the spelling of type references in general cannot appear without other an operation or |
I am fine making this opt-in though. |
9f33cf3
to
d7bc8a1
Compare
@allevato I think I addressed all of the feedback and I've also extended the rule to parameters with default values. |
+1 for this PR, |
/// Lint: Non-literal empty array initialization will yield a lint error. | ||
/// Format: All invalid use sites would be related with empty literal (with or without explicit type annotation). | ||
@_spi(Rules) | ||
public final class AlwaysUseLiteralForEmptyCollectionInit : SyntaxFormatRule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need Init
in the name here? Not that other rule names aren't already a mouthful, but it seems like we could drop it and have the same meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s useful to have it in the name to indicate that this is for initialization only, but I am open to suggestions if you have a better name in mind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of anything better off the top of my head, so if you think it's helpful to disambiguate, I'm ok with it 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always change it later if something better comes up :)
|
||
@_spi(Rules) import SwiftFormat | ||
|
||
final class AlwaysUseLiteralForEmptyCollectionInitTests: LintOrFormatRuleTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a couple examples in the tests using parameterized initializers (e.g., [Int](repeating: 0, count: 10)
for arrays) just to show that they are preserved and prevent future regressions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
…iteral array init Instead of using an archaic initializer call syntax, let's replace it with an empty array literal with a type annotation (if there isn't one).
…UseLiteralForEmptyCollectionInit`
…er with default values
…doesn't affect other cases
e1849ee
to
9ab323e
Compare
@allevato Added tests are requested although arguably we should suggest to replace them with leading-dot syntax, I've made a note of that in the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
The cynic in me slightly worries that if someone writes extension Array { func callAsFunction() { ... } }
, this will break them if they write let x = [y]()
, but then the other half of me thinks they deserve to be broken for writing that in the first place. 😁
…it-rewrite [Lint] Add a rule to detect and transform `[<Type>]()` into literal …
…array init
Instead of using an archaic initializer call syntax, let's replace it with
an empty array literal with a type annotation (if there isn't one).