onflow / flips

Flow Improvement Proposals
25 stars 22 forks source link

FLIP 134: relaxing interface conformance restrictions #134

Closed SupunS closed 10 months ago

SupunS commented 1 year ago

The FLIP addresses the last remaining concerns discussed in https://github.com/onflow/cadence/issues/2471

More discussions can be found in the ^ issue.

The reference implementation can be found at: https://github.com/onflow/cadence/pull/2725

SupunS commented 1 year ago

As discussed in the linked issue, the biggest risk with this relaxation is that enables interface authors to break downstream contracts. If A and B define the same function, but only A has a default implementation, if B's author later chooses to add a default implementation, any contracts downstream that implement both A and B will immediately break.

I don't think there is actually any new risk introduced by this, because even now, we allow B to have function definition with just conditions, while A is having a default implementation. So even now it is possible for B to add a default implementation and break downstream code.

joshuahannan commented 1 year ago

Agreed with Supun, I don't see how this adds any more risk than what is there currently

bluesign commented 11 months ago

To clarify:

This one will be Valid.

acess(all) resource interface Receiver {
    access(all) view fun isSupportedVaultType(): {Type: Bool} 
}
access(all) resource interface Vault: Receiver {    // OK
    access(all) view fun isSupportedVaultType(): {Type: Bool} {
        // Implementation...
    }
}
access(all) resource VaultImpl: Vault {}

what about this one?

acess(all) resource interface Receiver {
    access(all) view fun isSupportedVaultType(): {Type: Bool} 
}
access(all) resource interface Receiver2 {    
    access(all) view fun isSupportedVaultType(): {Type: Bool} {
        // Implementation...
    }
}
access(all) resource VaultImpl: Receiver, Receiver2 {}
SupunS commented 11 months ago

@bluesign The second one will be valid too.

Actually, the first one is an equivalent form (syntactic sugar) to the second one.

bluesign commented 11 months ago

If we allow first one and deny second, actually we can address @dsainati1 ‘s concern about breaking downstream I guess.

For me it makes sense that interface can change default implementation of inherited interface. ( like resource overriding default implementation ) but I think we can still reject when interfaces are siblings.

dsainati1 commented 11 months ago

If we allow first one and deny second, actually we can address @dsainati1 ‘s concern about breaking downstream I guess.

Allowing only the first one would still allow breaking downstream; if Receiver chose to add a default implementation to isSupportedVaultType then Vault's definition would break.

To be clear, I think the tradeoff in usability is worth it here, but it's worth being aware of.

bluesign commented 11 months ago

Allowing only the first one would still allow breaking downstream; if Receiver chose to add a default implementation to isSupportedVaultType then Vault's definition would break.

To be clear, I think the tradeoff in usability is worth it here, but it's worth being aware of.

I think we can allow that case ( if we disallow inheritance like ‘Vault, Receiver’ as Vault is already implementing Receiver )

This way we explicitly signal that we want Vault default implementation if exists

SupunS commented 11 months ago

Trying to understand what is proposed:

For me it makes sense that interface can change default implementation of inherited interface. ( like resource overriding default implementation ) but I think we can still reject when interfaces are siblings.

Is the proposal to allow the overriding of default functions? I think this could be a separate FLIP.

This FLIP actually does not propose any sort of overriding, but rather only proposes to relax 'who' can provide the implementation. Any interface in the conformance set can provide an implementation, except for the case where the empty function is in a descendant: https://github.com/onflow/flips/blob/123bf0edeb17a4b105014938cc5d7424e69e4bd9/cadence/20230816-relax-interface-conformance.md?plain=1#L109-L110


I think we can allow that case ( if we disallow inheritance like ‘Vault, Receiver’ as Vault is already implementing Receiver )

Is the suggestion to disallow a certain interface from being available from multiple paths (from siblings)?

The original proposal (https://github.com/onflow/flips/pull/40) has some examples on why this is currently allowed: the problem with disallowing is it can be too restrictive in certain cases. For e.g : https://github.com/onflow/flips/blob/123bf0edeb17a4b105014938cc5d7424e69e4bd9/cadence/20221024-interface-inheritance.md?plain=1#L277-L290

bluesign commented 11 months ago

Is the proposal to allow the overriding of default functions? I think this could be a separate FLIP.

Sorry my lack of technical vocabulary (override) caused a confusion here;

If Vault is Vault: Receiver, and DenizVault: Vault we can say that DenizVault wants to use Vault default implementation if exists. ( and if not defined implementation itself ofc ) But when we allow DenizVault: Vault, Receiver we cannot have an explicit preference. ( as interface order should not matter, or too confusing )

In your last example, we can still allow unless both Provider and Receiver implement log function. I mean it is still breakable for sure, but by disallowing DenizVault: Vault, Receiver case, we prevent a lot of cases where it breaks.

DenizVault: Provider, Receiver is fine by me, but DenizVault: Provider, Receiver, Logger seems making it easier to break. ( As then either Receiver or Provider implementing log will cause a break vs both implementing )

SupunS commented 11 months ago

ah, I see your point. Yes, we can reject such cases (i.e: DenizVault: Provider, Receiver, Logger). Even now, the third conformance (explicitly conforming to Logger) is non-effective, since the requirements are already met by conforming to either Provider or Receiver.

I honestly don't see how rejecting the above can prevent/affect the breakage of downstream codes, maybe due to my lack of knowledge of all the existing contracts out there / practical use cases, but I am on board with adding that restriction.

bluesign commented 11 months ago

I honestly don't see how rejecting the above can prevent/affect the breakage of downstream codes, maybe due to my lack of knowledge of all the existing contracts out there / practical use cases, but I am on board with adding that restriction.

I was thinking maybe we can traverse implementations with distance, and choose closest one if single, if more than one just error.

SupunS commented 11 months ago

Anyway, just wanted to add that rejecting the case of DenizVault: Provider, Receiver, Logger is unrelated to the change proposed in this FLIP. We can do the above irrespective of this FLIP.

bluesign commented 11 months ago

Yeah sorry I went off topic a bit again

SupunS commented 11 months ago

No worries! Feedback is always welcome!