Improve interface default functions

[SupunS]

Issue to be solved

Originated from discussion: #2112 (comment)

Currently, it is invalid for one interface to provide a default implementation, and another to declare the function definition only.

struct interface A {
    // Have default impl
    pub fun hello() {
        var a = 1
    }
}
            
struct interface B {
    // Declaration only
    pub fun hello()
}

struct C: A, B {}  // Error: missing default implementation for 'B.foo'

This is too restrictive, and also avoids use-cases where certain interfaces only want to define pre/post conditions, while some other interface provides the default implementation.

Suggested Solution

The above example should not give errors.

Furthermore, interface B should be able to define a pre/post condition. e.g:

struct interface A {
    // Have default impl
    pub fun hello() {
        var a = 1
    }
}
            
struct interface B {
    // Pre-condition only
    pub fun hello() {
       pre { someCondition() }
    }
}

struct C: A, B {}

[bluesign]

This is restrictive I agree, but also isn't it preventitive ? Chances are my contract ( in this case C ) will break is obviously higher in this case vs B not having hello method.

[SupunS]

Yeah, I think it comes down to what Cadence developers would want between the two options.

This definitely going to need a FLIP, I only created this issue for now to make it easier to track.

[turbolent]

Is this related to discussion we had in the PR that added interface default functions? #1076 (comment)

[SupunS]

Yeah, it is the same.

The concern now is, with interface inheritance, that particular case can become a useful pattern, where interfaces in the chain might define just conditions (which is part of the restrictions that you would get by conforming to that interface). But with the current restrictions, it makes it impossible to add such conditions if some other interface provides a default implementation already or vise-versa.
e.g:

pub resource interface Provider {
    pub fun withdraw(_ amount: Int): @NFT
}

pub resource interface Vault: Provider {
    pub fun withdraw(_ amount: Int): @NFT {
        // some default implementation
    }
}

pub resource interface ThrottledVault: Provider {
    pub fun withdraw(_ amount: Int): @NFT {
        pre { amount < 50 }
    }
}

pub resource MyVault: Vault, ThrottledVault {  // Will report an error
}

^ might not be the best example, but just want to show the idea.

Also, with stable-cadence, since conditions can now be view-only, I think some of the original concerns had at that time are no longer applicable? e.g: order of conditions isn't going to be a problem

[turbolent]

TLDR:

• For the first example in the description, keep the current behaviour of getting rejected
• For the second example in the description, change the current behaviour (restriction) and allow it

Current behaviour of conflicting default functions

I re-familiarized myself with the current behaviour for conflicting default functions and the reasoning behind it (original discussion: #1076 (comment)):

If conformance to multiple interfaces results in multiple default functions for a particular interface function declaration, then this conflict results in an error. I think we all agreed so far that this behaviour is better than resolving such a conflict automatically, e.g. by picking a particular default function based on order of the interfaces in the conformance list.

Let's assume there is currently no conflict, e.g.

struct interface I1 {
    fun test(): Int {
        return 1
    }
}

struct interface I2 {
    fun test(): Int
}

struct S: I1, I2 {}

If I2 gets updated and the current requirement of I2.test is turned into a default function, then a conflict is introduced, which breaks conforming types implementing the interfaces, i.e. S. Given that I1 and I2 are likely are defined by different authors, it is likely that the author of I2's local change, which independently is benign, may have a big impact on implementations (types implementing the interface).

Therefore, Deniz' point is that already the original definition/case before the update should be forbidden. I still think this reasoning makes sense.

Default functions and conditions

An interface function may be declared in three ways:

1. No body. An implementation of the interface must provide an implementation of the function.
   For example:

   struct interface I {
       fun test(): Int
   }

2. Body, only conditions. An implementation of the interface still must provide an implementation of the function. The conditions in the interface function "surround" the function implemented in the type implementing the interface.
   For example:

   struct interface I {
       fun test(): Int {
           pre { true }
       }
   }

3. Body, implementation. An implementation of the interface can, but does not have to provide an implementation of the function.

   struct interface I {
       fun test(): Int {
           return 1
       }
   }

Let's consider the example from the previous section again:I2 gets updated, and instead of the declaration gaining a full body, i.e. becoming a default function (1 -> 3), this time it gains a body with just conditions (1 -> 2):

struct interface I1 {
    fun test(): Int {
        return 1
    }
}

struct interface I2 {
    fun test(): Int {
      pre { true }
    }
}

struct S: I1, I2 {}

This seems OK to me: S does not provide an implementation of test, so the default function provided in I1 is picked. As always, all conditions of all interfaces are surrounding the function.

As a result, I would recommend

• Keep the current behaviour for conflicting default functions (3 + 3) and single default function (1 + 3) (as discussed in the previous section)
• Change the behaviour for the combination of conditions and default function (2 + 3) from currently being invalid to being valid

[SupunS]

Thanks, Bastian for the great writeup!

I find it hard to reason why (1 + 3) is not OK, whereas (2 + 3) is OK.

If I2 gets updated and the current requirement of I2.test is turned into a default function, then a conflict is introduced, which breaks conforming types implementing the interfaces, i.e. S. Given that I1 and I2 are likely are defined by different authors, it is likely that the author of I2's local change, which independently is benign, may have a big impact on implementations (types implementing the interface).

Therefore, Deniz' point is that already the original definition/case before the update should be forbidden. I still think this reasoning makes sense.

Wouldn't any local change to I2 could have an impact on implementations?
For e.g: I2 could initially have no members (i.e: struct interface I2 {}), and could be updated to have fun test(): Int {return 1}, which would still break the implementations. So in my opinion, I2 having just the function signature vs having no members have the same impact on updates.

I believe any combination other than (3+3) should be OK.

[turbolent]

Wouldn't any local change to I2 could have an impact on implementations?

Some local changes, like e.g. adding an additional function requirement (1), do break implementations of the interface, but already do so on their own, independent of the implementation conforming to other interfaces.

For example, assume there is an empty interface I and test is added:

struct interface I {
    fun test(): Int
 // ^^^^^^^^^^^^^^^ added in update
}

struct S: I {}
    //    ^ only one interface
    // ^ implementation is broken 

However, the local change of adding a function with a default implementation (3), or turning a function without without a body (1) into a default function, does not break implementations of the interface – that what the original use-case, adding additional functions to interface in a non-breaking way for implementations.

For example, assume there is an empty interface I and test is added:

struct interface I {
    fun test(): Int {
        return 0
    }
 // ^^^^^^^^^^^^^^^ added or changed from function without body in update
}

struct S: I {}
    //    ^ only one interface
    // ^ implementation is NOT broken 

The "(2 + 3) behaviour" basically just ensures that such a local change keeps on being a non-breaking change for implementations of the interface in the case where the implementing type conforms to multiple interfaces.

To be honest, it is just a "nice to have" when this particular situation is encountered, is not critical, and could be relaxed.

[SupunS]

Opened FLIP onflow/flips#83 for relaxing the current restriction, and to allow pre/post conditions to coexist with default implementations coming from different interfaces.

[SupunS]

Added in #2697

[joshuahannan]

I'd like to re-open this discussion about allowing an empty function declaration in one interface and a default implementation for the function in an inheriting interface. In the new fungible token and non-fungible token standards, there are functions that we want to declare in the Receiver interface and have default implementations for in the Vault interface which inherits from Receiver, for example.

access(all) resource interface Receiver {

        /// getSupportedVaultTypes optionally returns a list of vault types that this receiver accepts
        access(all) view fun getSupportedVaultTypes(): {Type: Bool}
    }

    access(all) resource interface Vault: Receiver, Balance, Transferor, Provider, ViewResolver.Resolver {

        /// getSupportedVaultTypes optionally returns a list of vault types that this receiver accepts
        access(all) view fun getSupportedVaultTypes(): {Type: Bool} {
            // Below check is implemented to make sure that run-time type would
            // only get returned when the parent resource conforms with `FungibleToken.Vault`. 
            if self.getType().isSubtype(of: Type<@{FungibleToken.Vault}>()) {
                return {self.getType(): true}
            } else {
                // Return an empty dictionary as the default value for resource who don't
                // implement `FungibleToken.Vault`, such as `FungibleTokenSwitchboard`, `TokenForwarder` etc.
                return {}
            }
        }

This allows us to have the benefits of defining custom resources that implement the Receiver interface, but also making it possible for inheritors of those, such as Vault, to be able to define implementations when it makes sense to have them. Without this, we can't have the default implementation for multiple FungibleToken and NonFungibleToken methods, including the metadata views methods. I think this is a pattern that won't be that uncommon and I'm not really sold on it being a security risk because there are always ways for inherited types to be updated to breaking inheriting contracts.

EDIT: This is also a blocker for finishing the updated token standards for stable cadence

[SupunS]

Thanks @joshuahannan for bringing this up. I opened a FLIP onflow/flips#134 proposing the suggested improvement.

[SupunS]

Added in #2725