Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

[Feature Request]Introduce Visibility to Struct #139

Open
jolestar opened this issue May 15, 2022 · 2 comments
Open

[Feature Request]Introduce Visibility to Struct #139

jolestar opened this issue May 15, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@jolestar
Copy link
Collaborator

馃殌 Feature Request

Motivation

Currently, Move's structs use implicit visibility, e.g.

struct Foo{
   f1: u64,
}

is equivalent to.

public struct Foo{
   private f1: u64, 
}

Also, an implicit global storage access restriction is used to ensure that a struct can only be borrowed in the module in which it is defined.

This is actually equivalent to a conditional visibility constraint and has the same effect as adding a private visibility constraint to the struct.

private struct Foo has key{
}

So I propose to introduce the visibility of structs. This leaves it up to the smart contract developer to decide on access control for the global storage and the struct fields.

For example.

module MyModule {

   public struct Foo has key{
        public f1:u64,
   }

   private struct Bar has key{
        f1:u64,
   }
  
   public fun do_some(){
       let foo = borrow_global<Foo>();
       let bar = borrow_global<Bar>();
      // Both of these work 
   }
}

module XXX {
    use MyModule::Foo;
    public fun so_some(){
        let foo = borrow_global<Foo>();
        let f1 = foo.f1

        let bar = borrow_global<Bar>();
        // the first one will succeed, the second one will fail
    }
}

Of course, compatibility is a bigger problem, this is just an idea. If it works, I can do more work on it.

Migrate from diem/move, previous discussion diem/move#157

@jolestar jolestar added the enhancement New feature or request label May 15, 2022
@wrwg
Copy link
Member

wrwg commented May 15, 2022

I wonder what we think how important this feature is. I would like to have it in the language (also in the context of our discussion of structs allowed as transaction parameters), but I'm not very much missing that I cannot create structs and access fields without wrapper functions outside of the declaring module. So I'm lukewarm about the relevance, specifically as we concluded to deal with transaction structs on adapter side.

The background why am I asking this is that this not trivial to implement with the way file format is defined today. A StructDefinition is local to the module where a struct is defined, and the existing opcodes for making a struct and borrowing fields are tailored to the expectation that the struct is local to a module. (I happen to remember that it was not trivial to implement the Move Prover on top of file format -- specifications have access to private structs and fields -- we needed to create move-model to deal with this).

One can always work around the current restrictions by introducing public constructor and accessor functions. Maybe instead of supporting public fields, we should derive those functions automatically i.e. have a #[derive(..)] attribute?

@sblackshear @tnowacki

@tnowacki
Copy link
Member

  • I think that we should definitely have public/private structs. Particularly given how much type-driven programming we have in Move
  • I'm agree with Wolfgang in that I'm not sure how important public/private field access is in the short term
  • Long term I think it might be something we add, the biggest reason is for borrows.
    • Imagine you want to do something like the following: let f = &mut s.f; let g = &mut s.g; foo(f, g)
    • This is not possible with public borrow_mut functions, as the borrow checker has no way of knowing that the function only borrows a specific field (this is lacking even in Rust FWIW, though I do think we might be able to do better if we wanted to)
  • We might also want to do it for public/private constructors in the same way that Rust supports them
    • But we could also make this a source only feature, and there are several ways I could see exposing that
    • I would also be fine adding fake public fields as a source only feature, and just not allowing &mut field borrows right away due to the limitation described above

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants