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

feat(ops): Implement #[state] for automatic OpState extraction #29

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jul 10, 2023

This provides a useful shortcut for OpState-based parameters using a #[state] annotation. Parameters annotation w/#[state] can be either &Something, &mut Something, Option<&Something>, or Option<&mut Something>.

Note that due to the Rust aliasing rules, only one &mut Something is allowed. This might be fixable in the future if we modify how the internal state system works. In many cases, a single mutable borrow should be sufficient.

Comment on lines +915 to +917
#[state] value32: &u32,
#[state] value16: &u16,
#[state] value8: Option<&u8>,
Copy link
Member

Choose a reason for hiding this comment

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

This feels very magical... I'm not sure how I feel about it. Is there a way to drop this borrow somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpState is borrowed during the entire time we're in these functions (regardless of whether we're doing this with the helpers or directly against OpState) -- the borrow on GothamState is a fake borrow that is more like a "deref the item with this key".

Technically you can control the lifetime of the borrow using Rc<RefCell> but usually we just assume that OpState is locked during the entire run of a sync function.

Copy link
Contributor Author

@mmastrac mmastrac Jul 10, 2023

Choose a reason for hiding this comment

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

This just desugars to:

let a: &A = opstate.borrow().borrow::<A>()
let b: &B = opstate.borrow().borrow::<B>()

Where it can be extremely useful is if we tweak the state to allow for multiple mutable borrows that might not be as ergonomic as this, or if we decide to change the state interface.

For example, we could reduce the cost of OpState lookups by turning types into keys that index into a slice.

@mmastrac mmastrac merged commit a822505 into denoland:main Jul 10, 2023
4 checks passed
@mmastrac mmastrac mentioned this pull request Jul 19, 2023
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants