nicklockwood / SwiftFormat

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

`redundantStaticSelf` removes required `Self` in extensions. #1510

Closed lhunath closed 1 year ago

lhunath commented 1 year ago

Consider:

public extension ButtonStyle where Self == RoundedButtonStyle {
    static func hsRounded(style: Self.Style = .plain) -> Self {
        Self(style: style)
    }
}
public struct RoundedButtonStyle: ButtonStyle {
    public var style:         Style

    public enum Style {
        case plain
    }

    public func makeBody(configuration: Configuration) -> some View {
        EmptyView()
    }
}

SwiftFormat's latest redundantStaticSelf rule causes Self.Style = .plain to be rewritten as Style = .plain. However, this results in invalid code since now Style cannot be resolved anymore, the Self reference is required in this case. This leads to the formatter breaking legal code.

$ swiftformat --version
0.52.0
nicklockwood commented 1 year ago

@lhunath I think this is already fixed in 0.52.1

lhunath commented 1 year ago

oh - I'll try again when brew updates.

soranoba commented 1 year ago

Hi, 0.52.1 is broken.

  1> class A { static let defaultName = "A"; let name: String; init() { name = Self.defaultName } } 
  2> class A { static let defaultName = "A"; let name: String; init() { name = defaultName } } 
expression failed to parse:
error: repl.swift:2:75: error: static member 'defaultName' cannot be used on instance of type 'A'
class A { static let defaultName = "A"; let name: String; init() { name = defaultName } }
                                                                          ^~~~~~~~~~~
                                                                          A.

0.52.1 will format (1) to (2), so it failed to compile.

guillaumealgis commented 1 year ago

I'm seeing this too in our project (0.52.1 removing Self. where it shouldn't), but I'm struggling to reproduce in a simple out-of-project file.

Will keep you updated if I can repro.

vgorloff commented 1 year ago

+1 on our side.

Formatting causes removal of the Self in expression guard let value = Self.location(for: warnRegion) ... which causes compile error.

Here is our code before formatting:

public class LocationDeeplink: Deeplink {

   convenience init?(warnRegion: String) {

      guard let value = Self.location(for: warnRegion) else {
         return nil
      }
      self.init(location: value)
   }
}

extension LocationDeeplink {

   private static func location(for warnRegion: String) -> WCOMLocation? {
      return ...
   }
}
nicklockwood commented 1 year ago

@soranoba I wasn't able to reproduce your issue with 0.52.1, but I was able to reproduce it with 0.52.0. Are you sure you're on the latest? Run swiftformat --version if you aren't sure.

nicklockwood commented 1 year ago

@vgorloff this is a different, but related bug. I'll fix it for the next release.

nicklockwood commented 1 year ago

@guillaumealgis if you are able to repro, please open a separate ticket. If you are able to share the entire file or some significant chunk of it, that will be fine - it doesn't need to compile.

soranoba commented 1 year ago

I use 0.52.1, but it does not happend to bug with this code. Sorry... Attached is the code from when it actually happened.

public struct Attributes: OptionSet, Decodable {
    public let rawValue: UInt

    public init(rawValue: UInt) {
        self.rawValue = rawValue
    }

    public init(from decoder: Decoder) throws {
        var container = try decoder.unkeyedContainer()
        var result: Attributes = []
        while !container.isAtEnd {
            let name = try container.decode(String.self)
            if let type = Self.mapping[name] { // <--- here
                result.insert(type)
            }
        }
        self = result
    }

    private static let mapping: [String: Attributes] = [
    ]
}
soranoba commented 1 year ago

And it reproduced. The if statement may be suspicious.

public class Store {
    private static let kUserName: String? = "USER_NAME"

    @objc lazy var userName: String = {
        if let name = Self.kUserName { // <---- here
            return name
        }
        return ""
    }
}
FelixLisczyk commented 1 year ago

I also have a few cases where Self is removed from my code. Here is an example:

protocol ContentDetectionConfiguration {
    static var contentTypes: [ContentType] { get }
}

struct ContentDetectionConfigurationDelegate: ContentDetectionConfiguration {
    private static var configurator = ContentTypeConfigurator()

    func isEnabled(at index: Int) -> Bool {
        let contentType = Self.contentTypes[index] // <-- here
        return Self.configurator.isEnabled(contentType: contentType)
    }
}

This only happens if I specify a Swift version, e.g. swiftformat --swiftversion 5.8 file.swift

swiftformat --version
0.52.1
nicklockwood commented 1 year ago

@soranoba OK cool, it seems to be the same issue as @vgorloff's. Basically it happens if Self is used inside let or if/guard let expressions.

FelixLisczyk commented 1 year ago

Version 0.52.2 fixes this issue in my project. Thank you! 👍

soranoba commented 1 year ago

Version 0.52.2 fixes my issue too. Thanks!!

vgorloff commented 1 year ago

Version 0.52.2 fixes this issue in our project. Thank you! 👍

danielga commented 1 year ago

Still have a few cases creating issues on 0.52.2. Made a small example to showcase it, where we use swift-composable-architecture and xctest-dynamic-overlay.

public func XCTUnimplemented<Result>(
    _ path: KeyPath<some Any, some Any>,
    file: StaticString = #file,
    fileID: StaticString = #fileID,
    line: UInt = #line
) -> @Sendable () -> Result {
    return { fatalError("don't care") }
}

public struct Environment {
    let test: () -> Void

    init(test: @escaping () -> Void) {
        self.test = test
    }

    static var unimplemented: Self {
        .init(test: XCTUnimplemented(\Self.test)) // this `Self` is removed by SwiftFormat, but is necessary
    }
}
lucianopa-msft commented 12 months ago

Hi, We are also seeing few extra cases where Self is still in 0.5.2 being removed incorrectly:

class A {
    static var p: Int = 0
}

class B: A {
    var test: Int {
        return Self.p.bitWidth // Still being removed
    }
}