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

Experimental: index, index_assign, and nested storage maps #4331

Closed
wants to merge 7 commits into from

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Mar 23, 2023

Description

Introduce index and index_assign to StorageMap and showcase how storage maps and nested storage maps could look like with and without the [] operator. These two methods should really be part of traits Index and IndexAssign but we're not ready to implement those yet because we do not have associated types for traits yet.

In any case, de-sugaring to index was implemented but de-sugaring to index_assign needed some work in how reassignments are handled.

Note that IndexAssign is inspired by rust-lang/rfcs#1129. This is probably the best we can do given that we don't want references in the language just yet (and probably ever).

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/nested_maps branch 2 times, most recently from 3945de9 to 3b94da2 Compare March 23, 2023 17:05
@mohammadfawaz mohammadfawaz self-assigned this Mar 23, 2023
@mohammadfawaz mohammadfawaz added lib: std Standard library testing General testing labels Mar 23, 2023
@mohammadfawaz mohammadfawaz changed the title New method get_handle to help implementing nested storage maps Experimental: New method get_handle to help implementing nested storage maps Mar 23, 2023
@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/storage_key branch 3 times, most recently from 648b2f0 to be85a10 Compare March 24, 2023 15:36
@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/nested_maps branch 2 times, most recently from 893905a to c7a2e45 Compare March 24, 2023 18:47
@mohammadfawaz mohammadfawaz changed the title Experimental: New method get_handle to help implementing nested storage maps Experimental: index, index_assign, and nested storage maps Mar 24, 2023
@mohammadfawaz mohammadfawaz added language feature Core language features visible to end users compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Mar 24, 2023
Copy link
Contributor

@mitchmindtree mitchmindtree 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 a bit weary of forging ahead with specialized indexing for the storage types while we don't have a clear idea of how we'll approach the Index trait just yet.

E.g. these implementations take self: StorageHandle<Self>, which would likely differ from the actual method signature we are likely to want on an Index trait. If we're to end up allowing specialized indexing on storage types that differs from an Index trait behaviour but both use the same [] syntax sugar, I'm worried we'll just cause more confusion for devs?

One alternative could be similar to the suggestion mentioned here, and allow for implementing the Index trait for StorageHandle<StorageMap<K, V>> (rather than using self type ascription). E.g.

trait Index<Ix> {
    type Output;
    fn index(self, ix: Ix) -> Self::Output;
}

impl<K, V> Index<K> for StorageHandle<StorageMap<K, V>> {
    type Output = StorageHandle<V>;
    fn index(self, ix: K) -> Self::Output {
        self.get(ix)
    }
}

This would not incur the same issues w.r.t. orphan rules as mentioned at the linked comment as the Index trait would likely be declared in the same library that provides the implementation (i.e. sway-lib-std).

* Added `try_read()` that behaves like `read` but returns an `Option`
* Changed `get` in `StorageMap` to return a `StorageHandle` instead of
  the value directly.
@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/storage_key branch 4 times, most recently from 985cde0 to 342094c Compare April 5, 2023 01:39
- Remove support for type ascription on `self` for now
- Instead, we now do `impl StorageKey<StorageMap<K, V>>` which seems to
  work just as well
… nested maps

using both `get/insert` and the `[]` operator
Base automatically changed from mohammadfawaz/storage_key to master April 13, 2023 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen language feature Core language features visible to end users lib: std Standard library testing General testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants