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

FLIP: interface inheritance in Cadence #40

Merged
merged 16 commits into from
May 11, 2023
Merged

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Oct 24, 2022

@SupunS SupunS force-pushed the supun/interface-inheritance branch from 87dfae2 to fb4f3be Compare October 25, 2022 00:15
Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for writing this up. This is a really thorny design space so we should take care to iron out all the edge cases here. I had some specific questions below, but also some general ones:

  1. How will this new feature interact with the proposed changes in Events emitted from interfaces  cadence#2069 or Allow composites to inherit events from interfaces cadence#2081 ? If events can be "inherited" from interfaces, how do they resolve in cases of multiple inheritance?

  2. I think this change would also benefit from a redundancy check in any places where a list of interfaces might appear. In a conformance list, for example, it would be redundant to say MyVault: Receiver, Vault, so we should have a static error warning that the Receiver annotation is unnecessary in the presence of the Vault annotation. The same would go for restricted types' restriction lists.

cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
cadence/2022-10-24-interface-inheritance.md Show resolved Hide resolved
@SupunS
Copy link
Member Author

SupunS commented Oct 25, 2022

How will this new feature interact with the proposed changes in onflow/cadence#2069 or onflow/cadence#2081 ? If events can be "inherited" from interfaces, how do they resolve in cases of multiple inheritance?

Thanks for bringing this up! I was thinking the type definitions would also behaves similarly to the default functions. Added a section for this: d98d658

@SupunS
Copy link
Member Author

SupunS commented Oct 25, 2022

I think this change would also benefit from a redundancy check in any places where a list of interfaces might appear. In a conformance list, for example, it would be redundant to say MyVault: Receiver, Vault, so we should have a static error warning that the Receiver annotation is unnecessary in the presence of the Vault annotation. The same would go for restricted types' restriction lists.

Yes definitely, we could add a redundancy check for conformances. Having redundant conformances can sometimes cause static errors by the current rules (i.e: when there are overridden default functions, etc.). Others can be checked at a linter phase.

@SupunS SupunS requested a review from dete October 25, 2022 15:53
@j1010001 j1010001 changed the title Add FLIP for interface inheritance in Cadence FLIP: interface inheritance in Cadence Dec 14, 2022
@turbolent
Copy link
Member

It looks like Swift's solution is to reject diamond conflicts, instead e.g. picking a certain default implementation: https://www.vadimbulavin.com/multiple-inheritance-swift/

@SupunS
Copy link
Member Author

SupunS commented Apr 25, 2023

We had a breakout session regarding the diamond problem today, and rejecting the default function conflicts (as proposed in this FLIP) was favored by the majority.

A detailed summary of the solutions discussed in the meeting is as below.

  • Reject ambiguous cases (proposed in the FLIP)
    • Concern:
      • Someone higher up the chain can break downstream code by introducing a default implementation.
    • Conclusion: Accepted solution ✅
  • Cadence to pick one, maybe the one closest to the current implementation.
    • Concerns:
      • This may result in unintended/surprising behaviors.
      • Already disregarded this option for default functions ambiguity resolution in concrete type implementations. (They also behave very similarly to what is proposed in the above FLIP)
    • Conclusion: Too "magical" 👎
  • Users to “lock in” to an implementation, optionally.
    • e.g:

      pub resource MyVault: Vault {
          pub inherited<Receiver> fun log(_ message: String)
      }
    • Concern:

      • This is optional. Doesn’t guarantee developers will use it. Their code could break if they don’t, same as the original FLIP proposal.
      • Cannot mandate this ‘locking-in’. Because it would contradict the original intention of default functions - which is to update interfaces (add methods) without breaking the downstream code.
    • Conclusion: Doesn't really solve the problem 👎

@turbolent
Copy link
Member

@SupunS Nice!

It looks like the decision needs to be reflected in this section: https://github.com/onflow/flips/pull/40/files#diff-0e651e7f8b8bd0a53ffe1e6333ac854ee0a0c7c6932d1bbcd3b51883f18161e8R190-R194

@SupunS
Copy link
Member Author

SupunS commented Apr 26, 2023

Updated the FLIP with the latest discussion outcomes (7127f6b).

Also added a new section on how the interfaces/conditions would be linearized (9005d7f). TL;DR is, conditions are linearized in a depth-first pre-order manner, ignoring any duplicates (duplicates are cases where there are two or more paths to the same interface/condition).

@SupunS
Copy link
Member Author

SupunS commented May 1, 2023

Reference implementation: onflow/cadence#2112

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 proposal!

cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
cadence/2022-10-24-interface-inheritance.md Outdated Show resolved Hide resolved
If a user needed to access the `Foo` struct coming from the super interface `Token`, then they can
access it using the fully qualified name. e.g: `let foo: Token.Foo`.

However, it is not allowed to have two or more inherited type/events definitions with identical names for an interface.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this forbidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because of the type requirements. If there are types with identical names, the concrete type that implements these two interfaces might not be able to satisfy them both. So here it is taking the most restricted approach by rejecting such conflicts.

However, this can be relaxed after the type requirements are removed.

cadence/2022-10-24-interface-inheritance.md Show resolved Hide resolved
Co-authored-by: Bastian Müller <bastian@turbolent.com>
@SupunS SupunS force-pushed the supun/interface-inheritance branch from c8a1a44 to df280d3 Compare May 1, 2023 23:15
@SupunS SupunS force-pushed the supun/interface-inheritance branch from df280d3 to 38d5c9a Compare May 1, 2023 23:17
Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

I'm pretty happy with this! Just had one comment

cadence/2022-10-24-interface-inheritance.md Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/interface-inheritance branch from 6f86d86 to f9737a2 Compare May 3, 2023 23:30
@SupunS SupunS force-pushed the supun/interface-inheritance branch from f9737a2 to 2d86db5 Compare May 3, 2023 23:31
@SupunS
Copy link
Member Author

SupunS commented May 11, 2023

Thanks everyone for the suggestions and feedback for this proposal so far!

This FLIP has been open for a while, and it seems we are all happy with the current proposal, and seems there are no any remaining concerns.
Thus, I'm going to consider this FLIP as accepted 🎉 🎉 🎉

You can follow how the implementation is progressing here ➡️ onflow/cadence#2112

@SupunS SupunS merged commit 048e02c into main May 11, 2023
@SupunS SupunS deleted the supun/interface-inheritance branch May 11, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Drafted
Development

Successfully merging this pull request may close these issues.

5 participants