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 for removing restricted types #85

Merged
merged 8 commits into from
Jun 21, 2023
Merged

Conversation

dsainati1
Copy link
Contributor

This proposes to remove restricted types and replace them with InterfaceSet types, as they are no longer necessary after the proposed changes in #54.

@dsainati1 dsainati1 self-assigned this May 5, 2023
@bluesign
Copy link
Collaborator

supporting 100%

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 not sure if keeping this feature is really much of a problem, especially if it makes the migration easier. It is fine in theory though

@dsainati1
Copy link
Contributor Author

Keeping the feature definitely isn't a problem per se, but it is now a vestigial part of the language, and given our commitment to supporting the feature set of Stable Cadence, this would really be our last chance to remove it. Given that we're already planning to break every contract anyways, it doesn't seem like that much additional overhead to use this opportunity to remove this while we still can.

@turbolent
Copy link
Member

I think this change makes sense, and might even avoid confusion and security issues. Older documentation, tutorials, blog posts, code, etc. might still use restricted references for access control and developers might continue to follow these examples. A combination of entitlements (which "replace" restricted references for access control) and restricted type does not make a lot of sense.

@SupunS This change could actually be a solution to the current problem we ran into yesterday: Built-in members are not available on restricted types.

@bjartek
Copy link
Contributor

bjartek commented Jun 1, 2023

I am not against the feature, but there needs to be a clear migration path. If a contract stores something like

access(self) let cap: Capability<&{MetadataViews.ResolverCollection}>
in a field there needs to be a way to migrate it. And we also would need to do it expand collapse, so there is a version that supports both and then this old restricted types is removed or?

@dsainati1
Copy link
Contributor Author

there needs to be a clear migration path

The migration path is pretty clear imo: the example you give with Capability<&{MetadataViews.ResolverCollection}> would not need to be migrated at all: this would simply be left as is. The only part of restricted types that is being removed is the outer T in a type of the form T{I}. So an existing Capability<&Vault{Provider}> reference would become invalid, but can be easily migrated to Capability<&Vault>.

@bluesign
Copy link
Collaborator

bluesign commented Jun 2, 2023

The only part of restricted types that is being removed is the outer T in a type of the form T{I}. So an existing Capability<&Vault{Provider}> reference would become invalid, but can be easily migrated to Capability<&Vault>.

nit: I think correct statement is {I} is removed, no? ( except AnyResource case, where then just interface )

@dsainati1
Copy link
Contributor Author

Yes, I suppose for the purpose of the migration in this case it would be the {I} part of the annotation that is being removed. The point I was trying to make was that the T part of that type is being removed from the language, but it is perhaps a little unclear.

@turbolent
Copy link
Member

It just occurred to me that this is also great from a syntactical perspective.

The removal of T in the syntax T{Us} would remove the ambiguity for functions in interfaces, e.g. fun foo(): T {}, which might mean that the function returns a T and has an empty body, or which returns T{} and has no body. Currently this is disambiguated through whitespace, which is bad.

Maybe we could If we could further change the syntax and bring it closer to similar features in other languages? It would be great to remove the overloading of the { token, as it is also used for dictionaries. For example, when parsing {T, U}, until the , token occurs it is unclear if it is a restricted type or a dictionary.

For example, TypeScript, Swift, and Scala have the syntax T & U & V ..., for intersection types, protocol composition, and intersection types respectively.

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.

Big 👍 from me

cadence/20230505-remove-restricted-types.md Show resolved Hide resolved
cadence/20230505-remove-restricted-types.md Show resolved Hide resolved
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.

👍

@dsainati1
Copy link
Contributor Author

This seems like it's reached consensus, so I am updating the FLIP to accepted and merging this PR.

@dsainati1 dsainati1 merged commit 395a112 into main Jun 21, 2023
@dsainati1 dsainati1 deleted the remove-restricted-types-flip branch June 21, 2023 13:50
@turbolent
Copy link
Member

turbolent commented Jun 21, 2023

Could we maybe still discuss the opportunity for changing the syntax as noted above?

@dsainati1
Copy link
Contributor Author

Perhaps a separate FLIP? Changing the syntax of intersection types does feel like a parallel change to this proposal.

dsainati1 added a commit that referenced this pull request Jul 11, 2023
* add flip

* Update cadence/2023-05-05-remove-restricted-types.md

Co-authored-by: Bastian Müller <[email protected]>

* add info about migration of entitled references

* Update cadence/2023-05-05-remove-restricted-types.md

Co-authored-by: Bastian Müller <[email protected]>

* rename

* respond to review

* Update cadence/20230505-remove-restricted-types.md

---------

Co-authored-by: Bastian Müller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants