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

Unneeded Synthesized Initializer removes needed initializers with property wrappers #5153

Closed
2 tasks done
NachoSoto opened this issue Aug 2, 2023 · 9 comments · Fixed by #5758
Closed
2 tasks done
Labels
repro-needed Issues that cannot be reproduced or miss proper descriptive examples.

Comments

@NachoSoto
Copy link
Contributor

New Issue Checklist

Describe the bug

This new rule correct actually needed initializers. Example:

private struct Data {
    @PropertyWrapper<E> var e: E

    init(e: E) {
        self.e = e
    }
}

SwiftLint removes that constructor, but it is required to be able to initialize it with a value of type E instead of @PropertyWrapper<E>

Environment

  • SwiftLint version: 0.52.4
@SimplyDanny SimplyDanny added bug Unexpected and reproducible misbehavior. good first issue Issue to be taken up by new contributors yet unfamiliar with the project. labels Aug 2, 2023
NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Aug 14, 2023
See https://realm.github.io/SwiftLint/unneeded_synthesized_initializer.html
This is currently broken (realm/SwiftLint#5153) which leads to `swiftlint --autocorrect` removing actually needed code.
NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Aug 14, 2023
See
https://realm.github.io/SwiftLint/unneeded_synthesized_initializer.html
This is currently broken
(realm/SwiftLint#5153) which leads to
`swiftlint --autocorrect` removing actually needed code.
MarkVillacampa pushed a commit to RevenueCat/purchases-ios that referenced this issue Sep 6, 2023
See
https://realm.github.io/SwiftLint/unneeded_synthesized_initializer.html
This is currently broken
(realm/SwiftLint#5153) which leads to
`swiftlint --autocorrect` removing actually needed code.
@aclima93
Copy link

aclima93 commented May 17, 2024

To add to this, it also removes initializers that just apply property wrappers to the arguments (it does not seem to trigger the rule in the linting digest?)

struct Title {
        let text: String
        let color: Color
        let font: Font

        init(
            @Localised text: String, // 👈 some property wrapper that alters the input
            color: Color,
            font: Font
        ) {
            self.text = text
            self.color = color
            self.font = font
        }
    }

weirder still, it will produce a "Superfluous Disable Command Violation" warning if disabled

        // swiftlint:disable:next unneeded_synthesized_initializer // 👈 stops the removal but produces a superfluous warning
        init(
            @Localised text: String,

swiftlint version: 0.55.1

@SimplyDanny
Copy link
Collaborator

To add to this, it also removes initializers that just apply property wrappers to the arguments (it does not seem to trigger the rule in the linting digest?)

struct Title {
        let text: String
        let color: Color
        let font: Font

        init(
            @Localised text: String, // 👈 some property wrapper that alters the input
            color: Color,
            font: Font
        ) {
            self.text = text
            self.color = color
            self.font = font
        }
    }

This is addressed by #5594.

weirder still, it will produce a "Superfluous Disable Command Violation" warning if disabled

        // swiftlint:disable:next unneeded_synthesized_initializer // 👈 stops the removal but produces a superfluous warning
        init(
            @Localised text: String,

I'm unable to reproduce this. The disable command works as expected in my little test setup.

@SimplyDanny
Copy link
Collaborator

SimplyDanny commented May 17, 2024

@NachoSoto: Taking a look at your initially reported example, I wonder why e would be initialized as @PropertyWrapper<E> without the explicit initializer. In a little example, the compiler seems to be happy without the initializer. Could you provide a complete example that shows the issue?

@SimplyDanny SimplyDanny added repro-needed Issues that cannot be reproduced or miss proper descriptive examples. and removed bug Unexpected and reproducible misbehavior. good first issue Issue to be taken up by new contributors yet unfamiliar with the project. labels Jul 3, 2024
@SimplyDanny
Copy link
Collaborator

@NachoSoto, @aclima93: Are these still issues for you?

@aclima93
Copy link

aclima93 commented Jul 4, 2024

... it also removes initializers that just apply property wrappers to the arguments

This is still happening,

struct Title {
    let text: String

    init(@Localised text: String) {
        self.text = text
    }
}

gets re-written as

struct Title {
    let text: String
}

unless I disable it with unneeded_synthesized_initializer

    // swiftlint:disable:next unneeded_synthesized_initializer
    init(@Localised text: String) {

weirder still, it will produce a "Superfluous Disable Command Violation" warning if disabled

I can no longer reproduce this part.

installed via homebrew and executed as /opt/homebrew/bin/swiftlint --fix && /opt/homebrew/bin/swiftlint

image

@SimplyDanny
Copy link
Collaborator

... it also removes initializers that just apply property wrappers to the arguments

This is still happening,

struct Title {
    let text: String

    init(@Localised text: String) {
        self.text = text
    }
}

gets re-written as

struct Title {
    let text: String
}

unless I disable it with unneeded_synthesized_initializer

    // swiftlint:disable:next unneeded_synthesized_initializer
    init(@Localised text: String) {

Yeah, this got fixed after the 0.55.1 release only.

@bradhowes
Copy link

@SimplyDanny will your fix also address the scenario when the initializer is marked @usableFromInline and is required in an @inlinable call chain?

@mildm8nnered
Copy link
Collaborator

@SimplyDanny will your fix also address the scenario when the initializer is marked @usableFromInline and is required in an @inlinable call chain?

Ah - I thought I saw an issue relating to this recently in the periphery codebase

@SimplyDanny
Copy link
Collaborator

@SimplyDanny will your fix also address the scenario when the initializer is marked @usableFromInline and is required in an @inlinable call chain?

It doesn't. I wonder if initializers with any type of attribute should be excluded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repro-needed Issues that cannot be reproduced or miss proper descriptive examples.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants