nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.63k stars 623 forks source link

Add `—nilinit` for `redundantNilInit` rule #1680

Closed rakuyoMo closed 2 months ago

rakuyoMo commented 2 months ago

Related https://github.com/nicklockwood/SwiftFormat/issues/1677 .

Added —nilinit option to redundantNilInit rule, its value includes insert and remove. Among them, remove is consistent with the previous logic, and insert is a new logic. It will add nil as the default value after the optional value type according to the opposite rules of remove.

I did not implement the properties-only option mentioned in the issue, firstly because I did not need it, and secondly because I was not familiar enough with this project.

I added documentation and test cases and all of them passed.

This is my first PR for this project. If I did something wrong or if there is anything else that needs to be modified, please let me know.

rakuyoMo commented 2 months ago

I fixed some issues and can now start the review.

nicklockwood commented 2 months ago

Thank you - this looks excellent. One thing I didn't previously consider is the case where the value is already being initialized in the init method, e.g.

class Foo {
    var foo: Int?// probably don't want to add = nil here?

    init() {
        foo = nil
    }
}

I checked and I don't think setting the value to nil twice will actually cause any compilation problems or changes in behavior, so it's probably fine to consider handling this an enhancement rather than a blocker for the feature though.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.18%. Comparing base (4e3bd75) to head (bd94a43). Report is 1 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1680 +/- ## =========================================== + Coverage 92.81% 95.18% +2.36% =========================================== Files 20 20 Lines 21162 22964 +1802 =========================================== + Hits 19642 21858 +2216 + Misses 1520 1106 -414 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rakuyoMo commented 2 months ago

One thing I didn't previously consider is the case where the value is already being initialized in the init method.

If there is a rule in the future (I'm not sure if there is one now): "All properties that do not rely on external input should have their initial values set when they are defined, rather than set in the init method" (Let’s call it define-init), then this problem will become simpler.

In my opinion, the correct solution to the situation you proposed is as follows:

// --nilinit insert

class Foo {
-  var foo: Int?
+  var foo: Int? = nil

  // If init is empty, the entire init should also be removed. 
  // But I remember that there seems to be another rule that can automatically remove empty methods. 
  // I'm not sure how SwiftFormat handles the same parts between the two rules, is it implemented twice?
  //
  // Considering that the `remove` option does not handle the situation of `var foo: Optional<Int> = nil` 
  // (there are other rules to process `Optional<Int>` as `Int?`), 
  // so I guess there should be no need to delete the empty init method here?
  init() {
-    foo = nil
  }
}

I'm not currently dealing with the init internals and I'd like to hear your thoughts. If define-init exists, then --nilinit insert does not need to consider this situation? Or should we consider this situation anyway.


After I implement the above logic, properties-only or variables-only are still not needed for me. I'm sorry that I may not have the time or energy to implement it, unless you tell me that these two options will be necessary conditions for insert, or you have any other suggestions for how I can implement the above.

rakuyoMo commented 2 months ago

Please let me list other situations:

// --nilinit insert

class Foo {
-  var foo: Int?
+  var foo: Int? = nil

  init() {
-    foo = nil

-    var foo: String?
+    var foo: String? = nil
  }
}
// --nilinit insert-only-properties

class Foo {
-  var foo: Int?
+  var foo: Int? = nil

  init() {
-    foo = nil

    var foo: String? // nil will not be automatically inserted
  }
}
// --nilinit insert-only-variables

class Foo {
  var foo: Int? // nil will not be automatically inserted

  init() {
    foo = nil

-    var foo: String?
+    var foo: String? = nil
  }
}
nicklockwood commented 2 months ago

@rakuyoMo thanks for the update - I was not actually expecting you to fix this!

After I implement the above logic, properties-only or variables-only are still not needed for me. I'm sorry that I may not have the time or energy to implement it, unless you tell me that these two options will be necessary conditions for insert, or you have any other suggestions for how I can implement the above.

This is absolutely fine not to include.