plaid / plaid-link-ios

Plaid Link iOS SDK
https://plaid.com/docs/link/ios
MIT License
136 stars 90 forks source link

Xcode Warnings #48

Closed Hartistic closed 4 days ago

Hartistic commented 1 month ago

A detailed description of the steps to reproduce the issue

What you expected to see?

No warnings

What you saw instead?

Warnings

Screenshots that show the state of the UI (if applicable) Screenshot 2024-10-13 at 12 17 44 PM

When submitting an issue for Link on the web or within a Webview, please include the following information:

Name Value
Link env
Browser
Operating system

When submitting an issue for Plaid Link iOS please include the following information:

Name Value
Link env
LinkKit version
iOS version
iOS device

List of used 3rd party libraries (e.g. CocoaPods):

dtroupe-plaid commented 1 month ago

@Hartistic Thanks for reporting. This warning was introduced by SE-0364.

I'll see what we can do to resolve these in the next major release v6.0.0. In what version of Xcode are you seeing these errors?

Hartistic commented 1 month ago

Thanks @dtroupe-plaid, we are currently using Version 16.0 (16A242d)

dtroupe-plaid commented 4 days ago

Hey @Hartistic looking at this more closely these warnings are NOT inside LinkKit they are inside your code. This is because you're implementing equatable for our models for example SuccessMetadata. Here is our code:

/// Metadata available when the Link flow was exited successfully.
public struct SuccessMetadata: Codable {
    /// The institution holding the accounts selected by the user.
    public var institution: Institution

    /// The accounts that were linked by the user.
    public var accounts: [Account]

    /// A unique identifier associated with a user's actions and events through the Link flow.
    ///
    /// Include this identifier when opening a support ticket for faster turnaround.
    public var linkSessionID: String

    /// Unprocessed metadata sent from Plaid API.
    public var metadataJSON: RawJSONMetadata?
}

SE-0364 is correctly warning you that if I add Equatable support to this model the compiler won't know which version to use. In order to "fix" these issues we would have to make all of these model equatable and you'd have to delete your implementation.

Hartistic commented 4 days ago

@dtroupe-plaid

I feel horrible, it is exactly that, I had added the following and was just too inconsiderate and foolish to look at it. My sincere apologies for opening this ticket:

import LinkKit

// In order to monitor and observe Link Kit properties we need to make sure their stuff
// is Equatable, this allows us more flexibility with the LinkKit SDK. - Joshua Hart

extension LinkKit.SuccessMetadata: Equatable {
    public static func == (lhs: SuccessMetadata, rhs: SuccessMetadata) -> Bool {
        return lhs.institution == rhs.institution &&
        lhs.accounts == rhs.accounts &&
        lhs.linkSessionID == rhs.linkSessionID &&
        lhs.metadataJSON == rhs.metadataJSON
    }
}

extension LinkKit.ExitMetadata: Equatable {
    public static func == (lhs: ExitMetadata, rhs: ExitMetadata) -> Bool {
        return lhs.status == rhs.status &&
        lhs.institution == rhs.institution &&
        lhs.linkSessionID == rhs.linkSessionID &&
        lhs.requestID == rhs.requestID &&
        lhs.metadataJSON == rhs.metadataJSON
    }
}

extension LinkKit.Institution: Equatable {
    public static func == (lhs: LinkKit.Institution, rhs: LinkKit.Institution) -> Bool {
        return lhs.id == rhs.id &&
        lhs.name == rhs.name
    }
}

extension LinkKit.Account: Equatable {
    public static func == (lhs: LinkKit.Account, rhs: LinkKit.Account) -> Bool {
        return lhs.id == rhs.id &&
        lhs.name == rhs.name &&
        lhs.mask == rhs.mask &&
        lhs.subtype == rhs.subtype &&
        lhs.verificationStatus == rhs.verificationStatus
    }
}

extension LinkKit.VerificationStatus: Equatable {
    public static func == (lhs: VerificationStatus, rhs: VerificationStatus) -> Bool {
        switch (lhs, rhs) {
        case (.pendingAutomaticVerification, .pendingAutomaticVerification),
            (.pendingManualVerification, .pendingManualVerification),
            (.manuallyVerified, .manuallyVerified):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype: Equatable {
    public static func == (lhs: AccountSubtype, rhs: AccountSubtype) -> Bool {
        switch (lhs, rhs) {
        case let (.unknown(type1, subtype1), .unknown(type2, subtype2)):
            return type1 == type2 && subtype1 == subtype2
        case let (.other(lhsOther), .other(rhsOther)):
            return lhsOther == rhsOther
        case let (.credit(lhsCredit), .credit(rhsCredit)):
            return lhsCredit == rhsCredit
        case let (.loan(lhsLoan), .loan(rhsLoan)):
            return lhsLoan == rhsLoan
        case let (.depository(lhsDepository), .depository(rhsDepository)):
            return lhsDepository == rhsDepository
        case let (.investment(lhsInvestment), .investment(rhsInvestment)):
            return lhsInvestment == rhsInvestment
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype.Other: Equatable {
    public static func == (lhs: AccountSubtype.Other, rhs: AccountSubtype.Other) -> Bool {
        switch (lhs, rhs) {
        case (.all, .all), (.other, .other):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype.Credit: Equatable {
    public static func == (lhs: AccountSubtype.Credit, rhs: AccountSubtype.Credit) -> Bool {
        switch (lhs, rhs) {
        case (.all, .all), (.creditCard, .creditCard), (.paypal, .paypal):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype.Loan: Equatable {
    public static func == (lhs: AccountSubtype.Loan, rhs: AccountSubtype.Loan) -> Bool {
        switch (lhs, rhs) {
        case (.all, .all), (.auto, .auto), (.business, .business), (.commercial, .commercial),
            (.construction, .construction), (.consumer, .consumer), (.homeEquity, .homeEquity),
            (.lineOfCredit, .lineOfCredit), (.loan, .loan), (.mortgage, .mortgage),
            (.overdraft, .overdraft), (.student, .student):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype.Depository: Equatable {
    public static func == (lhs: AccountSubtype.Depository, rhs: AccountSubtype.Depository) -> Bool {
        switch (lhs, rhs) {
        case (.all, .all), (.cashManagement, .cashManagement), (.cd, .cd),
            (.checking, .checking), (.ebt, .ebt), (.hsa, .hsa), (.moneyMarket, .moneyMarket),
            (.paypal, .paypal), (.prepaid, .prepaid), (.savings, .savings):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.AccountSubtype.Investment: Equatable {
    public static func == (lhs: AccountSubtype.Investment, rhs: AccountSubtype.Investment) -> Bool {
        switch (lhs, rhs) {
        case (.all, .all), (.brokerage, .brokerage), (.cashIsa, .cashIsa),
            (.educationSavingsAccount, .educationSavingsAccount), (.fixedAnnuity, .fixedAnnuity),
            (.gic, .gic), (.healthReimbursementArrangement, .healthReimbursementArrangement),
            (.hsa, .hsa), (.investment401a, .investment401a), (.investment401k, .investment401k),
            (.investment403B, .investment403B), (.investment457b, .investment457b), (.investment529, .investment529),
            (.ira, .ira), (.isa, .isa), (.keogh, .keogh), (.lif, .lif), (.lira, .lira),
            (.lrif, .lrif), (.lrsp, .lrsp), (.mutualFund, .mutualFund), (.nonTaxableBrokerageAccount, .nonTaxableBrokerageAccount),
            (.pension, .pension), (.plan, .plan), (.prif, .prif), (.profitSharingPlan, .profitSharingPlan),
            (.rdsp, .rdsp), (.resp, .resp), (.retirement, .retirement), (.rlif, .rlif), (.roth, .roth),
            (.roth401k, .roth401k), (.rrif, .rrif), (.rrsp, .rrsp), (.sarsep, .sarsep), (.sepIra, .sepIra),
            (.simpleIra, .simpleIra), (.sipp, .sipp), (.stockPlan, .stockPlan), (.tfsa, .tfsa),
            (.thriftSavingsPlan, .thriftSavingsPlan), (.trust, .trust), (.ugma, .ugma), (.utma, .utma),
            (.variableAnnuity, .variableAnnuity):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}

extension LinkKit.ExitStatus: Equatable {
    public static func == (lhs: ExitStatus, rhs: ExitStatus) -> Bool {
        switch (lhs, rhs) {
        case (.requiresQuestions, .requiresQuestions),
            (.requiresSelections, .requiresSelections),
            (.requiresCode, .requiresCode),
            (.chooseDevice, .chooseDevice),
            (.requiresCredentials, .requiresCredentials),
            (.institutionNotFound, .institutionNotFound),
            (.requiresAccountSelection, .requiresAccountSelection),
            (.continueToThirdParty, .continueToThirdParty):
            return true
        case let (.unknown(lhsValue), .unknown(rhsValue)):
            return lhsValue == rhsValue
        default:
            return false
        }
    }
}
Hartistic commented 4 days ago

@dtroupe-plaid is it difficult to add Equatable?

dtroupe-plaid commented 4 days ago

@Hartistic it's not difficult to add that support, but if you (or any other clients) don't remove your equatable conformance you will have undefined behavior. Additionally, if your logic and the logic I implement for equatable don't match your expected behavior might change.

The warning is basically saying that you should not use Equatable because the Library could implement it as well (with different logic). You should consider implementing an internal protocol that does your equality checks.

For example

// Internal version of Equatable to avoid polluting public API namespace
internal protocol InternalEquatable {
    static func == (lhs: Self, rhs: Self) -> Bool
}

// ....

extension SuccessMetadata: InternalEquatable {
    static func == (lhs: Self, rhs: Self) -> Bool {
        return
            lhs.linkSessionID == rhs.linkSessionID
            && lhs.institution == rhs.institution
            && lhs.accounts == rhs.accounts
            && lhs.metadataJSON == rhs.metadataJSON
    }
}

If I pollute the public namespace with equatable conformance every customer that's implemented equatable will have migration issues when updating to V6. I am not sure that's the right decision here. If the SDK was new - I'd probably implement Equatable from the start, but at this point it's been around for years so it's likely many customers did what you did - now Swift 6 is telling them this is not safe so they should alter their implementation like I recommended above.