[engine] remove Field type in favor of using String directly #4256

Merged
merged 4 commits into from Feb 13, 2017

Conversation

Projects
None yet
2 participants
@baroquebobcat
Contributor

baroquebobcat commented Feb 13, 2017

Problem

The Field type is opaque to the Rust code, which makes passing strings as field names from the Rust side awkward (see #4207)

Solution

  • Replace Field usages with String on the Rust side.
  • Use Buffers for the Python -> Rust components, and (ptr, len) for Rust -> Python.
  • Pass Rust a type id for Python's str so that externs that do type checks stay happy.

Result

Rust can now reference fields with strings instead of needing to be passed a string from Python.

@stuhood

Thanks Nick!

-def extern_project_multi(context_handle, val, field):
+@_FFI.callback("Value(ExternContext*, Value*, uint8_t*, uint64_t)")
+def extern_project_no_type(context_handle, val, field_str_ptr, field_str_len):
+ """Given a Value for `obj`, a field name, and a type, project the field as a new Value."""

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

xx: "and a type"

@stuhood

stuhood Feb 13, 2017

Member

xx: "and a type"

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

Thought I'd removed this. I'd extracted this patch from some work in progress. I'll rm it as it's unused.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

Thought I'd removed this. I'd extracted this patch from some work in progress. I'll rm it as it's unused.

return c.to_value(Exception(msg))
+def to_py_str(msg_ptr, msg_len):
+ msg = bytes(_FFI.buffer(msg_ptr, msg_len)).decode('utf-8')

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

return bytes(..)

@stuhood

stuhood Feb 13, 2017

Member

return bytes(..)

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

rgr. intellij method extraction in python leaves something to be desired.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

rgr. intellij method extraction in python leaves something to be desired.

@@ -60,11 +60,6 @@ pub struct TypeConstraint(pub Id);
#[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)]
pub struct Function(pub Id);
-// The name of a field.

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

Might consider leaving this behind as pub type Field = String; ? I don't feel strongly though.

@stuhood

stuhood Feb 13, 2017

Member

Might consider leaving this behind as pub type Field = String; ? I don't feel strongly though.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

Yeah. I meant to ask whether you'd prefer that. I think having the alias clarifies the purpose, I'll add it back in.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

Yeah. I meant to ask whether you'd prefer that. I think having the alias clarifies the purpose, I'll add it back in.

src/rust/engine/src/externs.rs
@@ -117,19 +120,28 @@ impl Externs {
(self.store_list)(self.context, values_clone.as_ptr(), values_clone.len() as u64, merge)
}
- pub fn project(&self, value: &Value, field: &Field, type_id: &TypeId) -> Value {
- (self.project)(self.context, value, field, type_id)
+ pub fn project(&self, value: &Value, field: &String, type_id: &TypeId) -> Value {

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

Use of &String should not be necessary: String is generally an "owned" type, so if you want to loan out a reference to it, you should use a &str (ie, "string slice"), which is more flexible.

An advantage of this is that you should be able to call project(value, "this_is_a_literal_string") directly, because literals are of type &str. For callers that have a String, they'd pass my_string.as_str(), which is a free cast.

@stuhood

stuhood Feb 13, 2017

Member

Use of &String should not be necessary: String is generally an "owned" type, so if you want to loan out a reference to it, you should use a &str (ie, "string slice"), which is more flexible.

An advantage of this is that you should be able to call project(value, "this_is_a_literal_string") directly, because literals are of type &str. For callers that have a String, they'd pass my_string.as_str(), which is a free cast.

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

(but, makes the typedef I mentioned above a little less useful... hm.)

@stuhood

stuhood Feb 13, 2017

Member

(but, makes the typedef I mentioned above a little less useful... hm.)

This comment has been minimized.

@stuhood

stuhood Feb 13, 2017

Member

Well, no. It would still be used in 99% of places. The only exception would be in the extern itself.

@stuhood

stuhood Feb 13, 2017

Member

Well, no. It would still be used in 99% of places. The only exception would be in the extern itself.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

So, switch to &str? I think that's reasonable.

@baroquebobcat

baroquebobcat Feb 13, 2017

Contributor

So, switch to &str? I think that's reasonable.

@stuhood

Thanks!

@baroquebobcat baroquebobcat merged commit d3d02b7 into pantsbuild:master Feb 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] remove Field type in favor of using String directly (#4256)
### Problem
The `Field` type is opaque to the Rust code, which makes passing strings as field names from the Rust  side awkward (see #4207)

### Solution

* Replace `Field` usages with `String` on the Rust side. 
* Use `Buffer`s for the Python -> Rust components, and `(ptr, len)` for Rust -> Python.
* Pass Rust a type id for Python's `str` so that externs that do type checks stay happy.

### Result

Rust can now reference fields with strings instead of needing to be passed a string from Python.

closes #4207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment