Skip to content
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

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Sep 6, 2023

…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).

@xedin xedin requested a review from allevato September 6, 2023 18:27
@xedin
Copy link
Contributor Author

xedin commented Sep 6, 2023

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<Int>() or [] as [Int].

Copy link
Member

@allevato allevato left a 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 {
Copy link
Member

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).

Copy link
Contributor Author

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@xedin
Copy link
Contributor Author

xedin commented Sep 6, 2023

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 .self in expression context elsewhere though, this is a very narrow special case we have to support, so it might be good to nudge people to switch to Swift spelling.

@xedin
Copy link
Contributor Author

xedin commented Sep 6, 2023

cc @airspeedswift

@allevato
Copy link
Member

allevato commented Sep 6, 2023

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 var someVar = someExpression for simple local variables that now there's an exception for empty collections where they're expected to use a type annotation or as cast.

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.)

@xedin
Copy link
Contributor Author

xedin commented Sep 6, 2023

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 .self.

@xedin
Copy link
Contributor Author

xedin commented Sep 6, 2023

I am fine making this opt-in though.

@xedin xedin force-pushed the upstream-literal-array-init-rewrite branch from 9f33cf3 to d7bc8a1 Compare September 6, 2023 20:20
@xedin xedin requested a review from allevato September 8, 2023 22:11
@xedin
Copy link
Contributor Author

xedin commented Sep 8, 2023

@allevato I think I addressed all of the feedback and I've also extended the rule to parameters with default values.

@mlavergn
Copy link
Contributor

+1 for this PR, var birds: [Bird] = [] is how Apple's code samples initialize empty collections. See: https://developer.apple.com/documentation/swiftui/backyard-birds-sample

/// 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 {
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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 🙂

Copy link
Contributor Author

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 {
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@xedin xedin force-pushed the upstream-literal-array-init-rewrite branch from e1849ee to 9ab323e Compare September 14, 2023 14:07
@xedin
Copy link
Contributor Author

xedin commented Sep 14, 2023

@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.

Copy link
Member

@allevato allevato left a 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. 😁

@allevato allevato merged commit 9362431 into swiftlang:main Sep 14, 2023
allevato added a commit to allevato/swift-format that referenced this pull request Sep 14, 2023
…it-rewrite

 [Lint] Add a rule to detect and transform `[<Type>]()` into literal …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants