Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interface default implementation support #1076

Merged
merged 51 commits into from
Aug 17, 2022
Merged

Conversation

bluesign
Copy link
Contributor

@bluesign bluesign commented Jul 16, 2021

Closes #989

Description

Add default implementation support to interface methods.


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

@bjartek
Copy link
Contributor

bjartek commented Jul 21, 2021

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

@turbolent turbolent self-assigned this Jul 22, 2021
@turbolent
Copy link
Member

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
Copy link
Contributor

bjartek commented Aug 23, 2021

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.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/sema/check_composite_declaration.go Outdated Show resolved Hide resolved
runtime/sema/check_composite_declaration.go Outdated Show resolved Hide resolved
runtime/sema/check_composite_declaration.go Outdated Show resolved Hide resolved
runtime/tests/checker/interface_test.go Outdated Show resolved Hide resolved
runtime/tests/checker/interface_test.go Outdated Show resolved Hide resolved
runtime/tests/checker/interface_test.go Outdated Show resolved Hide resolved
runtime/tests/interpreter/interpreter_test.go Outdated Show resolved Hide resolved
runtime/tests/interpreter/interpreter_test.go Outdated Show resolved Hide resolved
@bluesign
Copy link
Contributor Author

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

bluesign and others added 14 commits November 25, 2021 20:16
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
@bluesign bluesign requested a review from turbolent April 27, 2022 20:31
@bluesign
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

bluesign commented Jul 15, 2022

@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
Copy link
Member

turbolent commented Aug 15, 2022

@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
Copy link
Member

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

@turbolent
Copy link
Member

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

@turbolent
Copy link
Member

@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


_, err := ParseAndCheck(t, `
struct interface IA {
x: Int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, never knew defining a field without var/let is valid in an interface 🤔

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👌

@turbolent
Copy link
Member

Added some minimal documentation

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too. Thanks everyone!

@bjartek
Copy link
Contributor

bjartek commented Aug 17, 2022

Thank you so much everybody for your hard work! This is going to be a game changer in terms of composability #onFlow!

@bluesign
Copy link
Contributor Author

Thanks a lot @turbolent, for explanation and all the work. I am really glad this feature finally will be out.

@psiemens
Copy link
Contributor

This is so exciting! Great work everybody! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding default implementations to interface methods
8 participants