cadence
cadence copied to clipboard
Interface default implementation support
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
@turbolent any chance you could take a peak at this.
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
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.
@turbolent thanks for the review, I am planning to jump in tomorrow.
@bluesign Would it be OK to commit some improvements to this PR, or would you rather like a separate PR against this one?
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 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 Report
Merging #1076 (5653108) into master (674324f) will increase coverage by
0.03%
. The diff coverage is92.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.
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
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?
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.
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 )
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.
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
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.
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.
Any progress here? I still think it is valid to not allow default impl of init/destroy. This feature is sorely needed.
@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 π
@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 ?
@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())
Merging master
into this PR's branch should fix CI
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?
@joshuahannan yeah I messed up when fetching upstream I guess :( trying to fix
I think I managed to do all, just @SupunS's variable scope comment left, but I have no idea tbh how to approach that.
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
}
}
@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 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.
Resolved the merge conflicts and added new conformances to the errors (they are user errors)
Implemented the missing case for checking if type requirements have default special functions in b1ad20e
@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