Interface default implementation support

[bluesign]

Closes #989

Description

Add default implementation support to interface methods.

---

• [X] Targeted PR against master branch
• [X] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• [X] Code follows the standards mentioned here
• [ ] Updated relevant documentation
• [X] Re-reviewed Files changed in the Github PR explorer
• [ ] Added appropriate labels

[bjartek]

@turbolent any chance you could take a peak at this.

[turbolent]

Thank you again for your work here @bluesign! The review and merge will have to wait a while, as we currently have a backlog of work to integrate. Thank you for you patience

[bjartek]

If I have an interface that requires a method and that has a default implementation that appears to not be allowed now. In my GeneircNFT repo https://github.com/bjartek/flow-nfmt/blob/nftv2/contracts/GenericNFT.cdc i tried to make borrowNFT be default implemented but it does not work. If possible this should be fixed.

[bluesign]

@turbolent thanks for the review, I am planning to jump in tomorrow.

[turbolent]

@bluesign Would it be OK to commit some improvements to this PR, or would you rather like a separate PR against this one?

[turbolent]

Re-reviewed the implementation, it looks good and makes sense. There's one open question about some code removal still that would be good to address. I have some improvements that I could push up to the PR or in a separate PR as mentioned above.

I'll look at the tests on Monday, I think we're close with getting this in, thanks for the patience everyone!

[bluesign]

@bluesign Would it be OK to commit some improvements to this PR, or would you rather like a separate PR against this one?

Ofc It's ok. Just I am on a pentest in London right now, I will be able to check more on Monday/Tuesday when I am back.

[codecov-commenter]

Codecov Report

Merging #1076 (5653108) into master (674324f) will increase coverage by 0.03%. The diff coverage is 92.99%.

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   77.72%   77.75%   +0.03%     
==========================================
  Files         294      294              
  Lines       62193    62338     +145     
==========================================
+ Hits        48340    48472     +132     
- Misses      12129    12141      +12     
- Partials     1724     1725       +1     

Flag	Coverage Δ
unittests	77.75% <92.99%> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
runtime/sema/type.go	89.25% <ø> (ø)
runtime/sema/errors.go	73.65% <63.63%> (-0.32%)	:arrow_down:
runtime/interpreter/interpreter.go	89.54% <90.00%> (+<0.01%)	:arrow_up:
runtime/ast/block.go	97.12% <100.00%> (+0.03%)	:arrow_up:
runtime/sema/check_assignment.go	97.48% <100.00%> (ø)
runtime/sema/check_composite_declaration.go	95.87% <100.00%> (+0.23%)	:arrow_up:
runtime/sema/check_interface_declaration.go	96.48% <100.00%> (-0.11%)	:arrow_down:
runtime/ast/function_declaration.go	80.32% <0.00%> (+1.63%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[turbolent]

I'm currently adding tests for the feature using when type requirements, and it looks like the interface's type's default methods are declared in the concrete type's nested type. Looking into it

[turbolent]

Alternatively, maybe just for now, and given we might remove type requirements soon, we forbid default methods in type requirements?

But I guess this is actually exactly the feature that is needed for the NFT Metadata backwards-compatibility? There, default methods are added to the NFT, the nested type, not the contract, right?

[turbolent]

Finished implementing default functions for type requirements in the checker, remaining is implementing support for it in the interpreter.

Finally, we need to still discuss / decide on special functions (initializers and destructors) and potentially implement them and test the behaviour.

[bluesign]

Thanks @turbolent, I think initializers and destructors are also can be needed but I am not sure about what can be the side effects there. ( So I think I am a bit more leaning to against )

As every resource has most likely have an initializer and destructor, this can have problem when you are trying to compose something with multiple interfaces. Maybe instead of override, we need to call all interface initializers and destructors somehow. ( but then we are getting into dangerous waters again )

[bjartek]

Totally agree with @SupunS here. If you can say default implement a destructor and destroy things and you forget to override that in your impl it can be bad.

[bluesign]

Totally agree with @SupunS here. If you can say default implement a destructor and destroy things and you forget to override that in your impl it can be bad.

Not that I disagree ( I have the same feeling ) but how this can go bad? It is puzzling me last 4-5 days, trying to find a edge case, so I am curious

[bjartek]

Well lets say the default impl of a destructor is to burn something because it does by default contain anything useful. Then you a dev add some escrow to it and forget to override destructor. Now you suddebly burned ft.

[bluesign]

Well lets say the default impl of a destructor is to burn something because it does by default contain anything useful. Then you a dev add some escrow to it and forget to override destructor. Now you suddebly burned ft.

Very good point. Thats why cadence doesn't auto destroy nested stuff and want you to explicitly destroy them I guess.

[bjartek]

Any progress here? I still think it is valid to not allow default impl of init/destroy. This feature is sorely needed.

[turbolent]

@bjartek I think the current status and work left to be done is:

• We agree that interfaces should not be able to define default initializers and destructors (at least for now), therefore:
  • The type checker should report an error for all special function declarations (e.g. initializers, destructors) with statements in interfaces
  • Add a test case for the new error above
• Address PR feedback: Supun had some questions and suggestions
• Resolve conflicts

Once the points above are addressed, the PR is in a good shape to get merged 🎉

[bluesign]

@turbolent

The type checker should report an error for all special function declarations (e.g. initializers, destructors) with statements in interfaces, Add a test case for the new error above

Are there anything else than initializers, destructors I should disallow ?

Also any meaningful error message suggestion for this ?

[turbolent]

@bluesign

Are there anything else than initializers, destructors I should disallow ?

Initializers and destructors are currently the only kinds of special functions (ast.SpecialFunctionDeclaration), but it is maybe best to just forbid default methods for all special functions in general.

Also any meaningful error message suggestion for this ?

There are no errors defined for this yet, but maybe an error message like this should suffice for now (we can improve it later): fmt.Sprintf("%s may not be defined as a default function", kind.Name())

[turbolent]

Merging master into this PR's branch should fix CI

[joshuahannan]

Looks like there are a bunch of changes to the docs, best practices, and such that aren't related to default implementation support. Did you also make a bunch of edits to those or have you just not pulled the latest from master or something?

[bluesign]

@joshuahannan yeah I messed up when fetching upstream I guess :( trying to fix

[bluesign]

I think I managed to do all, just @SupunS's variable scope comment left, but I have no idea tbh how to approach that.

[turbolent]

Do we have (a) test case(s) that ensure that default implementations are only able to have the same access as non-receiver functions? It would be great to test e.g. that a default function does not have access to the members of the concrete type it is implemented by.

For example, this program should be invalid:

struct interface SI {
    s: Int

    fun default() {
        self.s = 1
    }
}

struct S: SI {
    var s: Int

    init() {
        self.s = 0
    }
}

[bluesign]

@turbolent I don't see the problem here, can you expand a bit?

that a default function does not have access to the members of the concrete type it is implemented by

default function should have access in my opinion. Otherwise they will not have any use case.

For example:

• FT.Provider can provide standard Withdraw function.
• We can use interfaces somehow like lego blocks, and compose resources without even coding.
• It will prevent copy paste coding ( we have exactly same code in thousands of contracts )

[turbolent]

@turbolent I don't see the problem here, can you expand a bit?

Yes, an example would be:

struct interface SI {
    fun default() {
        self.s = 1
    }
}

struct S: SI {
    var s: Int

    init() {
        self.s = 0
    }
}

Here, SI.default should not have access to S.s. SI should already be invalid on its own already, so there shouldn't be any changes needed, just a test case would be nice to prevent future accidental regressions.

Refarding the other example case from my previous post:

struct interface SI {
    s: Int

    fun default() {
        self.s = 1
    }
}

struct S: SI {
    var s: Int

    init() {
        self.s = 0
    }
}

Here, SI has a field s, but it has no let or var modifier – which is valid, and means that a concrete type conforming to the interface, like S, may choose to implement the field as either let or var. However, SI.default should not have write access to s.

This may already be handled, but I'm not sure, so it would be great to have this as a test case, too.

[turbolent]

Resolved the merge conflicts and added new conformances to the errors (they are user errors)

[turbolent]

Implemented the missing case for checking if type requirements have default special functions in b1ad20e

[turbolent]

@bluesign The first example case from my post above (field only exists in concrete type) already had a test case.

The second example case (field exist, but is not let or var) didn't have a test case, so I added a it in 2d29b42. It didn't pass, so fixed the implementation.

Looks like everything is there now! 🎉

@SupunS could you please have a final look? I'll have a final look as well