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

Permissions #3380

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Permissions #3380

wants to merge 8 commits into from

Conversation

DasLixou
Copy link

@DasLixou DasLixou commented Jan 30, 2023

Just wanna share my idea of a language with permissioned functions.
Feedback welcome, really interested in this 😊

Rendered

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jan 30, 2023
@TimNN
Copy link
Contributor

TimNN commented Jan 31, 2023

There have been a number of previous discussions on similar topics, e.g. https://internals.rust-lang.org/t/view-types-based-on-pattern-matching/16879 or #1215 in this repo.

Some terms that have been applied to this kind of functionality are "partial borrows" or "view types".

Comment on lines 13 to 18
So I've been latetly writing a lot of rust code where I need a struct mutable borrowed and stored paralelly to other borrows of the same data.
They really don't interop or block each other, they are two iterators nested and one needs the whole type to call a function on it (immutable) and get that data.
The other iterator needs a mutable borrow of an underlying `HashMap` in that same struct to convert the key to a mutable ref to the value.
But these two iterators nested wouldn't work, because they would both reference to the same data.
To resolve the error: ```cannot borrow `*app` as mutable more than once at a time
second mutable borrow occurs here```, I've just be using unsafe pointer magic..:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you write this down in code? I find it hard to understand what you mean.

text/0000-permissions.md Outdated Show resolved Hide resolved
text/0000-permissions.md Show resolved Hide resolved
text/0000-permissions.md Outdated Show resolved Hide resolved
@kornelski
Copy link
Contributor

I think this kind of functionality is definitely needed in Rust. Getters and setters are too restrictive, and it's not always feasible to expose the fields or split structs.

Compared to previous proposals, an interesting development here is having abstract names for sets of loans, instead of exposing field names in the interface.

@blueglyph
Copy link

Each time we read or write code like this, we would need to check which permission allows what, to understand the implication and to make sure it's correct. It seems to go against the visibility that Rust has maintained when it comes to safety features.

  let department = Department(&mut person); // implicit permission
  let calendar = Calendar(&'Age mut person); // explicit permission

This also means that you create an access partition in your methods, right? Because now you have to ensure that Age and Name don't have any common field mut reference. This will restrict what you can do in the "base" object methods, which may prove more an annoyance than a benefit. Unless you duplicate the methods to avoid those with permissions if you need, for example, to mutate both age and name... but this implies that these other methods cannot be used by Department or Calendar, thus further complicating the comprehension of the code (and the job of the compiler).

Also, permissions are defined in the "base" object, not by other code that reference it or in trait implementations. So I'm not sure how the abstraction can be an advantage, unless you create your object knowing exactly how to partition the mutability to anticipate future needs. Or will you anticipate all possibilities? Perhaps in some case it's obvious, but it seems to only make sense if you already know how it will be used, so it's probably not meant to be exposed outside the scope of a crate often. Maybe it's just not the right example, because here you are directly accessing fields. Maybe a more abstract permission name makes more sense with public methods.

I would venture that the compiler should already be able, theoretically, to determine whether two functions requiring mutability have an overlap in their write accesses or not. But the visibility wouldn't be good, it would be somewhat improved with an explicit declaration like the one proposed here. Yet it would not be visible enough in my opinion, unless maybe it's based on fields rather than a permission abstraction, and explicit when calling the methods.

PS: Just my nit-picking self: you start your motivation by "So", as if it were a conjunction, but there is no previous text that this could result of. I would remove it and start by "I've been writing".

@DasLixou
Copy link
Author

DasLixou commented Feb 2, 2023

Yeah.. this was just a stupid idea i wanna write out. #1215 seems way more well thought out

@konsumlamm
Copy link
Contributor

Yeah.. this was just a stupid idea i wanna write out. #1215 seems way more well thought out

Note that #1215 isn't actually an RFC, it's just an issue. Someone would have to go ahead and make a PR for it.

@matthieu-m
Copy link

Yeah.. this was just a stupid idea i wanna write out. #1215 seems way more well thought out

Just because an idea is not fully fleshed out doesn't mean it's stupid. The very RFC process is about coming all together to improve on an initial idea after all.


With regard to permissions, the compiler should be able to check them to ensure safety.

This, in turn, requires that fields be annotated with the permissions, which then lets the borrow-checker:

  1. Validate that only functions declaring the right permissions can access the fields.
  2. Partition the borrows to only the set of fields tagged with the permission.
  3. Refuse concurrent mutable borrows of overlapping permissions -- ie, permissions that both borrow a common field.

Throw in an (implicit) 'All permission which borrows all fields (thus overlaps with all other permissions) and is implicitly assumed whenever a method doesn't specify a permission -- the most common case -- and... I think this works?

@DasLixou
Copy link
Author

DasLixou commented Feb 2, 2023

Yeah but there are also other thinks where we would like Permissions but which just won’t work. For example: HashMap. What If I want a permission per key. Like 'Person("Mike") could mean that it has access to the hashmap entry with the key Mike but that won’t be really a readable permission… I think there’s no good way to fix this.. so permissions would only work for fields which doesn’t really gets fully rid of the problem..

@AaronKutch
Copy link

This sounds like a thing that could be accomplished by crates. For example, putting RefCells in my arena crate https://crates.io/crates/triple_arena and using its extremely flexible immutable borrowing could probably achieve what you want. Most algorithms don't need more than two mutable borrows at once (achievable without RefCells with get2_mut, although there are more general get-multiple-muts I could write for my crate), often you can use multiple arenas to split off orthogonal structures or use consecutive mutable borrows.

If there is something that could be improved in Rust, it is some mechanism of allowing multiple immutable and mutable borrows of elements of a resizable data structure, if the compiler can verify that there are no movement or resizing operations while the borrows are held, and keys are unique. That is the problem I run into the most when using my arena or other similar structures.

@DasLixou
Copy link
Author

DasLixou commented Feb 2, 2023

Yeah but other than RefCells permissions should be a syntactic safety that it is safe without having any things in runtime

@Aloso
Copy link

Aloso commented Feb 3, 2023

I mostly like this proposal, but I have some suggestions:

The RFC should mention in the first sentence that this allows partially borrowing a struct. Many people want this from what I heard, so the RFC has a good chance of being accepted if it is well written.

My other suggestion is to make it explicit what permissions exist, for example:

struct Foo {
    bar: i32,
    baz: (String, bool),
}

impl Foo {
    permission 'Bar(self.bar);
    permission 'Quux(self.baz.0);

    fn get_bar(&'Bar self) -> i32 {
        self.bar
    }
}

The permissions declare up front which fields they allow to borrow, so the compiler can check if permissions overlap, and when a method accidentally uses a field it isn't permitted to, it will produce an error.

The keyword permission can be context-aware, so no edition is needed to introduce it. If a shorter keyword is desired, I like view.


One thing I don't like is that it uses the lifetime syntax, because that makes it impossible to set both a lifetime and a permission. An alternative is to make permissions behave mostly like type aliases:

impl Foo {
    permission Bar(self.bar);
    permission Quux(self.baz.0);

    fn get_bar(self: &Self::Bar) -> i32 {
        self.bar
    }
}

where Self::Bar is an alias for Self that only allows accessing the bar field.

Of course this begs the question how it works with visibility. If you have a pub(crate) permission Quux(self.quux), does this allow calling a pub fn get_quux(&Self::Quux) -> &i32 from another crate? I would say no, so changing whether private permissions overlap is never a semver-breaking change.

@DasLixou
Copy link
Author

DasLixou commented Feb 3, 2023

I also thought about directly declaring the permissions today lol. And yeah lifetime / labeled style may be too confusing.. maybe use *? Like

pub fn get(&*inner self) { .. }
// or `$`?
pub fn get(&$inner self) { .. }

?

DasLixou and others added 2 commits February 3, 2023 15:58
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
@DasLixou
Copy link
Author

DasLixou commented Feb 3, 2023

Ok so i changed everything up. Also came across the idea of just semantic permissions. Because sometimes (mostly in traits) you just want an internal function with for example pub(crate) but therefor you would need a second trait and need the one trait impl the other. Permissions would make this lot easier.

@clarfonthey
Copy link
Contributor

Small note reading this thread: the term "permissions" made me think of C++'s "friend" syntax (which is a mess) and then when I found out what it was about, seems still like a bad name.

I would recommend calling this explicit partial lifetimes, or named partial lifetimes, since it better describes the feature. I don't care either way, but I think it would help to pitch what the feature is actually for.

I personally don't like the proposal as it is, but don't have anything better to propose than what other folks have already said, so, I'll let everyone else discuss. But figured I'd pitch in an alternative colour for the bike shed so that folks looking at this have a better idea what it's about.

@burdges
Copy link

burdges commented Feb 5, 2023

It's all partial borrows in one flavor or another..

At present, you need methods like [T]::split_at_mut to do this, which requires writing explicit substructs, and thus more a design decisions.

Aside from view types and #1215 we also discussed this under #1546 which then became https://github.com/nikomatsakis/fields-in-traits-rfc ala nikomatsakis/fields-in-traits-rfc#16

I suspect fields-in-traits kinda blocks anything more ad hoc like this, so if you want some more ad hoc solution, then you might justify how the ad hoc solution works better somehow. If otoh you think fields-in-traits works best, then maybe attempt to make some progress there.

Imho, fields-in-traits works best because otherwise we'll largely loose partial borrows when programming generically.

@VitWW VitWW mentioned this pull request Apr 18, 2023
@VitWW VitWW mentioned this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.