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

add generator::iter, clarify move/clone/reference for algebraic types #196

Merged
merged 2 commits into from
Dec 5, 2020

Conversation

tgolsson
Copy link
Contributor

@tgolsson tgolsson commented Dec 4, 2020

This adds generator::iter and INTO_ITER protocols.

Also, I noticed that the clone vs auto-ref vs move behaviour of Options/Result was unclear and so I've opted to implement as_ref() using an autoboxing approach. This isn't... optimal, performance-wise, but should be safe.

As far as I can tell this should work already using the defined ToValue methods for Option<T: ToValue> and &Value, but that code path causes the SharedBox to drop/inc out of order and I'm not sure why - and this is more explicit. It's a bit lewd to write to self in what should be a pure operation but it works and is safe.

Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Thanks! I'm fine with the other changes 👍 but I'd like to know what the overall goal is w/ the added Result::as_ref?

I also saw there was a long discussion on Discord, so forgive me if I'm repeating things. Just want a complete record here.

Two things to take note:

  • The closest thing to a reference in Rune is the clone of a value, which means a refcount increase for complex types.
  • We don't support references of primitive types (all though this might be supported in the future by boxing them).

unsafe fn unsafe_coerce(output: Self::Output) -> Self {
&mut *output
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These impls I don't mind adding for completeness sake 👍

}
}

fn as_ref_impl(result: &mut Result<Value, Value>) -> Result<Result<Value, Value>, VmError> {
Copy link
Collaborator

@udoprog udoprog Dec 5, 2020

Choose a reason for hiding this comment

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

I believe the effective outcome of this function is the same as cloning result.

  1. &result auto-derefs to a reference of result and we pattern match over its variants.
  2. The implementation of ToValue for &Value is to clone it.
  3. We clone the value in the variant when building a new variant.
  4. We then clone the result again at the bottom of the fn?

So I think this effectively has the same effect if we replace it with Ok(result.clone()) minus a bit of extra cloning unless I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this isn't actually doing what I expected. What I wanted and thought I got is Shared, this still doesn't work for primitives. Bah.

}
}

fn map_impl(this: &Result<Value, Value>, then: Function) -> Result<Result<Value, Value>, VmError> {
fn map_impl(this: Result<Value, Value>, then: Function) -> Result<Result<Value, Value>, VmError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify,this means that .map will now take ownership of the Result type it maps over.

@tgolsson
Copy link
Contributor Author

tgolsson commented Dec 5, 2020

Yeah, this is probably me not understanding how Value works, but I think this requires some work. The "bad case" I found during day 4 was code like this:

fn do_something(v) {
	false
}

struct Data {
	field
}

fn validate(v) {
	if !do_something(v.field?) {
		println!("field failed: {:?}", v.field);
	}
}

pub fn main() {
	let d = Data { field: Some("hello") };
	validate(d)
}

It's correct, but also... not what I wanted. And I found no way to do this easily without moving from the value, or explicitly cloning it. Maybe all operations on Option and Result should then take references on the Rust side and not values? And there's other cases where I'm not sure what the correct semantics would be but where I can't express what I want because I can't qualify how I want it to behave, e.g.,

let v = Some(10);
if let Some(v) = v {
     v += 10;
}
dbg(v); // 10

@@ -2322,14 +2322,14 @@ impl Vm {
let value = self.stack.pop()?;

let value = match value {
Value::Option(option) => match option.take()? {
Some(value) => value,
Value::Option(option) => match &*(option.borrow_ref()?) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parens not needed! (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems so, will remove!

@udoprog
Copy link
Collaborator

udoprog commented Dec 5, 2020

Thanks! Unwrap taking the value is such an obvious mistake in hindsight!

@udoprog udoprog merged commit 5a5039e into rune-rs:main Dec 5, 2020
@udoprog udoprog added enhancement New feature or request changelog Issue has been added to the changelog labels Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issue has been added to the changelog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants