Allow call of access(contract) interface function with a default implementation from an implementing contract

[joshuahannan]

Current Behavior

A contract interface defines an access(contract) function and gives it a default implementation.
A contract that implements the contract interface does not provide an implementation for the function, intending to use the default implementation, but when code in the implementing contract tries to call the function, there is an error:

cannot access `callableFromImpl`: function has contract access

Expected Behavior

The function should be able to be called by the implementation since it is access(contract)

Steps To Reproduce

1. Deploy this contract interface:

access(all) contract interface ContractFunction {

    access(contract) fun callableFromImpl() {
        log("no-op")
    }
}

2. Try to deploy this contract that imports and implements the contract interface:

import ContractFunction from 0x05

pub contract CallFunction: ContractFunction {
    init() {
        CallFunction.callableFromImpl()
    }
}

3. See the error that the function is not callable.

If an implementation for the function is added, the error goes away, but contracts may want to use the default implementation
because the default implementation in the contract may have access to some functionality that the implementing contract doesn't. This pattern is intended to be used in the V2 NonFungibleToken standard with the function that projects can call to emit a standard event from the contract interface.

Environment

Tested on play.flow.com with
- Cadence version: 0.42
- Network: Emulator

Tested with the Cadence 1.0 Emulator:
- Cadence Version:  github.com/onflow/cadence v1.0.0-M8
- Emulator Version: github.com/onflow/flow-emulator v1.0.0-M8

[joshuahannan]

This causes a bug in the new token standards that I missed testing while we were iterating. Just fell through the cracks, so I take responsibility for missing it. As of now, this either would require an update to Cadence to allow or we'll have to update the token standard with a workaround, but I'm curious to hear if this behavior by the language is intentional or not

[joshuahannan]

@SupunS @turbolent Can you take a look at this? This is preventing people from being able to update their contracts with all the functionality in the token standards

[SupunS]

In my view, it is correct to say that any function with access(contract) can only be accessed by the contract interface itself only. I guess what we are looking for here is an access modifier in between "contract" and "public" that can be seen by its implementations (i.e: something similar to protected in Java).

But then again, default functions defined with access(contract) are "partially visible" to their implementations already (related to #3207). I think we need to clean this up.

[turbolent]

See Discord discussion: https://discord.com/channels/613813861610684416/1222979306880372777

[joshuahannan]

This is different than the discussion about the Burner contract IMO. This is a function that, if there was an override implementation, would be accessible in the implementing contract, so it doesn't really make sense to not allow the default implementation to be accessed in the implementing contract. I would think that this should be allowed by Cadence, whereas I agree that the case of the interface contract being able to call access(contract) functions in implementations is okay.

[joshuahannan]

Like, if I were to make this function access(all), then my implementation contract could call the function default implementation via my contract (the implementation) even if my contract hasn't provided an implementation for the function. I don't see why an access(contract) function should be any different.

[turbolent]

I wouldn't consider the current behaviour a bug: A call of a access(contract) function (it does not matter if it happens to declare a default implementation for concrete types) from outside of the contract fails as expected.

I guess we could extend the access control mechanism and allow calls of the default function also from any code of a contract that defines a concrete type that implements the interface with the default function.

Is this really needed though? It seems to complicate both the language semantics and implementation even further.

As already pointed out in the Discord thread, the usage of access(contract) in an interface and its existing behaviour (Burner example) is already surprising. Adding even more functionality might not be a good idea.

[SupunS]

I was thinking we should even reject interface default functions with access(contract), because right now its a bit inconsistent/confusing. i.e: they would have to be public

[turbolent]

I was thinking we should even reject interface default functions with access(contract), because right now its a bit inconsistent/confusing. i.e: they would have to be public

Yeah, I made a similar suggestion in the Discord thread, we might want to restrict this further instead of extending it, potentially even forbidding access(contract) (maybe also access(self)?) in interfaces, to avoid confusion.

[joshuahannan]

Well, I definitely wouldn't want to restrict it any more because that would break the Burner contract! 😅 If we were to restrict it more, we would need a new access modifier that can allow the behavior that we are asking for here and with the Burner contract.

For this issue specifically, the problem is with the NFT standard needing to be able to emit a standard event, but we want that standard event to only be able to be triggered by a contract for its own NFTs. I've just made the function access(all) for now to make the event emission possible. That is acceptable, but isn't ideal because now that means that anyone with an authorized reference to any NFT can call it and emit junk events.

[bluesign]

interface can call the implementation but implementation cannot call the interface? I think both should be able to call each other ( without being public )

my mental model was when interface with default conditions is; there is no interface. Implementation and interface becomes one. Depending on how you access the resource/contract ( via interface or via type ), you see a limited view of the merged state.